Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.5
    • Fix Version/s: 2.6
    • Component/s: Libraries
    • Story Points:
      40
    • Rank:
      50594
    • 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

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

        Issue Links

          Activity

          Hide
          Petr Škoda added a comment -
          Show
          Petr Škoda added a comment - Example code demonstrating the idea is here: https://github.com/skodak/moodle/compare/master...wip_MDL-39854_m26_classloading
          Hide
          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
          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
          Petr Škoda 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
          Petr Škoda 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
          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
          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
          Petr Škoda 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
          Petr Škoda 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
          Petr Škoda 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
          Petr Škoda 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
          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
          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 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 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
          Petr Škoda 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
          Petr Škoda 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
          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
          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
          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
          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
          Petr Škoda added a comment -

          Thanks a lot, I was blind!

          Show
          Petr Škoda added a comment - Thanks a lot, I was blind!
          Hide
          Petr Škoda 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
          Petr Škoda 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
          Petr Škoda 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
          Petr Škoda 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
          Petr Škoda added a comment -

          Submitting for integration. Thanks a lot everybody.

          Show
          Petr Škoda added a comment - Submitting for integration. Thanks a lot everybody.
          Hide
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          Petr Škoda added a comment -

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

          Show
          Petr Škoda added a comment - Basic documentation is already there, see http://docs.moodle.org/dev/Automatic_class_loading
          Hide
          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 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
          Petr Škoda 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
          Petr Škoda 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 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 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
          Petr Škoda 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
          Petr Škoda 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
          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
          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
          Sam Hemelryk added a comment -

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

          Show
          Sam Hemelryk added a comment - Just one other note testing should mention testing with debugging on and off.
          Hide
          Petr Škoda 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
          Petr Škoda 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
          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
          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
          Petr Škoda added a comment -

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

          Show
          Petr Škoda added a comment - Rebased on top of current integration, the class in now called "core_components" (credit goes to Marina).
          Hide
          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
          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
          Petr Škoda 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
          Petr Škoda 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
          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
          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
          Dan Poltawski added a comment -

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

          Show
          Dan Poltawski added a comment - I'm taking this, and hoping Eloys comments can be addressed
          Hide
          Petr Škoda 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
          Petr Škoda 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
          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
          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
          Petr Škoda 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
          Petr Škoda 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
          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
          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
          Dan Poltawski added a comment -

          +1 for core_component.

          Show
          Dan Poltawski added a comment - +1 for core_component.
          Hide
          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
          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
          Martin Dougiamas added a comment -

          +1 for singular everywhere.

          Show
          Martin Dougiamas added a comment - +1 for singular everywhere.
          Hide
          Petr Škoda added a comment - - edited

          rebased, the new class is now called "core_component"

          edit:rebasing on top of integration now...

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

          done

          Show
          Petr Škoda added a comment - done
          Hide
          Dan Poltawski added a comment -

          Thanks

          Show
          Dan Poltawski added a comment - Thanks
          Hide
          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
          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
          Petr Škoda added a comment -

          fixed and big thanks!

          Show
          Petr Škoda added a comment - fixed and big thanks!
          Hide
          Dan Poltawski added a comment -

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

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

          yay, thanks everybody!!!

          Show
          Petr Škoda added a comment - yay, thanks everybody!!!
          Hide
          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
          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
          Petr Škoda added a comment -
          Show
          Petr Škoda added a comment - one more regression in installers: https://github.com/skodak/moodle/commits/fix_classloading5
          Hide
          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
          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
          Eloy Lafuente (stronk7) added a comment -

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

          Show
          Eloy Lafuente (stronk7) added a comment - Added 2 new commits from Petr fixing installer and test finder.
          Hide
          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
          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
          Petr Škoda 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
          Petr Škoda 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
          Dan Poltawski added a comment -
          Show
          Dan Poltawski added a comment - I think you mean https://github.com/skodak/moodle/commits/fix_classloading6 Which i've merged.
          Hide
          Petr Škoda added a comment -

          oh, right!

          Show
          Petr Škoda added a comment - oh, right!
          Hide
          Dan Poltawski added a comment -

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

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

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

          Show
          Petr Škoda added a comment - one more commit in fix_classloading6 for behat utils (disabled dataroot caching)
          Hide
          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
          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
          Petr Škoda 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
          Petr Škoda 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
          David Monllaó added a comment -

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

          Show
          David Monllaó added a comment - Thanks Petr and Dan, no warnings with latest integration (reinstalling the test site)
          Hide
          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
          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
          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
          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
          Petr Škoda 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
          Petr Škoda 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
          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
          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
          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
          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
          Petr Škoda 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
          Petr Škoda 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
          Petr Škoda added a comment -

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

          Show
          Petr Škoda added a comment - Could somebody with the 'mod' trouble (that did not run Behat) attach here the dataroot/cache/core_component.php file?
          Hide
          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
          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
          Mark Nelson added a comment -

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

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

          Hi Petr,

          I just attached my cache/core_component.php file.

          Thanks

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

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

          Show
          Petr Škoda added a comment - ARRRGGGHHH!!!!!! I found it, I did wrong var_export there!
          Hide
          Andrew Davis added a comment -

          See attached.

          Show
          Andrew Davis added a comment - See attached.
          Hide
          Petr Škoda 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
          Petr Škoda 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
          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
          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
          Dan Poltawski added a comment -

          And Petr, how you've disappointed me

          D) my code is 100% correct

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

          Thanks Petr and Dan.

          The last patch fixed the offset error.

          Show
          Rossiani Wijaya added a comment - Thanks Petr and Dan. The last patch fixed the offset error.
          Hide
          Petr Škoda 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
          Petr Škoda 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
          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
          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
          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
          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
          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
          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
          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
          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 Scaramuccia added a comment -
          Show
          Matteo Scaramuccia added a comment - @ Ashley Holman : FYI MDL-40034 .
          Hide
          Petr Škoda 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
          Petr Škoda 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
          Petr Škoda 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
          Petr Škoda 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
          Dan Poltawski added a comment -

          Hi Petr,

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

          Show
          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:

                Agile