Details

    • Testing Instructions:
      Hide

      This is quite difficult to test as we don't often use SimpleYUI and it isn't often obvious that the configuration is not being used. It will become more obvious in time as we Shift more and more modules.

      The easiest way of telling at present is to ensure that Y.log does not print your message when called directly on the Y object and when debug is disabled.

      For completeness, I would advise testing this before the patch is applied (sorry - I know how much of a pain that is), and comparing with the new result.

      • Set $CFG->debug = 0; in your config.php to disable debugging.
      • Open the JS console
      • Refresh a page
      • Confirm that no errors are shown
      • Enter:
        Y.log("MDL-38507");
      • Confirm that the log message was not displayed

      Note: You may see a YUI object displayed in the console. This is the return value of Y.log() and is not normally printed to the console. It's just a side-effect of using the console for this kind of testing.

      Show
      This is quite difficult to test as we don't often use SimpleYUI and it isn't often obvious that the configuration is not being used. It will become more obvious in time as we Shift more and more modules. The easiest way of telling at present is to ensure that Y.log does not print your message when called directly on the Y object and when debug is disabled. For completeness, I would advise testing this before the patch is applied (sorry - I know how much of a pain that is), and comparing with the new result. Set $CFG->debug = 0; in your config.php to disable debugging. Open the JS console Refresh a page Confirm that no errors are shown Enter: Y.log(" MDL-38507 "); Confirm that the log message was not displayed Note: You may see a YUI object displayed in the console. This is the return value of Y.log() and is not normally printed to the console. It's just a side-effect of using the console for this kind of testing.
    • Affected Branches:
      MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull Master Branch:
    • Rank:
      48502

      Description

      As discussed in the chat the other day, SimpleYUI is loaded (and thus set up) before we define YUI_config in the page header.

      You can see this by setting:

      $CFG->debug = 0;
      

      Refreshing your page, and in your browser JS console running:

      Y.log("This should not be shown");
      

      Which will of course display your message.

      Conversely:

      YUI().use('node', function(Y) { Y.log("This won't actually be shown"); });
      

      Will not actually show a message (though your browser may display info about the YUI() return value itself).

      The best option I see is to move the definition of YUI_config to above the inclusion of the simpleYUI script.

      This will need to be integrated after MDL-36198 and MDL-38391.

        Issue Links

          Activity

          Hide
          Andrew Nicols added a comment -

          Although we started using SimpleYUI in Moodle 2.4, there is little benefit in backporting these changes.

          At present, the main effect of these changes is that Y.log does not show output when debugging is disabled so the benefit is minimal.

          In the longer-run when we have issues such as MDL-38391 integrated, and have a significant number of modules defining meta-data, this will become more important as modules which are loaded using the SimpleYUI instance will be loaded without using the meta-data and therefore potentially incorrectly loaded. In reality people shouldn't load this way (they should use YUI().use() not Y.use()) but there may be a use-case we have not considered.

          I do not think that it makes sense to have a separate configuration for SimpleYUI as this could lead to confusion; or to use Y.applyConfig() instead as I believe that this will copy the config rather than reference it (http://yuilibrary.com/yui/docs/api/files/yui_js_yui.js.html#l255).

          Show
          Andrew Nicols added a comment - Although we started using SimpleYUI in Moodle 2.4, there is little benefit in backporting these changes. At present, the main effect of these changes is that Y.log does not show output when debugging is disabled so the benefit is minimal. In the longer-run when we have issues such as MDL-38391 integrated, and have a significant number of modules defining meta-data, this will become more important as modules which are loaded using the SimpleYUI instance will be loaded without using the meta-data and therefore potentially incorrectly loaded. In reality people shouldn't load this way (they should use YUI().use() not Y.use()) but there may be a use-case we have not considered. I do not think that it makes sense to have a separate configuration for SimpleYUI as this could lead to confusion; or to use Y.applyConfig() instead as I believe that this will copy the config rather than reference it ( http://yuilibrary.com/yui/docs/api/files/yui_js_yui.js.html#l255 ).
          Hide
          Petr Škoda added a comment - - edited

          +1, thanks for fixing it, I agree this is the right way

          Show
          Petr Škoda added a comment - - edited +1, thanks for fixing it, I agree this is the right way
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Nothing to argue, integrated (master only), thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Nothing to argue, integrated (master only), thanks!
          Hide
          Frédéric Massart added a comment -

          Passing, thanks!

          Show
          Frédéric Massart added a comment - Passing, thanks!
          Hide
          Damyon Wiese added a comment -

          This issue has been integrated upstream and is now available via git (and in some hours, via mirrors and downloads).

          Thanks for your contributions!

          Show
          Damyon Wiese added a comment - This issue has been integrated upstream and is now available via git (and in some hours, via mirrors and downloads). Thanks for your contributions!

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: