Details

    • Testing Instructions:
      Hide

      Apply the top commit from git://github.com/andrewnicols/moodle.git MDL-38391-testing (https://github.com/andrewnicols/moodle/commits/MDL-38391-testing)

      This adds some testing data with some sample good and bad modules.

      Open your browser JS tools - you'll need this.
      With the Network tab, you probably only want to view scripts.

      Whilst logged out:

      • open the login page

      When:

      unset($CFG->jsrev);
      

      confirm that the JS load for tooltip.js was comboloaded with a few other modules
      (I see 6 JS requests)

      Open the JS console and type:

      YUI_config.groups.moodle.modules
      

      You should get an array with various modules defined. On current integration/master I get 5:

      • moodle-core-38391good
      • moodle-core-tooltip
      • moodle-form-38391good
      • moodle-question-38391good
      • moodle-mod_assign-38391good

      These are modules from:

      • core
      • a core subsystem in a subdirectory
      • a core subsystem in a top-level directory
      • a plugin (module)

      You shouldn't see any with the name bad in them.

      When

      $CFG->jsrev = -1;
      

      confirm that the JS load for tooltip.js was /not/ comboloaded
      (I see 7 JS requests)

      Open the JS console and type:

      YUI_config.groups.moodle.modules
      

      An empty array should be returned []

      Show
      Apply the top commit from git://github.com/andrewnicols/moodle.git MDL-38391 -testing ( https://github.com/andrewnicols/moodle/commits/MDL-38391-testing ) This adds some testing data with some sample good and bad modules. Open your browser JS tools - you'll need this. With the Network tab, you probably only want to view scripts. Whilst logged out: open the login page When: unset($CFG->jsrev); confirm that the JS load for tooltip.js was comboloaded with a few other modules (I see 6 JS requests) Open the JS console and type: YUI_config.groups.moodle.modules You should get an array with various modules defined. On current integration/master I get 5: moodle-core-38391good moodle-core-tooltip moodle-form-38391good moodle-question-38391good moodle-mod_assign-38391good These are modules from: core a core subsystem in a subdirectory a core subsystem in a top-level directory a plugin (module) You shouldn't see any with the name bad in them. When $CFG->jsrev = -1; confirm that the JS load for tooltip.js was /not/ comboloaded (I see 7 JS requests) Open the JS console and type: YUI_config.groups.moodle.modules An empty array should be returned []
    • Affected Branches:
      MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull Master Branch:

      Description

      Now that shifter (MDL-37127) has landed, we need to look at writing a task for cron to trawl through all of the yui/src directories, pick up meta files, and compile those into a single json structure which can be passed to YUI_config['groups']['moodle']['modules']

      This should inform the loader of dependency and improve our combo loading massively.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            dobedobedoh Andrew Nicols added a comment -

            I should note, I think it is safe to run this on cron and we do not need to ship with an initial configuration or run this on every load. Where dependencies are incorrect, YUI handles this anyway. This system is only to seed the YUI loader to try and reduce the number of http requests.

            Show
            dobedobedoh Andrew Nicols added a comment - I should note, I think it is safe to run this on cron and we do not need to ship with an initial configuration or run this on every load. Where dependencies are incorrect, YUI handles this anyway. This system is only to seed the YUI loader to try and reduce the number of http requests.
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Also, thinking about it, this probably only needs to be done occasionally. For example, when a new plugin is installed, or on version increment.
            It may be possible to do this as an upgrade step or something, but that doesn't feel quite right.

            Show
            dobedobedoh Andrew Nicols added a comment - Also, thinking about it, this probably only needs to be done occasionally. For example, when a new plugin is installed, or on version increment. It may be possible to do this as an upgrade step or something, but that doesn't feel quite right.
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Adding Petr - he'll have good opinions on this.

            Show
            dobedobedoh Andrew Nicols added a comment - Adding Petr - he'll have good opinions on this.
            Hide
            skodak Petr Skoda added a comment -

            Hi,

            I do not like the proposed dependency on cron at all, I do not think this kind of precompilation belongs there. It should not imo depend on MUC or its resets.

            I think it might be enough to handle this in upgrade/install code only because after every change developers MUST bump up the version number in plugins or core. I am wondering if other plugin related meta information could be precompiled at the same time.

            Show
            skodak Petr Skoda added a comment - Hi, I do not like the proposed dependency on cron at all, I do not think this kind of precompilation belongs there. It should not imo depend on MUC or its resets. I think it might be enough to handle this in upgrade/install code only because after every change developers MUST bump up the version number in plugins or core. I am wondering if other plugin related meta information could be precompiled at the same time.
            Hide
            dobedobedoh Andrew Nicols added a comment -

            I've taken out the proposed dependency on cron after our discussion earlier.

            I've converted YUI_config into a new class with various helper functions to allow addition and updating of group configuration, module configuration, and getting and setting of module metadata from Shifted YUI modules.

            • The module metadata function use a new cache definition in MUC for YUI modules.
              • the cache key is set the first time it is used but does not exist - this should be on the first page view after an upgrade or cache purge
              • the cache key is cleared when Moodle caches are purged (or specifically JS caches are purged).
              • if JS caching is disabled (e.g. jsrev = -1 or cachejs = false) then the cache is not used at all (though it is not cleared either)
            Show
            dobedobedoh Andrew Nicols added a comment - I've taken out the proposed dependency on cron after our discussion earlier. I've converted YUI_config into a new class with various helper functions to allow addition and updating of group configuration, module configuration, and getting and setting of module metadata from Shifted YUI modules. The module metadata function use a new cache definition in MUC for YUI modules. the cache key is set the first time it is used but does not exist - this should be on the first page view after an upgrade or cache purge the cache key is cleared when Moodle caches are purged (or specifically JS caches are purged). if JS caching is disabled (e.g. jsrev = -1 or cachejs = false) then the cache is not used at all (though it is not cleared either)
            Hide
            skodak Petr Skoda added a comment -

            Hmm, why is the @GALLERYCONFIGFN@ magic necessary? Can we move it somehow to the new class?

            Also this is going to collide with my patch in MDL-36198, maybe it could be based on top of it...

            Show
            skodak Petr Skoda added a comment - Hmm, why is the @GALLERYCONFIGFN@ magic necessary? Can we move it somehow to the new class? Also this is going to collide with my patch in MDL-36198 , maybe it could be based on top of it...
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Hmm - the galleryconfigfn can be removed since I've removed the gallery config here (it didn't seem prudent to keep broken config in).
            We should be able to move it to the same class I think... I shall have a think.

            I'll rebase on your patch.

            One other question I've not thought of a suitable solution to yet, but should we clear the cache if jsrev = -1? I imagine that doing so would be a bad idea..?

            Show
            dobedobedoh Andrew Nicols added a comment - Hmm - the galleryconfigfn can be removed since I've removed the gallery config here (it didn't seem prudent to keep broken config in). We should be able to move it to the same class I think... I shall have a think. I'll rebase on your patch. One other question I've not thought of a suitable solution to yet, but should we clear the cache if jsrev = -1? I imagine that doing so would be a bad idea..?
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Petr Skoda Any idea why the existing config functions are written as variables? I assume it's to allow some magic so that they can be rewritten later on?

            Show
            dobedobedoh Andrew Nicols added a comment - Petr Skoda Any idea why the existing config functions are written as variables? I assume it's to allow some magic so that they can be rewritten later on?
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Your wish is my command, I've rebased on top of your patch and remove the GALLERYCONFIGFN.

            In order to try and keep all of the YUI_config together, I've written a function to take the /contents/ of each of the config functions, and generate a name to be used in that group's pattern definition.

            As a side-note, the reason behind the variables instead of functions is because json_encode converts them to strings (which is obviously utterly useless when you want a function or variable). I've maintained the general idea, but modified how it works slightly.

            Should be good to go for peer review now.

            Show
            dobedobedoh Andrew Nicols added a comment - Your wish is my command, I've rebased on top of your patch and remove the GALLERYCONFIGFN. In order to try and keep all of the YUI_config together, I've written a function to take the /contents/ of each of the config functions, and generate a name to be used in that group's pattern definition. As a side-note, the reason behind the variables instead of functions is because json_encode converts them to strings (which is obviously utterly useless when you want a function or variable). I've maintained the general idea, but modified how it works slightly. Should be good to go for peer review now.
            Hide
            dobedobedoh Andrew Nicols added a comment -

            MDL-36198 will need to be integrated first obviously.

            Show
            dobedobedoh Andrew Nicols added a comment - MDL-36198 will need to be integrated first obviously.
            Hide
            dobedobedoh Andrew Nicols added a comment -

            The only thing I haven't been able to solve with this is how to clear the cache after jsrev has been set to -1.
            It feels very wrong to delete the key on every page load when whilst developing, but equally as wrong to have to manually purge the cache after doing development work.

            Sam, or Petr: Do you have any advice on this one? What kind of overhead is there with instantiating the cache object to call delete each time.

            I notice from my work on this issue that we now set up a complete page object (including the entire YUI_config) each time that we use yui_combo.php and yui_image.php (and possibly javascript.php and image.php). I'm not sure if this is a bug as such, but it probably isn't great for performance and is probably a good area for improvement. This is a result of MDL-37598.

            Show
            dobedobedoh Andrew Nicols added a comment - The only thing I haven't been able to solve with this is how to clear the cache after jsrev has been set to -1. It feels very wrong to delete the key on every page load when whilst developing, but equally as wrong to have to manually purge the cache after doing development work. Sam, or Petr: Do you have any advice on this one? What kind of overhead is there with instantiating the cache object to call delete each time. I notice from my work on this issue that we now set up a complete page object (including the entire YUI_config) each time that we use yui_combo.php and yui_image.php (and possibly javascript.php and image.php). I'm not sure if this is a bug as such, but it probably isn't great for performance and is probably a good area for improvement. This is a result of MDL-37598 .
            Hide
            dobedobedoh Andrew Nicols added a comment -

            I think that moving YUI_config into it's own class (which is done as a part of this) will help us to implement the Gallery integration - providing that we can access the $PAGE object from the local plugin.

            Also, moving the configFns to this class, and refactoring them should help to prevent modules interfering with one another further.

            Show
            dobedobedoh Andrew Nicols added a comment - I think that moving YUI_config into it's own class (which is done as a part of this) will help us to implement the Gallery integration - providing that we can access the $PAGE object from the local plugin. Also, moving the configFns to this class, and refactoring them should help to prevent modules interfering with one another further.
            Hide
            skodak Petr Skoda added a comment - - edited

            I would personally delete the cache always to prevent problems when admins enable js caching via config.php.

            +1 for the rest, thanks a lot

            Show
            skodak Petr Skoda added a comment - - edited I would personally delete the cache always to prevent problems when admins enable js caching via config.php. +1 for the rest, thanks a lot
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Deleting the cache each time when jsrev == -1
            Have also renamed the protected method which actually does the work to fit with coding guidelines.

            As mentioned before, this needs to be integrated after MDL-36198.

            Show
            dobedobedoh Andrew Nicols added a comment - Deleting the cache each time when jsrev == -1 Have also renamed the protected method which actually does the work to fit with coding guidelines. As mentioned before, this needs to be integrated after MDL-36198 .
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Added two additional commits to add a note explaining use of camelCase in YUI_config class, and to remove unnecessary purging of MUC caches when core caches are purged (it's already handled elsewhere).

            Show
            dobedobedoh Andrew Nicols added a comment - Added two additional commits to add a note explaining use of camelCase in YUI_config class, and to remove unnecessary purging of MUC caches when core caches are purged (it's already handled elsewhere).
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Andrew, we agreed yesterday that this was missing still some points, yup? I'll hold this for some hours...

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Andrew, we agreed yesterday that this was missing still some points, yup? I'll hold this for some hours...
            Hide
            dobedobedoh Andrew Nicols added a comment -

            I've added the missing core subsystem support and renamed the moodle metadata adder from set_moodle_metadata to add_moodle_metadata.

            I'll provide some updated testing instructions at some point today to ensure that we fully test all types of metadata, and handle cases where we have incorrect naming structure of that metadata.

            Show
            dobedobedoh Andrew Nicols added a comment - I've added the missing core subsystem support and renamed the moodle metadata adder from set_moodle_metadata to add_moodle_metadata. I'll provide some updated testing instructions at some point today to ensure that we fully test all types of metadata, and handle cases where we have incorrect naming structure of that metadata.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Integrated, thanks!

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Integrated, thanks!
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Sorry Andrew,

            I am seeing opposite to what is described in Testing instructions.

            1. $CFG->jsrev = -1;
            • return []
            1. unset($CFG->jsrev); or not adding $CFG->jsrev to config;
            • return

              moodle-core-38391good: Object
              moodle-core-tooltip: Object
              moodle-form-38391good: Object
              moodle-mod_assign-38391good: Object
              moodle-question-38391good: Object
              

            Show
            rajeshtaneja Rajesh Taneja added a comment - Sorry Andrew, I am seeing opposite to what is described in Testing instructions. $CFG->jsrev = -1; return [] unset($CFG->jsrev); or not adding $CFG->jsrev to config; return moodle-core-38391good: Object moodle-core-tooltip: Object moodle-form-38391good: Object moodle-mod_assign-38391good: Object moodle-question-38391good: Object
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Corrected test instructions - they should be the other way around.

            Thanks for picking that up

            Show
            dobedobedoh Andrew Nicols added a comment - Corrected test instructions - they should be the other way around. Thanks for picking that up
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Thanks for awesome work, Andrew.

            Passing.

            Show
            rajeshtaneja Rajesh Taneja added a comment - Thanks for awesome work, Andrew. Passing.
            Hide
            damyon 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 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:
                1 Vote for this issue
                Watchers:
                5 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  14/May/13