Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-55133

find_all_amd_modules() does not use proper frankenstyle for subsystems

    XMLWordPrintable

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Critical
    • Resolution: Fixed
    • Affects Version/s: 3.0.4, 3.1, 3.2
    • Fix Version/s: 3.0.6, 3.1.2
    • Component/s: JavaScript
    • Labels:
    • Testing Instructions:
      Hide

      Run phpunit tests.

      1. Add an AMD module to a subsystem (e.g. group)
      2. Try and "require" it without the "core_" prefix on the component name :

        require(['group/mymodule'], function(g) { console.log(g); });
        

        It should not work.

      3. Try and "require" it with the "core_" prefix on the component name :

        require(['core_group/mymodule'], function(g) { console.log(g); });
        

        It should work.

      (Note: if you run grunt on the above example code it will complain about console.log - either use --force or invent your own test code).

      Show
      Run phpunit tests. Add an AMD module to a subsystem (e.g. group) Try and "require" it without the "core_" prefix on the component name : require(['group/mymodule'], function(g) { console.log(g); }); It should not work. Try and "require" it with the "core_" prefix on the component name : require(['core_group/mymodule'], function(g) { console.log(g); }); It should work. (Note: if you run grunt on the above example code it will complain about console.log - either use --force or invent your own test code).
    • Affected Branches:
      MOODLE_30_STABLE, MOODLE_31_STABLE, MOODLE_32_STABLE
    • Fixed Branches:
      MOODLE_30_STABLE, MOODLE_31_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-55133-master

      Description

      Was integrating MDL-55000, first amd module belonging to a subsystem, apparently, when I detected that it was not using proper component name but just the subsystem name.

      So things like this was working:

      $PAGE->requires->js_call_amd('grades/edittree_index', 'enhance');
      

      But this was not (and is the correct one):

      $PAGE->requires->js_call_amd('core_grades/edittree_index', 'enhance');
      

      Finally, curiously using it in a -lazy way, was working ok with correct component name:

      $PAGE->requires->js_call_amd('core_grades/edittree_index-lazy', 'enhance');
      

      Following the code, the problem seems to be @ lib/classes/requirejs.php where the directories for subsystems are added by name, not by component. I just applied ths quick and dirty hack and then 'core_grades' started to work:

      diff --git a/lib/classes/requirejs.php b/lib/classes/requirejs.php
      index 9894dc5..ebe4e6f 100644
      --- a/lib/classes/requirejs.php
      +++ b/lib/classes/requirejs.php
      @@ -90,7 +90,7 @@ class core_requirejs {
               $subsystems = core_component::get_core_subsystems();
               foreach ($subsystems as $subsystem => $dir) {
                   if (!empty($dir) && is_dir($dir . '/amd')) {
      -                $jsdirs[$subsystem] = $dir . '/amd';
      +                $jsdirs['core_' . $subsystem] = $dir . '/amd';
                   }
               }
               $plugintypes = core_component::get_plugin_types();
      

      Surely we should change it to always use correct component names, both for plugins (working already) and subsystems, covered with unit tests, before we start spreading amd modules over subsystems. That will make things work as documented (by component name) and will allow us to use correct @module, @package and js_call_amd() calls. And also will allow to move some current modules (tag....) to their corresponding subsystem.

      For your consideration, ciao

      PS: I'd consider this a bug at all effects.

        Attachments

          Issue Links

            Activity

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  12/Sep/16