Details

    • Type: New Feature
    • Status: Closed
    • Priority: Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.5
    • Fix Version/s: 2.6
    • Component/s: Libraries
    • Story Points (Obsolete):
      40
    • Sprint:
      BACKEND Sprint 1

      Description

      It would be nice to have an official class loader for Frankenstyle based class names in Moodle core.

      See http://docs.moodle.org/dev/Automatic_Class_Loading_Proposal

        Gliffy Diagrams

        1. core_component.php
          40 kB
          Andrew Davis
        2. core_component.php
          41 kB
          Rossiani Wijaya

          Issue Links

            Activity

            Hide
            skodak Petr Skoda added a comment -
            Show
            skodak Petr Skoda added a comment - Example code demonstrating the idea is here: https://github.com/skodak/moodle/compare/master...wip_MDL-39854_m26_classloading
            Hide
            rajeshtaneja Rajesh Taneja added a comment - - edited

            Specs looks good Petr.

            We need to look at usage of class_exists(), as it will call autoloader. Which seems to cause performance issues.

            For example front page as guest create:

            1. cachestore_file
            2. cachestore_session
            3. cachestore_static
            4. cachestore_memcache
            5. block_site_main_menu – This will call get_plugin_list and do processing.
            6. block_navigation
            7. block_settings
            8. block_course_summary
            9. block_calendar_month

            FYI: For demo course 67 classes are loaded.

            There are few things which we can consider for optimization:

            1. Investigate usage of class_exists()
            2. Load only those classes which use frankenstyle.
            3. Avoid extra processing.
            4. Will it be nice to provide helper api to register new loader? core_frankenstyle::registerclassloader()
            5. Consider using require_once/include_once
            6. Should check if file is_readable() rather than file_exists()
            Show
            rajeshtaneja Rajesh Taneja added a comment - - edited Specs looks good Petr. We need to look at usage of class_exists(), as it will call autoloader. Which seems to cause performance issues. For example front page as guest create: cachestore_file cachestore_session cachestore_static cachestore_memcache block_site_main_menu – This will call get_plugin_list and do processing. block_navigation block_settings block_course_summary block_calendar_month FYI: For demo course 67 classes are loaded. There are few things which we can consider for optimization: Investigate usage of class_exists() Load only those classes which use frankenstyle. Avoid extra processing. Will it be nice to provide helper api to register new loader? core_frankenstyle::registerclassloader() Consider using require_once/include_once Should check if file is_readable() rather than file_exists()
            Hide
            skodak Petr Skoda added a comment -

            Yes, I agree we need to minimise class_exists calls. file_exists() and include() should give you nice PHP error/warning. include_once() would hide potential coding errors, it seems PHP manual recommends that.

            The performance of get_plugin_types() can be significantly improved (used for first word of class name), that would hopefully resolve most perf issues with class_exists()...

            Show
            skodak Petr Skoda added a comment - Yes, I agree we need to minimise class_exists calls. file_exists() and include() should give you nice PHP error/warning. include_once() would hide potential coding errors, it seems PHP manual recommends that. The performance of get_plugin_types() can be significantly improved (used for first word of class name), that would hopefully resolve most perf issues with class_exists()...
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            It might be nice to pass second parameter as false for class_exists in blocklib.php (block_load_class) and probably in get_plugins to avoid call to autoload

            Show
            rajeshtaneja Rajesh Taneja added a comment - It might be nice to pass second parameter as false for class_exists in blocklib.php (block_load_class) and probably in get_plugins to avoid call to autoload
            Hide
            skodak Petr Skoda added a comment -

            I have updated the patch, the side effects are:
            1/ phpunit installation 2m20s --> 2m1s
            2/ phpunit execution 12min --> 9min
            3/ tiny bit less memory used
            4/ page load time seems to be lower

            It should also detect some more problems with frankenstyle plugin/component names. There are tests for all new methods and original functions that were replaced. Some minor bugs discovered by the tests were fixed (invalid core subsystem definitions, etc.)

            Show
            skodak Petr Skoda added a comment - I have updated the patch, the side effects are: 1/ phpunit installation 2m20s --> 2m1s 2/ phpunit execution 12min --> 9min 3/ tiny bit less memory used 4/ page load time seems to be lower It should also detect some more problems with frankenstyle plugin/component names. There are tests for all new methods and original functions that were replaced. Some minor bugs discovered by the tests were fixed (invalid core subsystem definitions, etc.)
            Hide
            skodak Petr Skoda added a comment - - edited

            Here is class_exists() test:

             
            $start = microtime(true);
            for($i=0;$i<1000000;$i++) {
                class_exists('mod_forum_xx_yy_zz'.$i, false);
            }
            echo "class exists (no classload): ".(microtime(true)-$start)."</br>";
             
            $start = microtime(true);
            for($i=0;$i<1000000;$i++) {
                class_exists('xxxx_x'.$i, true);
            }
            echo "classload non-moodle: ".(microtime(true)-$start)."</br>";
             
            $start = microtime(true);
            for($i=0;$i<1000000;$i++) {
                class_exists('mod_resource_aa'.$i, true);
            }
            echo "classload short: ".(microtime(true)-$start)."</br>";
             
            $start = microtime(true);
            for($i=0;$i<1000000;$i++) {
                class_exists('mod_resource_aa_bb'.$i, true);
            }
            echo "classload middle: ".(microtime(true)-$start)."</br>";
             
            $start = microtime(true);
            for($i=0;$i<1000000;$i++) {
                class_exists('mod_resource_xx_yy_zz'.$i, true);
            }
            echo "classload long: ".(microtime(true)-$start)."</br>";
             
            $start = microtime(true);
            for($i=0;$i<1000000;$i++) {
                class_exists('mod_resource_xx_yy_zz', true);
            }
            echo "classload long, repeated: ".(microtime(true)-$start)."</br>";
            

            class exists (no classload): 0.67539310455322
            classload non-moodle: 2.3796601295471
            classload short: 10.212798118591
            classload middle: 13.242247819901
            classload long: 17.886957883835
            classload long, repeated: 17.059753894806
            

            The results are for 1 million of class_exists(), where all of them are misses (the most expensive possibility).

            Please note the patch implements class loading from 3 different location of class files, if there was only one option the middle and long would be similar to short.

            I guess we would have a few hundreds of class_exists() misses on any moodle page, so the performance of class_exists() and class loading in general should be pretty good.

            Ideas?

            PS: I think we do not need to test class_exists() miss performance for core_ because we know what is there, no need to guess.

            Show
            skodak Petr Skoda added a comment - - edited Here is class_exists() test:   $start = microtime(true); for($i=0;$i<1000000;$i++) { class_exists('mod_forum_xx_yy_zz'.$i, false); } echo "class exists (no classload): ".(microtime(true)-$start)."</br>";   $start = microtime(true); for($i=0;$i<1000000;$i++) { class_exists('xxxx_x'.$i, true); } echo "classload non-moodle: ".(microtime(true)-$start)."</br>";   $start = microtime(true); for($i=0;$i<1000000;$i++) { class_exists('mod_resource_aa'.$i, true); } echo "classload short: ".(microtime(true)-$start)."</br>";   $start = microtime(true); for($i=0;$i<1000000;$i++) { class_exists('mod_resource_aa_bb'.$i, true); } echo "classload middle: ".(microtime(true)-$start)."</br>";   $start = microtime(true); for($i=0;$i<1000000;$i++) { class_exists('mod_resource_xx_yy_zz'.$i, true); } echo "classload long: ".(microtime(true)-$start)."</br>";   $start = microtime(true); for($i=0;$i<1000000;$i++) { class_exists('mod_resource_xx_yy_zz', true); } echo "classload long, repeated: ".(microtime(true)-$start)."</br>"; class exists (no classload): 0.67539310455322 classload non-moodle: 2.3796601295471 classload short: 10.212798118591 classload middle: 13.242247819901 classload long: 17.886957883835 classload long, repeated: 17.059753894806 The results are for 1 million of class_exists(), where all of them are misses (the most expensive possibility). Please note the patch implements class loading from 3 different location of class files, if there was only one option the middle and long would be similar to short. I guess we would have a few hundreds of class_exists() misses on any moodle page, so the performance of class_exists() and class loading in general should be pretty good. Ideas? PS: I think we do not need to test class_exists() miss performance for core_ because we know what is there, no need to guess.
            Hide
            timhunt Tim Hunt added a comment -

            I would like to help progress this issue by peer reviewing this change, but I don't have time this week, and I am on holiday next week. However, any patch that introduces a class called core_frankenstyle must be a good thing . Thanks for working on this Petr. I hope you find a peer reviewer.

            Show
            timhunt Tim Hunt added a comment - I would like to help progress this issue by peer reviewing this change, but I don't have time this week, and I am on holiday next week. However, any patch that introduces a class called core_frankenstyle must be a good thing . Thanks for working on this Petr. I hope you find a peer reviewer.
            Hide
            ankit_frenz Ankit Agarwal added a comment -

            Hi Petr,
            I looked at the patch briefly. Everything looks great, a few minor things you might want to consider

            1. public function test_deptecated_get_core_subsystems() - typo
            2. The deprecations need to be mentioned in respective upgrade.txt
            3. say I have a plugin "tool_plugin_awesome" , and it defines a class "tool_plugin_awesome_xx", we have the code (in core_frankenstyle::classloader) :-

                      for ($i=0; $i<5; $i++) {
                          if (!isset($plugins[$pluginname])) {
                              continue;
                          }
                          if (!$parts) {
                              break;
                          }
               
                          $fulldir = $plugins[$pluginname];
                          $candidate = implode('/', $parts);
                          $file = "$fulldir/classes/$candidate.php";
                          if (file_exists($file)) {
                              include($file);
                              return;
                          }
                          if (count($parts) > 1) {
                              $candidate = implode('_', $parts);
                              $file = "$fulldir/classes/$candidate.php";
                              if (file_exists($file)) {
                                  include($file);
                                  return;
                              }
               
                              if (count($parts) > 2) {
                                  $candidate = preg_replace('/_/', '/', $candidate, 1);
                                  $file = "$fulldir/classes/$candidate.php";
                                  if (file_exists($file)) {
                                      include($file);
                                      return;
                                  }
                              }
                          }
               
                          echo $pluginname .= '_' . array_shift($parts);
                      }

              $plugins[$pluginname] will never be set in this case, since tool_plugin doesn't exist. Thus failing the autoload.

            4. I also agree that only one consistent location is better than having 3 possible location, both in terms of performance and complexity.
            Show
            ankit_frenz Ankit Agarwal added a comment - Hi Petr, I looked at the patch briefly. Everything looks great, a few minor things you might want to consider public function test_deptecated_get_core_subsystems() - typo The deprecations need to be mentioned in respective upgrade.txt say I have a plugin "tool_plugin_awesome" , and it defines a class "tool_plugin_awesome_xx", we have the code (in core_frankenstyle::classloader) :- for ($i=0; $i<5; $i++) { if (!isset($plugins[$pluginname])) { continue; } if (!$parts) { break; }   $fulldir = $plugins[$pluginname]; $candidate = implode('/', $parts); $file = "$fulldir/classes/$candidate.php"; if (file_exists($file)) { include($file); return; } if (count($parts) > 1) { $candidate = implode('_', $parts); $file = "$fulldir/classes/$candidate.php"; if (file_exists($file)) { include($file); return; }   if (count($parts) > 2) { $candidate = preg_replace('/_/', '/', $candidate, 1); $file = "$fulldir/classes/$candidate.php"; if (file_exists($file)) { include($file); return; } } }   echo $pluginname .= '_' . array_shift($parts); } $plugins [$pluginname] will never be set in this case, since tool_plugin doesn't exist. Thus failing the autoload. I also agree that only one consistent location is better than having 3 possible location, both in terms of performance and complexity.
            Hide
            skodak Petr Skoda added a comment -

            1.-3. Thanks! done
            4. I am afraid that we would never come up to agreement with consistency for core and plugins. I am not going to waste time persuading others, so I did all 3 at the same time.

            Show
            skodak Petr Skoda added a comment - 1.-3. Thanks! done 4. I am afraid that we would never come up to agreement with consistency for core and plugins. I am not going to waste time persuading others, so I did all 3 at the same time.
            Hide
            rajeshtaneja Rajesh Taneja added a comment - - edited

            Hello Petr,

            At https://github.com/skodak/moodle/compare/master...wip_MDL-39854_m26_classloading#L6R235, should it be

            if (sha1_file($cachefile) === sha1($content)) {
            

            Also, at https://github.com/skodak/moodle/compare/master...wip_MDL-39854_m26_classloading#L6R223, can we not use $CFG->cachedir, rather then finding dirname. As this is hard-coded at https://github.com/skodak/moodle/compare/master...wip_MDL-39854_m26_classloading#L6R200

            Show
            rajeshtaneja Rajesh Taneja added a comment - - edited Hello Petr, At https://github.com/skodak/moodle/compare/master...wip_MDL-39854_m26_classloading#L6R235 , should it be if (sha1_file($cachefile) === sha1($content)) { Also, at https://github.com/skodak/moodle/compare/master...wip_MDL-39854_m26_classloading#L6R223 , can we not use $CFG->cachedir, rather then finding dirname. As this is hard-coded at https://github.com/skodak/moodle/compare/master...wip_MDL-39854_m26_classloading#L6R200
            Hide
            rajeshtaneja Rajesh Taneja added a comment - - edited

            In assignment https://github.com/skodak/moodle/compare/master...wip_MDL-39854_m26_classloading#L19R3823 get_plugin_list is returning full path of assignment type.
            So code should be altered to

            $assignmenttypes = array_keys(get_plugin_list('assignment'));
            

            Similarly https://github.com/skodak/moodle/compare/master...wip_MDL-39854_m26_classloading#L20R243 will give error.

            Also, https://github.com/skodak/moodle/compare/master...wip_MDL-39854_m26_classloading#L21R1882 fulldir is not required a we are getting full path of directory.

            Show
            rajeshtaneja Rajesh Taneja added a comment - - edited In assignment https://github.com/skodak/moodle/compare/master...wip_MDL-39854_m26_classloading#L19R3823 get_plugin_list is returning full path of assignment type. So code should be altered to $assignmenttypes = array_keys(get_plugin_list('assignment')); Similarly https://github.com/skodak/moodle/compare/master...wip_MDL-39854_m26_classloading#L20R243 will give error. Also, https://github.com/skodak/moodle/compare/master...wip_MDL-39854_m26_classloading#L21R1882 fulldir is not required a we are getting full path of directory.
            Hide
            skodak Petr Skoda added a comment -

            Thanks a lot, I was blind!

            Show
            skodak Petr Skoda added a comment - Thanks a lot, I was blind!
            Hide
            skodak Petr Skoda added a comment -

            I have updated the patch to support namespaces, that should significantly simplify the classes/ directory structure, see the forum for more info.

            Show
            skodak Petr Skoda added a comment - I have updated the patch to support namespaces, that should significantly simplify the classes/ directory structure, see the forum for more info.
            Hide
            skodak Petr Skoda added a comment - - edited

            I have added one more commit with class map cache that gets us to 2 seconds per 1 million of class_exists() misses.

            I am going to clean up the comments a bit and recommit it in reasonable chunks tomorrow, please shout if you object to submitting this to integration for the next Monday.

            Ciao

            Show
            skodak Petr Skoda added a comment - - edited I have added one more commit with class map cache that gets us to 2 seconds per 1 million of class_exists() misses. I am going to clean up the comments a bit and recommit it in reasonable chunks tomorrow, please shout if you object to submitting this to integration for the next Monday. Ciao
            Hide
            skodak Petr Skoda added a comment -

            Submitting for integration. Thanks a lot everybody.

            Show
            skodak Petr Skoda added a comment - Submitting for integration. Thanks a lot everybody.
            Hide
            salvetore Michael de Raadt added a comment -

            It will be a brave integrator who looks at this issue.

            I would like to point out that this issue is a dependency for other issues the team has in its sprint backlog. It would be good if this issue could be given priority in integration, especially if there is going to be a bit of back-and-forth.

            Show
            salvetore Michael de Raadt added a comment - It will be a brave integrator who looks at this issue. I would like to point out that this issue is a dependency for other issues the team has in its sprint backlog. It would be good if this issue could be given priority in integration, especially if there is going to be a bit of back-and-forth.
            Hide
            poltawski Dan Poltawski added a comment -

            I notice that this issue doesn't have a peer reviewer assigned, nor a peer review since the latest major change adding namespaces.

            Show
            poltawski Dan Poltawski added a comment - I notice that this issue doesn't have a peer reviewer assigned, nor a peer review since the latest major change adding namespaces.
            Hide
            poltawski Dan Poltawski added a comment -

            This issue doesn't have to hold up any further development, this is a major advantage of having a distributed version control system.

            So, i'd quite like to see that peer review and i'd also also personally like to see Eloy Lafuente (stronk7) and Martin Dougiamas to chime in, rather than pressure for us to integrate this 3 days after the code has been done.

            Show
            poltawski Dan Poltawski added a comment - This issue doesn't have to hold up any further development, this is a major advantage of having a distributed version control system. So, i'd quite like to see that peer review and i'd also also personally like to see Eloy Lafuente (stronk7) and Martin Dougiamas to chime in, rather than pressure for us to integrate this 3 days after the code has been done.
            Hide
            salvetore Michael de Raadt added a comment -

            I've asked Ankit to do a formal peer review.

            I've also added Eloy and Martin as watchers of this issue and, I'm sure, the team would welcome their feedback.

            Show
            salvetore Michael de Raadt added a comment - I've asked Ankit to do a formal peer review. I've also added Eloy and Martin as watchers of this issue and, I'm sure, the team would welcome their feedback.
            Hide
            dougiamas Martin Dougiamas added a comment -

            I don't have an opinion on this, really.

            • I'm happy if there's a consensus from the backend team on it.
            • It sounds like it simplifies and modernises coding process.
            • I was assured there's no backward compatibility problems.

            My only concern is that there will be yet another way to do things so documentation needs to be good (inline and on docs).

            My +1 for it.

            Show
            dougiamas Martin Dougiamas added a comment - I don't have an opinion on this, really. I'm happy if there's a consensus from the backend team on it. I'm happy there are no complaints on https://moodle.org/mod/forum/discuss.php?d=229474 . It sounds like it simplifies and modernises coding process. I was assured there's no backward compatibility problems. My only concern is that there will be yet another way to do things so documentation needs to be good (inline and on docs). My +1 for it.
            Hide
            skodak Petr Skoda added a comment -

            Basic documentation is already there, see http://docs.moodle.org/dev/Automatic_class_loading

            Show
            skodak Petr Skoda added a comment - Basic documentation is already there, see http://docs.moodle.org/dev/Automatic_class_loading
            Hide
            marina Marina Glancy added a comment -

            I thought it was just autoloading, but you refactored all plugin list retrieval as well...

            trying to do some pre-review:

            • @deprecated should be since 2.6 and not 2.5 (btw I thought we add functions to the end of deprecatedlib.php and not in the beginning)
            • get_plugin_types() is not always replaced with core_frankenstyle::get_plugin_types() (same for is_valid_plugin_name(), get_plugin_list()). Maybe at least inside core we should substitute them always?
            • get_plugin_list_with_function() and get_plugin_list_with_file() are not moved to core_frankenstyle, is it on purpose?
            • core_frankenstyle::is_valid_plugin_name() imho should have as the first argument the plugin name (to be similar to existing is_valid_plugin_name()) and the second argument boolean wether it's a module or not. Using 'tool' is quite confusing.
            • get_list_of_plugins() has a very strange check. Actually I can't understand this function at all.
            • why FRANKENSTYLE_CLASSLOADER is a constant and not $CFG value?
            • I think upgrade.txt should list all deprecated functions
            • I can't really review new caching approach

            This is a huge change, have you done any performance measuring tests?

            Show
            marina Marina Glancy added a comment - I thought it was just autoloading, but you refactored all plugin list retrieval as well... trying to do some pre-review: @deprecated should be since 2.6 and not 2.5 (btw I thought we add functions to the end of deprecatedlib.php and not in the beginning) get_plugin_types() is not always replaced with core_frankenstyle::get_plugin_types() (same for is_valid_plugin_name(), get_plugin_list()). Maybe at least inside core we should substitute them always? get_plugin_list_with_function() and get_plugin_list_with_file() are not moved to core_frankenstyle, is it on purpose? core_frankenstyle::is_valid_plugin_name() imho should have as the first argument the plugin name (to be similar to existing is_valid_plugin_name()) and the second argument boolean wether it's a module or not. Using 'tool' is quite confusing. get_list_of_plugins() has a very strange check. Actually I can't understand this function at all. why FRANKENSTYLE_CLASSLOADER is a constant and not $CFG value? I think upgrade.txt should list all deprecated functions I can't really review new caching approach This is a huge change, have you done any performance measuring tests?
            Hide
            skodak Petr Skoda added a comment -

            Thanks Marina:

            1. oops, fixing the deprecated version, there are no fixed rules for deprecatedlib.php, I prefer adding stuff that is kept forever at the top and stuff that will be removed soon to the end
            2. deprecated functions are not removed because I did not know when this will be integrated, we always fist create new api and migrate the rest in separate issue in order to prevent merging collisions
            3. get_plugin_list_with_function() and get_plugin_list_with_file() should be deprecated without a replacement, we should use classes instead
            4. core_frankenstyle::is_valid_plugin_name() - other methods take type first, it is mandatory parameter; is_valid_plugin_name() is useless for modules
            5. get_list_of_plugins() should not be used for plugins at all, it is for plugin-like directories only, that is why the check there
            6. FRANKENSTYLE_CLASSLOADER - we use constants for stuff like this, such as SESSION_CUSTOM_CLASS; this is not a setting - it alters the code
            7. upgrade.txt can be improved, again there are problems with collisions caused by other issues; this can be improved in new issue together with new docs

            Yes I did extensive performance testing, the install and uprages are faster, phpunit is faster, class_exists() does not access filesystem at all.

            Show
            skodak Petr Skoda added a comment - Thanks Marina: oops, fixing the deprecated version, there are no fixed rules for deprecatedlib.php, I prefer adding stuff that is kept forever at the top and stuff that will be removed soon to the end deprecated functions are not removed because I did not know when this will be integrated, we always fist create new api and migrate the rest in separate issue in order to prevent merging collisions get_plugin_list_with_function() and get_plugin_list_with_file() should be deprecated without a replacement, we should use classes instead core_frankenstyle::is_valid_plugin_name() - other methods take type first, it is mandatory parameter; is_valid_plugin_name() is useless for modules get_list_of_plugins() should not be used for plugins at all, it is for plugin-like directories only, that is why the check there FRANKENSTYLE_CLASSLOADER - we use constants for stuff like this, such as SESSION_CUSTOM_CLASS; this is not a setting - it alters the code upgrade.txt can be improved, again there are problems with collisions caused by other issues; this can be improved in new issue together with new docs Yes I did extensive performance testing, the install and uprages are faster, phpunit is faster, class_exists() does not access filesystem at all.
            Hide
            ankit_frenz Ankit Agarwal added a comment - - edited

            Hi Petr,
            I looked into this again. Patch looks polished and nice. A few things I came across,

            1. "MDL-38109 use always get_plugin() to get list of plugins" - Should be "get_plugins_list()" in the first commit msg
            2. get_plugins_list used to do

              if ($plugintype === '') {
                 $plugintype = 'mod';
              } 
              

              We might want to keep that for BC ? If we do that, it would be worth adding a few more asserts to test_deprecated_get_plugin_list() for the same.

            3. Not sure if integrators are going to be okay with simply renaming the class "tool_installaddon_installfromzip"

            Thanks

            Show
            ankit_frenz Ankit Agarwal added a comment - - edited Hi Petr, I looked into this again. Patch looks polished and nice. A few things I came across, " MDL-38109 use always get_plugin() to get list of plugins" - Should be "get_plugins_list()" in the first commit msg get_plugins_list used to do if ($plugintype === '') { $plugintype = 'mod'; } We might want to keep that for BC ? If we do that, it would be worth adding a few more asserts to test_deprecated_get_plugin_list() for the same. Not sure if integrators are going to be okay with simply renaming the class "tool_installaddon_installfromzip" Thanks
            Hide
            skodak Petr Skoda added a comment -

            1. fixed, thanks!
            2. I have added the '' $mod workaround, that is not hopefully used for a very long time. The new frankenstyle just returns empty list for any invalid params, the problem there is we can not use debugging() or exceptions there, it must be 100% self-contained.
            3. the class had incorrect name, there is no problem in renaming it, I already asked David to peer-review it yesterday and he did not see any problems there

            Show
            skodak Petr Skoda added a comment - 1. fixed, thanks! 2. I have added the '' $mod workaround, that is not hopefully used for a very long time. The new frankenstyle just returns empty list for any invalid params, the problem there is we can not use debugging() or exceptions there, it must be 100% self-contained. 3. the class had incorrect name, there is no problem in renaming it, I already asked David to peer-review it yesterday and he did not see any problems there
            Hide
            samhemelryk Sam Hemelryk added a comment -

            I've given this a good look over this morning.
            I like the code there are however a couple of things that I'd like thoughts on:

            1. core_frankenstyle - I really dislike that as a class name. I don't find the name really describes what this class does. Frankenstyle is a term we've coined to describe a plugin in the simpliest way possible. This class is about managing core subsystems and plugins, autoloading classes now and while internally it deals with frankenstyle I don't think that is reflected in the methods it provides.

            2. core_frankenstyle::get_cache_content - Could that not be building a $frankenstyle array and then calling var_export on just that? I don't imagine readability is a big thing there? Also should that including "defined('MOODLE_INTERNAL') || die();" before the variable definition? (not sure of the importance of that really just a thought)

            3. In two places you've included the root namespace def before a class e.g. "new \DirectoryIterator" would be nice to have that cleaned up (only two instances we have in core code excluding third party libs and I'm guessing it was just your editor being helpful)

            4. IMHO all uses of now deprecated functions need to be converted.

            Finally I got a failure in the unit tests - looks trivial but obviously we'd want that fixed.

            1) core_frankenstyle_testcase::test_get_plugin_list
            /var/www/integration/lib/tests/frankenstyle_test.php:155
            /var/www/integration/lib/phpunit/classes/advanced_testcase.php:76

            Other than that this is a really awesome change and I look forward to it landing.

            Many thanks Petr.

            Show
            samhemelryk Sam Hemelryk added a comment - I've given this a good look over this morning. I like the code there are however a couple of things that I'd like thoughts on: 1. core_frankenstyle - I really dislike that as a class name. I don't find the name really describes what this class does. Frankenstyle is a term we've coined to describe a plugin in the simpliest way possible. This class is about managing core subsystems and plugins, autoloading classes now and while internally it deals with frankenstyle I don't think that is reflected in the methods it provides. 2. core_frankenstyle::get_cache_content - Could that not be building a $frankenstyle array and then calling var_export on just that? I don't imagine readability is a big thing there? Also should that including "defined('MOODLE_INTERNAL') || die();" before the variable definition? (not sure of the importance of that really just a thought) 3. In two places you've included the root namespace def before a class e.g. "new \DirectoryIterator" would be nice to have that cleaned up (only two instances we have in core code excluding third party libs and I'm guessing it was just your editor being helpful) 4. IMHO all uses of now deprecated functions need to be converted. Finally I got a failure in the unit tests - looks trivial but obviously we'd want that fixed. 1) core_frankenstyle_testcase::test_get_plugin_list /var/www/integration/lib/tests/frankenstyle_test.php:155 /var/www/integration/lib/phpunit/classes/advanced_testcase.php:76 Other than that this is a really awesome change and I look forward to it landing. Many thanks Petr.
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Just one other note testing should mention testing with debugging on and off.

            Show
            samhemelryk Sam Hemelryk added a comment - Just one other note testing should mention testing with debugging on and off.
            Hide
            skodak Petr Skoda added a comment - - edited

            1. I did not find any better name, the rename would take 10 seconds...

            2. Yes, one export would be better/faster, I will fix that. The MOODLE_INTERNAL is not necessary because this script does not produce any errors if included directly, it can not be used because MOODLE_INTERNAL is defined after inclusion, we would have to define the MOODLE_INTERNAL before ABORT_AFTER_CONFIG which would be a change of API.

            3. The \DirectoryIterator were necessary when testing \core\frankenstyle::, it helped me to do make sure this class is completely self-contained.

            4. I do not have any failures there, maybe you have some weird dirs in some plugin directory.

            Thanks a lot.

            Show
            skodak Petr Skoda added a comment - - edited 1. I did not find any better name, the rename would take 10 seconds... 2. Yes, one export would be better/faster, I will fix that. The MOODLE_INTERNAL is not necessary because this script does not produce any errors if included directly, it can not be used because MOODLE_INTERNAL is defined after inclusion, we would have to define the MOODLE_INTERNAL before ABORT_AFTER_CONFIG which would be a change of API. 3. The \DirectoryIterator were necessary when testing \core\frankenstyle::, it helped me to do make sure this class is completely self-contained. 4. I do not have any failures there, maybe you have some weird dirs in some plugin directory. Thanks a lot.
            Hide
            poltawski Dan Poltawski added a comment -

            I just tested phpunit time and the time improvement is SIGNIFICANT! On my machine.

            Before:

            Time: 19:01, Memory: 278.50Mb
             
            OK, but incomplete or skipped tests!
            Tests: 1910, Assertions: 32119, Skipped: 5.

            After:

            Time: 11:11, Memory: 274.00Mb
             
            OK, but incomplete or skipped tests!
            Tests: 1924, Assertions: 34505, Skipped: 5.

            Show
            poltawski Dan Poltawski added a comment - I just tested phpunit time and the time improvement is SIGNIFICANT ! On my machine. Before: Time: 19:01, Memory: 278.50Mb   OK, but incomplete or skipped tests! Tests: 1910, Assertions: 32119, Skipped: 5. After: Time: 11:11, Memory: 274.00Mb   OK, but incomplete or skipped tests! Tests: 1924, Assertions: 34505, Skipped: 5.
            Hide
            skodak Petr Skoda added a comment -

            Rebased on top of current integration, the class in now called "core_components" (credit goes to Marina).

            Show
            skodak Petr Skoda added a comment - Rebased on top of current integration, the class in now called "core_components" (credit goes to Marina).
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment - - edited

            Hi, some comments, surely none affecting this to go forward:

            A) There are a bunch of (now) deprecated calls to get_plugin_types(), get_plugin_list(), normalize_component(), get_plugin_list_with_class()... a new issue should take rid of them in core. I think Sam commented about that above, but it became forgotten in your reply.

            B) We need also another issue to consider the 3 get_plugin_list_with_[class|file|function]() functions. And provide alternative for the later 2 ones.

            C) Tiny detail, or we use singulars or we use plurals. I don't like having "core_components" and "core_cache_definition" (for example), plz, homogenize. My +1 goes to singular (like we do in plugins and 99% of subsystems).

            D) I'm not sure I agree about to move away from MUC and, instead, create a particular implementation of caching for plugins types, lists and subsystems. Of course it leads to quicker unit tests executions, but at the cost of breaking the principle of reseting for each test. Sure it's something that does not change between tests, but the same can be applied to a lot of other caches and we are reseting them a thousand of times. Seems unbalanced (un-pretty) for me.

            E) Of course upgrade.txt requires MORE detailed information about what becomes deprecated, links to uses/docs... can be completed later, NP, but don't forget it.

            F) Introduction of namespaces. While I don't oppose to this, I think this requires way more detailed documentation about the uses we are expecting and the do-s and don't-s with them. Note this is apart from this issue itself, but if we officially start supporting them, because of their nice autoloading capabilities, they need to be rulez in the docs/codings style/code checker. We should not allow, for sure, things like:

            • bracketed nomenclature
            • multiple namespaces (or classes) per file
            • incorrect (non component) namespace prefixes. for example "\superduper".
            • incorrect (non subsystem) namespace suffixes. for example "\mod_forum\inventedbyme" or "\core\becauseiwant".

            Side note: Surely this is about to make the moodlecheck/codechecker checkers able to verify all these pieces of information together: @category, @package, namespace and path.

            G) In the (future) process of converting current /lib stuff to new autoloaded classes... one of the most common cases is when we have at least 2 classes related with a given element. Say, for example (not sure if it's valid or no, just the idea): core_textib and textlib_exception, i.e. one class and one exception for it. How is that going to be done, using the \core\textlib namespace and then.... ? Or putting the 2 files into lib/classes and naming theand naming them textlib.php and textlibexception.php ...? It's a simple example, but I think we are plenty of multi-classes examples.

            So I'd give this my +1, but please, clarify/don't forget about all the points listed above. This opens the gates to a great new way to do things, but it cannot be done without putting some fences in the field (aka, document, restrict, rule).

            My 2 cents, ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - - edited Hi, some comments, surely none affecting this to go forward: A) There are a bunch of (now) deprecated calls to get_plugin_types(), get_plugin_list(), normalize_component(), get_plugin_list_with_class()... a new issue should take rid of them in core. I think Sam commented about that above, but it became forgotten in your reply. B) We need also another issue to consider the 3 get_plugin_list_with_ [class|file|function] () functions. And provide alternative for the later 2 ones. C) Tiny detail, or we use singulars or we use plurals. I don't like having "core_components" and "core_cache_definition" (for example), plz, homogenize. My +1 goes to singular (like we do in plugins and 99% of subsystems). D) I'm not sure I agree about to move away from MUC and, instead, create a particular implementation of caching for plugins types, lists and subsystems. Of course it leads to quicker unit tests executions, but at the cost of breaking the principle of reseting for each test. Sure it's something that does not change between tests, but the same can be applied to a lot of other caches and we are reseting them a thousand of times. Seems unbalanced (un-pretty) for me. E) Of course upgrade.txt requires MORE detailed information about what becomes deprecated, links to uses/docs... can be completed later, NP, but don't forget it. F) Introduction of namespaces. While I don't oppose to this, I think this requires way more detailed documentation about the uses we are expecting and the do-s and don't-s with them. Note this is apart from this issue itself, but if we officially start supporting them, because of their nice autoloading capabilities, they need to be rulez in the docs/codings style/code checker. We should not allow, for sure, things like: bracketed nomenclature multiple namespaces (or classes) per file incorrect (non component) namespace prefixes. for example "\superduper". incorrect (non subsystem) namespace suffixes. for example "\mod_forum\inventedbyme" or "\core\becauseiwant". Side note: Surely this is about to make the moodlecheck/codechecker checkers able to verify all these pieces of information together: @category, @package, namespace and path. G) In the (future) process of converting current /lib stuff to new autoloaded classes... one of the most common cases is when we have at least 2 classes related with a given element. Say, for example (not sure if it's valid or no, just the idea): core_textib and textlib_exception, i.e. one class and one exception for it. How is that going to be done, using the \core\textlib namespace and then.... ? Or putting the 2 files into lib/classes and naming theand naming them textlib.php and textlibexception.php ...? It's a simple example, but I think we are plenty of multi-classes examples. So I'd give this my +1, but please, clarify/don't forget about all the points listed above. This opens the gates to a great new way to do things, but it cannot be done without putting some fences in the field (aka, document, restrict, rule). My 2 cents, ciao
            Hide
            skodak Petr Skoda added a comment -

            A) sure

            B) I think we do not need to change them because they should not be used, everything new should use classes

            C) Whatever you guy decide

            D) I am 100% sure MUC can not be used here - 1/ it MUST be self contained 2/ MUC should use it 3/ this must be as fast as possible 4/ it must work 100% in install/upgrade

            E) sure

            F) the major rule for namespaces is that the top most should be frankenstyle compoment

            G) current lib will not be changed much, we could just hardcode the class map for existing class names

            Show
            skodak Petr Skoda added a comment - A) sure B) I think we do not need to change them because they should not be used, everything new should use classes C) Whatever you guy decide D) I am 100% sure MUC can not be used here - 1/ it MUST be self contained 2/ MUC should use it 3/ this must be as fast as possible 4/ it must work 100% in install/upgrade E) sure F) the major rule for namespaces is that the top most should be frankenstyle compoment G) current lib will not be changed much, we could just hardcode the class map for existing class names
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            C) As said, I'd go for singulars for all packages, categories, namespaces... feel free to vote/decide.

            D) Well, my main reason for arguing about the MUC was not the fact of this using its own caching but the fact that you are not reseting it on each test and, in the other side, it has been required for all MUC caches to be reset always (when surely a bunch of them can be kept along then whole execution). That was the point I found "unbalanced". Or we reset all, or we decide to selectively reset and analyze each current cache.

            F) Talking about that... while I understood the difference... isn't "\mod_forum\event" and "\core\event" a bit unpaired? Shouldn't it be "\core_event" ? Aka, always component name. I imagine the "event" in forum comes from valid subsystem name and it's different from the component name required always as first element.

            G) Ah, you're planning to add them in a harcoded way? I thought they were going to be added in an automated way always.

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - C) As said, I'd go for singulars for all packages, categories, namespaces... feel free to vote/decide. D) Well, my main reason for arguing about the MUC was not the fact of this using its own caching but the fact that you are not reseting it on each test and, in the other side, it has been required for all MUC caches to be reset always (when surely a bunch of them can be kept along then whole execution). That was the point I found "unbalanced". Or we reset all, or we decide to selectively reset and analyze each current cache. F) Talking about that... while I understood the difference... isn't "\mod_forum\event" and "\core\event" a bit unpaired? Shouldn't it be "\core_event" ? Aka, always component name. I imagine the "event" in forum comes from valid subsystem name and it's different from the component name required always as first element. G) Ah, you're planning to add them in a harcoded way? I thought they were going to be added in an automated way always.
            Hide
            poltawski Dan Poltawski added a comment -

            I'm taking this, and hoping Eloys comments can be addressed

            Show
            poltawski Dan Poltawski added a comment - I'm taking this, and hoping Eloys comments can be addressed
            Hide
            skodak Petr Skoda added a comment -

            C) it was already voted to use core_components::, core_component:: would be ok for me too

            D) MUC cache invalidation is pretty lame, both performance and accuracy are critical here - there is no way MUC would fit here imo, sorry

            F) there is no core_event subsystem or directory, that leaves us with \core\event\ only - anyway this can be standardised later

            G) the current core classes do not use frankenstyle - there is no way to automate that in backwards compatible way apart from whitelisting some of them

            Show
            skodak Petr Skoda added a comment - C) it was already voted to use core_components::, core_component:: would be ok for me too D) MUC cache invalidation is pretty lame, both performance and accuracy are critical here - there is no way MUC would fit here imo, sorry F) there is no core_event subsystem or directory, that leaves us with \core\event\ only - anyway this can be standardised later G) the current core classes do not use frankenstyle - there is no way to automate that in backwards compatible way apart from whitelisting some of them
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            C) +1 for singular everywhere

            D) again, I'm not saying about to add MUC here. I'm saying that YOUR cache is not being reset and MUC caches are being always reset. And I find that "unbalanced" and surely we should consider / implement some way to avoid some of them to be reset always.

            F) Consider "cohort" instead of "event". Which ones would be the namespaces used for them?

            G) But... instead of whitelisting.... shouldn't be, for the textlib case again, be doing something like:

            • remove the current include in setup.php
            • split current file into 3-4-5 files (one for each class presently there)
            • use autoloader without any whitelisting (with or without namespaces)

            I think that's the future instead of whitelisting current libs. Aka, proper migration.

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - C) +1 for singular everywhere D) again, I'm not saying about to add MUC here. I'm saying that YOUR cache is not being reset and MUC caches are being always reset. And I find that "unbalanced" and surely we should consider / implement some way to avoid some of them to be reset always. F) Consider "cohort" instead of "event". Which ones would be the namespaces used for them? G) But... instead of whitelisting.... shouldn't be, for the textlib case again, be doing something like: remove the current include in setup.php split current file into 3-4-5 files (one for each class presently there) use autoloader without any whitelisting (with or without namespaces) I think that's the future instead of whitelisting current libs. Aka, proper migration. Ciao
            Hide
            skodak Petr Skoda added a comment - - edited

            C) you need a lot more +1's if you want me to change that

            D) my code is 100% correct, I will not use MUC there - create new issues for MUC if you want, it is not relevant to this issue

            F) this is not my problem here, namespaces are not used in this patch, it only contains general top level frankenstyle namespace which is 100% correct

            G) whitelisting is the only way forward for legacy core classes without "core_" prefix, I just added notes where to add whitelist for existing core classes


            This leaves us with only one question: "core_components::" or "core_component::" ?

            my +1 for core_components:: because it is not a subsystem, it is a "components" thing in "core".

            Show
            skodak Petr Skoda added a comment - - edited C) you need a lot more +1's if you want me to change that D) my code is 100% correct, I will not use MUC there - create new issues for MUC if you want, it is not relevant to this issue F) this is not my problem here, namespaces are not used in this patch, it only contains general top level frankenstyle namespace which is 100% correct G) whitelisting is the only way forward for legacy core classes without "core_" prefix, I just added notes where to add whitelist for existing core classes This leaves us with only one question: "core_components::" or "core_component::" ? my +1 for core_components:: because it is not a subsystem, it is a "components" thing in "core".
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment - - edited

            C) NP, I can wait for more votes to arrive. Nice, we now have, core subsystems, core apis and also core "things".

            D) Thanks for NOT reading what I'm saying.

            F) I'm not saying any action is needed, just stating that, if this opens the gates to a new thing (namespaces), we need to define their correct uses before getting them out of control. And yes, it's our problem.

            G) Agree whitelisting is for legacy classes. What I'm asking is which is the plan for any "thing" having multiple classes. I just used textlib (3-4 classes) as an example.

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - - edited C) NP, I can wait for more votes to arrive. Nice, we now have, core subsystems, core apis and also core "things". D) Thanks for NOT reading what I'm saying. F) I'm not saying any action is needed, just stating that, if this opens the gates to a new thing (namespaces), we need to define their correct uses before getting them out of control. And yes, it's our problem. G) Agree whitelisting is for legacy classes. What I'm asking is which is the plan for any "thing" having multiple classes. I just used textlib (3-4 classes) as an example.
            Hide
            poltawski Dan Poltawski added a comment -

            +1 for core_component.

            Show
            poltawski Dan Poltawski added a comment - +1 for core_component.
            Hide
            poltawski Dan Poltawski added a comment - - edited

            Hi Petr,

            1. We discussed in the integration team and would prefer the change to to the singular core_component.
            2. This is conflicting with integration/master, could you rebase against integration/masterif possible
            3. I added MDL-40220 for the deprecated calls and upgrade.txt

            thanks
            Dan

            Show
            poltawski Dan Poltawski added a comment - - edited Hi Petr, We discussed in the integration team and would prefer the change to to the singular core_component. This is conflicting with integration/master, could you rebase against integration/masterif possible I added MDL-40220 for the deprecated calls and upgrade.txt thanks Dan
            Hide
            dougiamas Martin Dougiamas added a comment -

            +1 for singular everywhere.

            Show
            dougiamas Martin Dougiamas added a comment - +1 for singular everywhere.
            Hide
            skodak Petr Skoda added a comment - - edited

            rebased, the new class is now called "core_component"

            edit:rebasing on top of integration now...

            Show
            skodak Petr Skoda added a comment - - edited rebased, the new class is now called "core_component" edit:rebasing on top of integration now...
            Hide
            skodak Petr Skoda added a comment -

            done

            Show
            skodak Petr Skoda added a comment - done
            Hide
            poltawski Dan Poltawski added a comment -

            Thanks

            Show
            poltawski Dan Poltawski added a comment - Thanks
            Hide
            poltawski Dan Poltawski added a comment - - edited

            Hi Petr,

            I've just done a test web install after the patch and this looks problematic:

            Invalid get_string() identifier: 'pluginname' or component 'auth_tests'. Perhaps you are missing $string['pluginname'] = ''; in /Users/danp/git/integration/auth/tests/lang/en/auth_tests.php?
            line 6925 of /lib/moodlelib.php: call to debugging()
            line 7617 of /lib/moodlelib.php: call to core_string_manager->get_string()
            line 57 of /user/editadvanced_form.php: call to get_string()
            line 191 of /lib/formslib.php: call to user_editadvanced_form->definition()
            line 150 of /user/editadvanced.php: call to moodleform->moodleform()
            

            EDIT: doesn't need a new install, also visible just by integrating the patch

            Show
            poltawski Dan Poltawski added a comment - - edited Hi Petr, I've just done a test web install after the patch and this looks problematic: Invalid get_string() identifier: 'pluginname' or component 'auth_tests'. Perhaps you are missing $string['pluginname'] = ''; in /Users/danp/git/integration/auth/tests/lang/en/auth_tests.php? line 6925 of /lib/moodlelib.php: call to debugging() line 7617 of /lib/moodlelib.php: call to core_string_manager->get_string() line 57 of /user/editadvanced_form.php: call to get_string() line 191 of /lib/formslib.php: call to user_editadvanced_form->definition() line 150 of /user/editadvanced.php: call to moodleform->moodleform() EDIT: doesn't need a new install, also visible just by integrating the patch
            Hide
            skodak Petr Skoda added a comment -

            fixed and big thanks!

            Show
            skodak Petr Skoda added a comment - fixed and big thanks!
            Hide
            poltawski Dan Poltawski added a comment -

            Thanks Petr, i've integrated this to master now.

            Show
            poltawski Dan Poltawski added a comment - Thanks Petr, i've integrated this to master now.
            Hide
            skodak Petr Skoda added a comment -

            yay, thanks everybody!!!

            Show
            skodak Petr Skoda added a comment - yay, thanks everybody!!!
            Hide
            poltawski Dan Poltawski added a comment -

            When moodle.org is back, it'd be good to post about the final solution here on the forums (we often forget that step )

            Show
            poltawski Dan Poltawski added a comment - When moodle.org is back, it'd be good to post about the final solution here on the forums (we often forget that step )
            Hide
            skodak Petr Skoda added a comment -
            Show
            skodak Petr Skoda added a comment - one more regression in installers: https://github.com/skodak/moodle/commits/fix_classloading5
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            great to see this landed, congrats. I can see a brilliant future once we start using this in a standard/documented/rule way!

            Offtopic: just detected a problem with get_all_subsystems_with_tests() returning 3 non-full paths and causing cli/util.php --buildcomponentconfigs to fail.

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - great to see this landed, congrats. I can see a brilliant future once we start using this in a standard/documented/rule way! Offtopic: just detected a problem with get_all_subsystems_with_tests() returning 3 non-full paths and causing cli/util.php --buildcomponentconfigs to fail.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Added 2 new commits from Petr fixing installer and test finder.

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Added 2 new commits from Petr fixing installer and test finder.
            Hide
            dmonllao David Monllaó added a comment -

            Hi,

            Not 100% sure if it's related with this, but is what it seems; I see a PHP warning when enabling the behat test environment:

            PHP Warning:  Illegal string offset 'mod' in /home/davidm/Desktop/moodlecode/INTEGRATION/master/lib/classes/component.php on line 105
            PHP Stack trace:
            PHP   1. {main}() /home/davidm/Desktop/moodlecode/INTEGRATION/master/admin/tool/behat/cli/util.php:0
            PHP   2. require() /home/davidm/Desktop/moodlecode/INTEGRATION/master/admin/tool/behat/cli/util.php:157
            PHP   3. initialise_cfg() /home/davidm/Desktop/moodlecode/INTEGRATION/master/lib/setup.php:576
            PHP   4. get_config() /home/davidm/Desktop/moodlecode/INTEGRATION/master/lib/setuplib.php:741
            PHP   5. cache::make() /home/davidm/Desktop/moodlecode/INTEGRATION/master/lib/moodlelib.php:1408
            PHP   6. cache_factory->create_cache_from_definition() /home/davidm/Desktop/moodlecode/INTEGRATION/master/cache/classes/loaders.php:169
            PHP   7. cache_factory->create_definition() /home/davidm/Desktop/moodlecode/INTEGRATION/master/cache/classes/factory.php:181
            PHP   8. cache_factory->create_config_instance() /home/davidm/Desktop/moodlecode/INTEGRATION/master/cache/classes/factory.php:379
            PHP   9. cache_config->load() /home/davidm/Desktop/moodlecode/INTEGRATION/master/cache/classes/factory.php:347
            PHP  10. class_exists() /home/davidm/Desktop/moodlecode/INTEGRATION/master/cache/classes/config.php:194
            PHP  11. core_component::classloader() /home/davidm/Desktop/moodlecode/INTEGRATION/master/cache/classes/config.php:0
            PHP  12. core_component::init() /home/davidm/Desktop/moodlecode/INTEGRATION/master/lib/classes/component.php:60

            Show
            dmonllao David Monllaó added a comment - Hi, Not 100% sure if it's related with this, but is what it seems; I see a PHP warning when enabling the behat test environment: PHP Warning: Illegal string offset 'mod' in /home/davidm/Desktop/moodlecode/INTEGRATION/master/lib/classes/component.php on line 105 PHP Stack trace: PHP 1. {main}() /home/davidm/Desktop/moodlecode/INTEGRATION/master/admin/tool/behat/cli/util.php:0 PHP 2. require() /home/davidm/Desktop/moodlecode/INTEGRATION/master/admin/tool/behat/cli/util.php:157 PHP 3. initialise_cfg() /home/davidm/Desktop/moodlecode/INTEGRATION/master/lib/setup.php:576 PHP 4. get_config() /home/davidm/Desktop/moodlecode/INTEGRATION/master/lib/setuplib.php:741 PHP 5. cache::make() /home/davidm/Desktop/moodlecode/INTEGRATION/master/lib/moodlelib.php:1408 PHP 6. cache_factory->create_cache_from_definition() /home/davidm/Desktop/moodlecode/INTEGRATION/master/cache/classes/loaders.php:169 PHP 7. cache_factory->create_definition() /home/davidm/Desktop/moodlecode/INTEGRATION/master/cache/classes/factory.php:181 PHP 8. cache_factory->create_config_instance() /home/davidm/Desktop/moodlecode/INTEGRATION/master/cache/classes/factory.php:379 PHP 9. cache_config->load() /home/davidm/Desktop/moodlecode/INTEGRATION/master/cache/classes/factory.php:347 PHP 10. class_exists() /home/davidm/Desktop/moodlecode/INTEGRATION/master/cache/classes/config.php:194 PHP 11. core_component::classloader() /home/davidm/Desktop/moodlecode/INTEGRATION/master/cache/classes/config.php:0 PHP 12. core_component::init() /home/davidm/Desktop/moodlecode/INTEGRATION/master/lib/classes/component.php:60
            Hide
            skodak Petr Skoda added a comment -

            one more fix for tests: https://github.com/skodak/moodle/commits/fix_classloading5
            thanks Fred for discovering this weirdness in filesystem

            Show
            skodak Petr Skoda added a comment - one more fix for tests: https://github.com/skodak/moodle/commits/fix_classloading5 thanks Fred for discovering this weirdness in filesystem
            Hide
            poltawski Dan Poltawski added a comment -
            Show
            poltawski Dan Poltawski added a comment - I think you mean https://github.com/skodak/moodle/commits/fix_classloading6 Which i've merged.
            Hide
            skodak Petr Skoda added a comment -

            oh, right!

            Show
            skodak Petr Skoda added a comment - oh, right!
            Hide
            poltawski Dan Poltawski added a comment -

            David: I tested behat init earlier and didn't ge that problem.

            Show
            poltawski Dan Poltawski added a comment - David: I tested behat init earlier and didn't ge that problem.
            Hide
            skodak Petr Skoda added a comment -

            one more commit in fix_classloading6 for behat utils (disabled dataroot caching)

            Show
            skodak Petr Skoda added a comment - one more commit in fix_classloading6 for behat utils (disabled dataroot caching)
            Hide
            poltawski Dan Poltawski added a comment -

            Petr has pushed a fix for the behat problem (which seemingly didn't hit me), i've pulled it.

            Show
            poltawski Dan Poltawski added a comment - Petr has pushed a fix for the behat problem (which seemingly didn't hit me), i've pulled it.
            Hide
            skodak Petr Skoda added a comment -

            The behat issue could be a race condition or stale data, my fix should eliminate both of these possibilities in the future.

            Show
            skodak Petr Skoda added a comment - The behat issue could be a race condition or stale data, my fix should eliminate both of these possibilities in the future.
            Hide
            dmonllao David Monllaó added a comment -

            Thanks Petr and Dan, no warnings with latest integration (reinstalling the test site)

            Show
            dmonllao David Monllaó added a comment - Thanks Petr and Dan, no warnings with latest integration (reinstalling the test site)
            Hide
            andyjdavis Andrew Davis added a comment -

            Hi. Im getting the following in master current integration. Is it related to this issue?

            Warning: Illegal string offset 'mod' in /home/andrew/Desktop/code/moodle/int/master/lib/classes/component.php on line 105
            Call Stack
            #	Time	Memory	Function	Location
            1	0.0035	364040	{main}( )	../index.php:0
            2	0.0038	373440	require( '/home/andrew/Desktop/code/moodle/int/master/config.php' )	../index.php:57
            3	0.0070	533704	require_once( '/home/andrew/Desktop/code/moodle/int/master/lib/setup.php' )	../config.php:25
            4	0.2236	13773216	initialise_cfg( )	../setup.php:576
            5	0.2236	13773304	get_config( )	../setuplib.php:741
            6	0.2240	13775648	cache::make( )	../moodlelib.php:1408
            7	0.2240	13776200	cache_factory->create_cache_from_definition( )	../loaders.php:169
            8	0.2240	13776432	cache_factory->create_definition( )	../factory.php:181
            9	0.2240	13776944	cache_factory->create_config_instance( )	../factory.php:379
            10	0.2241	13778144	cache_config->load( )	../factory.php:347
            11	0.2249	13827272	class_exists ( )	../config.php:194
            12	0.2249	13827736	core_component::classloader( )	../config.php:0
            13	0.2249	13827800	core_component::init( )	../component.php:60
             
            ( ! ) Warning: session_start(): Cannot send session cache limiter - headers already sent (output started at /home/andrew/Desktop/code/moodle/int/master/lib/classes/component.php:105) in /home/andrew/Desktop/code/moodle/int/master/lib/sessionlib.php on line 190
            Call Stack
            #	Time	Memory	Function	Location
            1	0.0035	364040	{main}( )	../index.php:0
            2	0.0038	373440	require( '/home/andrew/Desktop/code/moodle/int/master/config.php' )	../index.php:57
            3	0.0070	533704	require_once( '/home/andrew/Desktop/code/moodle/int/master/lib/setup.php' )	../config.php:25
            4	0.2770	14499856	session_get_instance( )	../setup.php:760
            5	0.2770	14500208	database_session->__construct( )	../sessionlib.php:66
            6	0.2770	14500400	session_stub->__construct( )	../sessionlib.php:482
            7	0.2772	14506528	session_start ( )	../sessionlib.php:190

            Show
            andyjdavis Andrew Davis added a comment - Hi. Im getting the following in master current integration. Is it related to this issue? Warning: Illegal string offset 'mod' in /home/andrew/Desktop/code/moodle/int/master/lib/classes/component.php on line 105 Call Stack # Time Memory Function Location 1 0.0035 364040 {main}( ) ../index.php:0 2 0.0038 373440 require( '/home/andrew/Desktop/code/moodle/int/master/config.php' ) ../index.php:57 3 0.0070 533704 require_once( '/home/andrew/Desktop/code/moodle/int/master/lib/setup.php' ) ../config.php:25 4 0.2236 13773216 initialise_cfg( ) ../setup.php:576 5 0.2236 13773304 get_config( ) ../setuplib.php:741 6 0.2240 13775648 cache::make( ) ../moodlelib.php:1408 7 0.2240 13776200 cache_factory->create_cache_from_definition( ) ../loaders.php:169 8 0.2240 13776432 cache_factory->create_definition( ) ../factory.php:181 9 0.2240 13776944 cache_factory->create_config_instance( ) ../factory.php:379 10 0.2241 13778144 cache_config->load( ) ../factory.php:347 11 0.2249 13827272 class_exists ( ) ../config.php:194 12 0.2249 13827736 core_component::classloader( ) ../config.php:0 13 0.2249 13827800 core_component::init( ) ../component.php:60   ( ! ) Warning: session_start(): Cannot send session cache limiter - headers already sent (output started at /home/andrew/Desktop/code/moodle/int/master/lib/classes/component.php:105) in /home/andrew/Desktop/code/moodle/int/master/lib/sessionlib.php on line 190 Call Stack # Time Memory Function Location 1 0.0035 364040 {main}( ) ../index.php:0 2 0.0038 373440 require( '/home/andrew/Desktop/code/moodle/int/master/config.php' ) ../index.php:57 3 0.0070 533704 require_once( '/home/andrew/Desktop/code/moodle/int/master/lib/setup.php' ) ../config.php:25 4 0.2770 14499856 session_get_instance( ) ../setup.php:760 5 0.2770 14500208 database_session->__construct( ) ../sessionlib.php:66 6 0.2770 14500400 session_stub->__construct( ) ../sessionlib.php:482 7 0.2772 14506528 session_start ( ) ../sessionlib.php:190
            Hide
            dmonllao David Monllaó added a comment -

            I still see the same warning when running the tests, BEHAT_UTIL is defined when running install/drop... and BEHAT_TEST is defined when the behat external CLI command is running (not when the tests are running using a browser) so if I'm not missing something BEHAT_TEST should also be included. Adding possible fix, but maybe is better to wait for Petr to confirm it.
            git pull MDL-39854_master-fix-behat_test

            PHP Warning:  Illegal string offset 'mod' in /home/davidm/Desktop/moodlecode/INTEGRATION/master/lib/classes/component.php on line 105
            PHP Stack trace:
            PHP   1. {main}() /home/davidm/Desktop/moodlecode/INTEGRATION/master/vendor/behat/behat/bin/behat:0
            PHP   2. Symfony\Component\Console\Application->run() /home/davidm/Desktop/moodlecode/INTEGRATION/master/vendor/behat/behat/bin/behat:32
            PHP   3. Behat\Behat\Console\BehatApplication->doRun() /home/davidm/Desktop/moodlecode/INTEGRATION/master/vendor/symfony/console/Symfony/Component/Console/Application.php:106
            PHP   4. Symfony\Component\Console\Application->doRun() /home/davidm/Desktop/moodlecode/INTEGRATION/master/vendor/behat/behat/src/Behat/Behat/Console/BehatApplication.php:68
            PHP   5. Symfony\Component\Console\Command\Command->run() /home/davidm/Desktop/moodlecode/INTEGRATION/master/vendor/symfony/console/Symfony/Component/Console/Application.php:193
            PHP   6. Behat\Behat\Console\Command\BehatCommand->execute() /home/davidm/Desktop/moodlecode/INTEGRATION/master/vendor/symfony/console/Symfony/Component/Console/Command/Command.php:242
            PHP   7. Behat\Behat\Console\Command\BehatCommand->beforeSuite() /home/davidm/Desktop/moodlecode/INTEGRATION/master/vendor/behat/behat/src/Behat/Behat/Console/Command/BehatCommand.php:127
            PHP   8. Symfony\Component\EventDispatcher\EventDispatcher->dispatch() /home/davidm/Desktop/moodlecode/INTEGRATION/master/vendor/behat/behat/src/Behat/Behat/Console/Command/BehatCommand.php:180
            PHP   9. Symfony\Component\EventDispatcher\EventDispatcher->doDispatch() /home/davidm/Desktop/moodlecode/INTEGRATION/master/vendor/symfony/event-dispatcher/Symfony/Component/EventDispatcher/EventDispatcher.php:53
            PHP  10. call_user_func() /home/davidm/Desktop/moodlecode/INTEGRATION/master/vendor/symfony/event-dispatcher/Symfony/Component/EventDispatcher/EventDispatcher.php:164
            PHP  11. Behat\Behat\Hook\HookDispatcher->beforeSuite() /home/davidm/Desktop/moodlecode/INTEGRATION/master/vendor/symfony/event-dispatcher/Symfony/Component/EventDispatcher/EventDispatcher.php:164
            PHP  12. Behat\Behat\Hook\HookDispatcher->fireHooks() /home/davidm/Desktop/moodlecode/INTEGRATION/master/vendor/behat/behat/src/Behat/Behat/Hook/HookDispatcher.php:114
            PHP  13. Behat\Behat\Hook\Annotation\Hook->run() /home/davidm/Desktop/moodlecode/INTEGRATION/master/vendor/behat/behat/src/Behat/Behat/Hook/HookDispatcher.php:276
            PHP  14. call_user_func() /home/davidm/Desktop/moodlecode/INTEGRATION/master/vendor/behat/behat/src/Behat/Behat/Hook/Annotation/Hook.php:50
            PHP  15. behat_hooks::before_suite() /home/davidm/Desktop/moodlecode/INTEGRATION/master/vendor/behat/behat/src/Behat/Behat/Hook/Annotation/Hook.php:50
            PHP  16. require_once() /home/davidm/Desktop/moodlecode/INTEGRATION/master/lib/tests/behat/behat_hooks.php:83
            PHP  17. require_once() /home/davidm/Desktop/moodlecode/INTEGRATION/master/config.php:89
            PHP  18. initialise_cfg() /home/davidm/Desktop/moodlecode/INTEGRATION/master/lib/setup.php:576
            PHP  19. get_config() /home/davidm/Desktop/moodlecode/INTEGRATION/master/lib/setuplib.php:741
            PHP  20. cache::make() /home/davidm/Desktop/moodlecode/INTEGRATION/master/lib/moodlelib.php:1408
            PHP  21. cache_factory->create_cache_from_definition() /home/davidm/Desktop/moodlecode/INTEGRATION/master/cache/classes/loaders.php:169
            PHP  22. cache_factory->create_definition() /home/davidm/Desktop/moodlecode/INTEGRATION/master/cache/classes/factory.php:181
            PHP  23. cache_factory->create_config_instance() /home/davidm/Desktop/moodlecode/INTEGRATION/master/cache/classes/factory.php:379
            PHP  24. cache_config->load() /home/davidm/Desktop/moodlecode/INTEGRATION/master/cache/classes/factory.php:347
            PHP  25. class_exists() /home/davidm/Desktop/moodlecode/INTEGRATION/master/cache/classes/config.php:194
            PHP  26. core_component::classloader() /home/davidm/Desktop/moodlecode/INTEGRATION/master/cache/classes/config.php:0
            PHP  27. core_component::init() /home/davidm/Desktop/moodlecode/INTEGRATION/master/lib/classes/component.php:60

            Show
            dmonllao David Monllaó added a comment - I still see the same warning when running the tests, BEHAT_UTIL is defined when running install/drop... and BEHAT_TEST is defined when the behat external CLI command is running (not when the tests are running using a browser) so if I'm not missing something BEHAT_TEST should also be included. Adding possible fix, but maybe is better to wait for Petr to confirm it. git pull MDL-39854 _master-fix-behat_test PHP Warning: Illegal string offset 'mod' in /home/davidm/Desktop/moodlecode/INTEGRATION/master/lib/classes/component.php on line 105 PHP Stack trace: PHP 1. {main}() /home/davidm/Desktop/moodlecode/INTEGRATION/master/vendor/behat/behat/bin/behat:0 PHP 2. Symfony\Component\Console\Application->run() /home/davidm/Desktop/moodlecode/INTEGRATION/master/vendor/behat/behat/bin/behat:32 PHP 3. Behat\Behat\Console\BehatApplication->doRun() /home/davidm/Desktop/moodlecode/INTEGRATION/master/vendor/symfony/console/Symfony/Component/Console/Application.php:106 PHP 4. Symfony\Component\Console\Application->doRun() /home/davidm/Desktop/moodlecode/INTEGRATION/master/vendor/behat/behat/src/Behat/Behat/Console/BehatApplication.php:68 PHP 5. Symfony\Component\Console\Command\Command->run() /home/davidm/Desktop/moodlecode/INTEGRATION/master/vendor/symfony/console/Symfony/Component/Console/Application.php:193 PHP 6. Behat\Behat\Console\Command\BehatCommand->execute() /home/davidm/Desktop/moodlecode/INTEGRATION/master/vendor/symfony/console/Symfony/Component/Console/Command/Command.php:242 PHP 7. Behat\Behat\Console\Command\BehatCommand->beforeSuite() /home/davidm/Desktop/moodlecode/INTEGRATION/master/vendor/behat/behat/src/Behat/Behat/Console/Command/BehatCommand.php:127 PHP 8. Symfony\Component\EventDispatcher\EventDispatcher->dispatch() /home/davidm/Desktop/moodlecode/INTEGRATION/master/vendor/behat/behat/src/Behat/Behat/Console/Command/BehatCommand.php:180 PHP 9. Symfony\Component\EventDispatcher\EventDispatcher->doDispatch() /home/davidm/Desktop/moodlecode/INTEGRATION/master/vendor/symfony/event-dispatcher/Symfony/Component/EventDispatcher/EventDispatcher.php:53 PHP 10. call_user_func() /home/davidm/Desktop/moodlecode/INTEGRATION/master/vendor/symfony/event-dispatcher/Symfony/Component/EventDispatcher/EventDispatcher.php:164 PHP 11. Behat\Behat\Hook\HookDispatcher->beforeSuite() /home/davidm/Desktop/moodlecode/INTEGRATION/master/vendor/symfony/event-dispatcher/Symfony/Component/EventDispatcher/EventDispatcher.php:164 PHP 12. Behat\Behat\Hook\HookDispatcher->fireHooks() /home/davidm/Desktop/moodlecode/INTEGRATION/master/vendor/behat/behat/src/Behat/Behat/Hook/HookDispatcher.php:114 PHP 13. Behat\Behat\Hook\Annotation\Hook->run() /home/davidm/Desktop/moodlecode/INTEGRATION/master/vendor/behat/behat/src/Behat/Behat/Hook/HookDispatcher.php:276 PHP 14. call_user_func() /home/davidm/Desktop/moodlecode/INTEGRATION/master/vendor/behat/behat/src/Behat/Behat/Hook/Annotation/Hook.php:50 PHP 15. behat_hooks::before_suite() /home/davidm/Desktop/moodlecode/INTEGRATION/master/vendor/behat/behat/src/Behat/Behat/Hook/Annotation/Hook.php:50 PHP 16. require_once() /home/davidm/Desktop/moodlecode/INTEGRATION/master/lib/tests/behat/behat_hooks.php:83 PHP 17. require_once() /home/davidm/Desktop/moodlecode/INTEGRATION/master/config.php:89 PHP 18. initialise_cfg() /home/davidm/Desktop/moodlecode/INTEGRATION/master/lib/setup.php:576 PHP 19. get_config() /home/davidm/Desktop/moodlecode/INTEGRATION/master/lib/setuplib.php:741 PHP 20. cache::make() /home/davidm/Desktop/moodlecode/INTEGRATION/master/lib/moodlelib.php:1408 PHP 21. cache_factory->create_cache_from_definition() /home/davidm/Desktop/moodlecode/INTEGRATION/master/cache/classes/loaders.php:169 PHP 22. cache_factory->create_definition() /home/davidm/Desktop/moodlecode/INTEGRATION/master/cache/classes/factory.php:181 PHP 23. cache_factory->create_config_instance() /home/davidm/Desktop/moodlecode/INTEGRATION/master/cache/classes/factory.php:379 PHP 24. cache_config->load() /home/davidm/Desktop/moodlecode/INTEGRATION/master/cache/classes/factory.php:347 PHP 25. class_exists() /home/davidm/Desktop/moodlecode/INTEGRATION/master/cache/classes/config.php:194 PHP 26. core_component::classloader() /home/davidm/Desktop/moodlecode/INTEGRATION/master/cache/classes/config.php:0 PHP 27. core_component::init() /home/davidm/Desktop/moodlecode/INTEGRATION/master/lib/classes/component.php:60
            Hide
            skodak Petr Skoda added a comment -

            one more commit to disable the behat completely: https://github.com/skodak/moodle/commits/fix_classloading7
            Btw the short defined('BEHAT_*') or not 100% correct, it should also verify the value.

            Show
            skodak Petr Skoda added a comment - one more commit to disable the behat completely: https://github.com/skodak/moodle/commits/fix_classloading7 Btw the short defined('BEHAT_*') or not 100% correct, it should also verify the value.
            Hide
            poltawski Dan Poltawski added a comment -

            I've pulled in davids fix (both were the same and he was first).

            Is this the same as Andrews problem? Can anyone reproduce that?

            Show
            poltawski Dan Poltawski added a comment - I've pulled in davids fix (both were the same and he was first). Is this the same as Andrews problem? Can anyone reproduce that?
            Hide
            rwijaya Rossiani Wijaya added a comment -

            Hi Dan,

            I updated my master branch and the error still exist.

            Warning: Illegal string offset 'mod' in /integration/master/lib/classes/component.php on line 107

            Show
            rwijaya Rossiani Wijaya added a comment - Hi Dan, I updated my master branch and the error still exist. Warning: Illegal string offset 'mod' in /integration/master/lib/classes/component.php on line 107
            Hide
            skodak Petr Skoda added a comment -

            The undefined 'mod' means that the on-disk component cache is somehow severely broken. It is automatically rebuilt if you visit http://yoursite/admin

            Behat is doing tricky modification of $CFG->dataroot which explains it, but I have no idea how that could happen in normal operation.

            Show
            skodak Petr Skoda added a comment - The undefined 'mod' means that the on-disk component cache is somehow severely broken. It is automatically rebuilt if you visit http://yoursite/admin Behat is doing tricky modification of $CFG->dataroot which explains it, but I have no idea how that could happen in normal operation.
            Hide
            skodak Petr Skoda added a comment -

            Could somebody with the 'mod' trouble (that did not run Behat) attach here the dataroot/cache/core_component.php file?

            Show
            skodak Petr Skoda added a comment - Could somebody with the 'mod' trouble (that did not run Behat) attach here the dataroot/cache/core_component.php file?
            Hide
            markn Mark Nelson added a comment -

            Hi Petr, works as expected for me. I did not receive any of the mentioned errors following those testing instructions, though I am surprised they were so short considering the size of this change.

            Show
            markn Mark Nelson added a comment - Hi Petr, works as expected for me. I did not receive any of the mentioned errors following those testing instructions, though I am surprised they were so short considering the size of this change.
            Hide
            markn Mark Nelson added a comment -

            (I am perfectly happy with the testing instructions being short)

            Show
            markn Mark Nelson added a comment - (I am perfectly happy with the testing instructions being short)
            Hide
            rwijaya Rossiani Wijaya added a comment -

            Hi Petr,

            I just attached my cache/core_component.php file.

            Thanks

            Show
            rwijaya Rossiani Wijaya added a comment - Hi Petr, I just attached my cache/core_component.php file. Thanks
            Hide
            skodak Petr Skoda added a comment -

            ARRRGGGHHH!!!!!! I found it, I did wrong var_export there!

            Show
            skodak Petr Skoda added a comment - ARRRGGGHHH!!!!!! I found it, I did wrong var_export there!
            Hide
            andyjdavis Andrew Davis added a comment -

            See attached.

            Show
            andyjdavis Andrew Davis added a comment - See attached.
            Hide
            skodak Petr Skoda added a comment - - edited

            This should be finally fixed in https://github.com/skodak/moodle/commits/fix_classloading8,

            pelase test with developer debug and without debug (set in config.php).

            Show
            skodak Petr Skoda added a comment - - edited This should be finally fixed in https://github.com/skodak/moodle/commits/fix_classloading8 , pelase test with developer debug and without debug (set in config.php).
            Hide
            poltawski Dan Poltawski added a comment -

            I've integrated the fix.

            Mark, since you were so happy about the short testing instructions, here is another round of testing for you in DEBUG_DEVELOPER and not.

            thanks!

            Show
            poltawski Dan Poltawski added a comment - I've integrated the fix. Mark, since you were so happy about the short testing instructions, here is another round of testing for you in DEBUG_DEVELOPER and not. thanks!
            Hide
            poltawski Dan Poltawski added a comment -

            And Petr, how you've disappointed me

            D) my code is 100% correct

            Show
            poltawski Dan Poltawski added a comment - And Petr, how you've disappointed me D) my code is 100% correct
            Hide
            rwijaya Rossiani Wijaya added a comment -

            Thanks Petr and Dan.

            The last patch fixed the offset error.

            Show
            rwijaya Rossiani Wijaya added a comment - Thanks Petr and Dan. The last patch fixed the offset error.
            Hide
            skodak Petr Skoda added a comment -

            well, my code was correct before I listened to coding style improvements hints from peer reviews,
            next time I will try to retest more after tweaking or I'll just ignore some coding style objections...

            anyway thansk!!!

            Show
            skodak Petr Skoda added a comment - well, my code was correct before I listened to coding style improvements hints from peer reviews, next time I will try to retest more after tweaking or I'll just ignore some coding style objections... anyway thansk!!!
            Hide
            markn Mark Nelson added a comment -

            It worked liked before with both debugging enabled and disabled. I should also mention I found one unrelated issue which I have linked.

            Show
            markn Mark Nelson added a comment - It worked liked before with both debugging enabled and disabled. I should also mention I found one unrelated issue which I have linked.
            Hide
            poltawski Dan Poltawski added a comment -

            Thanks for your contributions!

            _main:
            @ BB#0:
                    push    {r7, lr}
                    mov     r7, sp
                    sub     sp, #4
                    movw    r0, :lower16:(L_.str-(LPC0_0+4))
                    movt    r0, :upper16:(L_.str-(LPC0_0+4))
            LPC0_0:
                    add     r0, pc
                    bl      _printf
                    movs    r1, #0
                    movt    r1, #0
                    str     r0, [sp]                @ 4-byte Spill
                    mov     r0, r1
                    add     sp, #4
                    pop     {r7, pc}
             
                    .section        __TEXT,__cstring,cstring_literals
            L_.str:                                 @ @.str
                    .asciz   "This code is now upstream!"
            

            Show
            poltawski Dan Poltawski added a comment - Thanks for your contributions! _main: @ BB#0: push {r7, lr} mov r7, sp sub sp, #4 movw r0, :lower16:(L_.str-(LPC0_0+4)) movt r0, :upper16:(L_.str-(LPC0_0+4)) LPC0_0: add r0, pc bl _printf movs r1, #0 movt r1, #0 str r0, [sp] @ 4-byte Spill mov r0, r1 add sp, #4 pop {r7, pc}   .section __TEXT,__cstring,cstring_literals L_.str: @ @.str .asciz "This code is now upstream!"
            Hide
            ashleyholman Ashley Holman added a comment -

            In regards to this comment:

            // Note: cachedir MUST be shared by all servers in a cluster, sorry guys...

            NetSpot are going to have to modify this code because we don't share cachedir between nodes.

            Our initial thoughts are to generate the class list file each time we build a new revision, and then roll that out to each node. So rather than having Moodle dynamically regenerate the file, it's just static and we have to include an updated version with each new build. Does this sound like a workable approach or is there some need to have Moodle modify the file at run time?

            Cheers
            Ash

            Show
            ashleyholman Ashley Holman added a comment - In regards to this comment: // Note: cachedir MUST be shared by all servers in a cluster, sorry guys... NetSpot are going to have to modify this code because we don't share cachedir between nodes. Our initial thoughts are to generate the class list file each time we build a new revision, and then roll that out to each node. So rather than having Moodle dynamically regenerate the file, it's just static and we have to include an updated version with each new build. Does this sound like a workable approach or is there some need to have Moodle modify the file at run time? Cheers Ash
            Hide
            dougiamas Martin Dougiamas added a comment -

            I would really like it for people using clusters not to have to modify ANYTHING. Cluster support must be #1 on our minds here with all new core additions by BACKEND.

            I understand that we might SOMETIMES need a shared cachedir, but what I don't understand is why automatic class loading needs to use it, and why the caching it is doing can't be done separately on each node.

            Petr, can you please explain exactly what your reasoning is about this?

            Show
            dougiamas Martin Dougiamas added a comment - I would really like it for people using clusters not to have to modify ANYTHING. Cluster support must be #1 on our minds here with all new core additions by BACKEND. I understand that we might SOMETIMES need a shared cachedir, but what I don't understand is why automatic class loading needs to use it, and why the caching it is doing can't be done separately on each node. Petr, can you please explain exactly what your reasoning is about this?
            Hide
            matteo Matteo Scaramuccia added a comment -
            Show
            matteo Matteo Scaramuccia added a comment - @ Ashley Holman : FYI MDL-40034 .
            Hide
            skodak Petr Skoda added a comment -

            Really, cachedir was always required to be shared, it is not going to change. If you alter that moodle code is going to break, in this particular case expect problems when modifying PHP files === during upgrades or addon installation.

            Show
            skodak Petr Skoda added a comment - Really, cachedir was always required to be shared, it is not going to change. If you alter that moodle code is going to break, in this particular case expect problems when modifying PHP files === during upgrades or addon installation.
            Hide
            skodak Petr Skoda added a comment -

            I have created a new issue MDL-40475 for new setting that lets you store component cache outside of cachedir with manual cache invalidation.

            Show
            skodak Petr Skoda added a comment - I have created a new issue MDL-40475 for new setting that lets you store component cache outside of cachedir with manual cache invalidation.
            Hide
            poltawski Dan Poltawski added a comment -

            Hi Petr,

            I discovered a new problem with this breaking $CFG->admin in MDL-40488.

            Show
            poltawski Dan Poltawski added a comment - Hi Petr, I discovered a new problem with this breaking $CFG->admin in MDL-40488 .

              People

              • Votes:
                3 Vote for this issue
                Watchers:
                19 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  18/Nov/13

                  Agile