Moodle
  1. Moodle
  2. MDL-37598

Drop ABORT_AFTER_CONFIG from theme/yui_combo.php

    Details

    • Testing Instructions:
      Hide

      Testing difficulty: Trivial

      Standard regression test: Purge caches and make sure that Javascript works here and there. More technically (if using some browser extension that monitors the request traffic), make sure that loading theme/yui_combo.php and theme/yui_image.php still works.

      Show
      Testing difficulty: Trivial Standard regression test: Purge caches and make sure that Javascript works here and there. More technically (if using some browser extension that monitors the request traffic), make sure that loading theme/yui_combo.php and theme/yui_image.php still works.
    • Affected Branches:
      MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-37598-abort-after-config
    • Rank:
      47285

      Description

      While working on adding MUC support into plugin related core functions, I am facing yet another issue. The script theme/yui_combo.php defines ABORT_AFTER_CONFIG and thence the MUC libraries are not loaded during the setup. But then the script calls get_component_directory() which in turn ends by calling plugin related methods with the MUC support.

      PHP Fatal error:  Class 'cache' not found
      

      and JS not loaded.

      https://github.com/mudrd8mz/moodle/compare/master...MDL-34401-muc-pluginlib shows my work in progress on MDL-34401

        Issue Links

          Activity

          Hide
          Petr Škoda added a comment - - edited

          I am sorry, but this is definitely not a bug in ABORT_AFTER_CONFIG. ABORT_AFTER_CONFIG was designed to allow access to config settings without loading any libraries. It is intended for integration with 3rd party stuff that needs access to basic $CFG settings such as phpmyadmin, one of the goals was to prevent function and class name collisions. Alternatively it can be used in simple scripts such as theme resource serving files (please note they are supposed to include only lib/configonlylib.php) - these files do it for performance reasons only.

          I suppose the obvious solution is to do normal include in yui loader - the database query should be eliminated by MUC, that was the main reason why the ABORT_AFTER_CONFIG was used there after all. Caching should be now also a lot more reliable, the extra memory necessary for includes should not be a big issue there.

          Show
          Petr Škoda added a comment - - edited I am sorry, but this is definitely not a bug in ABORT_AFTER_CONFIG. ABORT_AFTER_CONFIG was designed to allow access to config settings without loading any libraries. It is intended for integration with 3rd party stuff that needs access to basic $CFG settings such as phpmyadmin, one of the goals was to prevent function and class name collisions. Alternatively it can be used in simple scripts such as theme resource serving files (please note they are supposed to include only lib/configonlylib.php) - these files do it for performance reasons only. I suppose the obvious solution is to do normal include in yui loader - the database query should be eliminated by MUC, that was the main reason why the ABORT_AFTER_CONFIG was used there after all. Caching should be now also a lot more reliable, the extra memory necessary for includes should not be a big issue there.
          Hide
          Petr Škoda added a comment -

          Alternatively we could create a new constant that does minimal init without database and with minimum of libraries for things like theme/yui serving scripts.

          Show
          Petr Škoda added a comment - Alternatively we could create a new constant that does minimal init without database and with minimum of libraries for things like theme/yui serving scripts.
          Hide
          David Mudrak added a comment -

          Sure, I did not mean this was a bug as the title might suggest. I just reported this to discuss a good solution for MUC support in yui_combo.php. I will change the title of the bug to make it clearer, or maybe rather open a new issue: "Drop ABORT_AFTER_CONFIG from theme/yui_combo.php".

          Show
          David Mudrak added a comment - Sure, I did not mean this was a bug as the title might suggest. I just reported this to discuss a good solution for MUC support in yui_combo.php. I will change the title of the bug to make it clearer, or maybe rather open a new issue: "Drop ABORT_AFTER_CONFIG from theme/yui_combo.php".
          Hide
          Petr Škoda added a comment -

          AH, great, we discussed this as a potential problem in yui loader when the component handling was introduced. Thanks anyway for working on this.

          Show
          Petr Škoda added a comment - AH, great, we discussed this as a potential problem in yui loader when the component handling was introduced. Thanks anyway for working on this.
          Hide
          David Mudrak added a comment -

          Renaming the issue from "MUC cache framework not loaded when ABORT_AFTER_CONFIG is defined" to "Drop ABORT_AFTER_CONFIG from theme/yui_combo.php"

          Show
          David Mudrak added a comment - Renaming the issue from "MUC cache framework not loaded when ABORT_AFTER_CONFIG is defined" to "Drop ABORT_AFTER_CONFIG from theme/yui_combo.php"
          Hide
          Sam Hemelryk added a comment -

          Woot glad the two of you have sorted out the direction for the solution.

          Worth mentioning is that we'd also need to check yui_image, surely that is affected as well.

          Show
          Sam Hemelryk added a comment - Woot glad the two of you have sorted out the direction for the solution. Worth mentioning is that we'd also need to check yui_image, surely that is affected as well.
          Hide
          David Mudrak added a comment -

          Petr, can you please peer-review this patch? Thanks in advance.

          Show
          David Mudrak added a comment - Petr, can you please peer-review this patch? Thanks in advance.
          Hide
          Petr Škoda added a comment -

          +1

          Show
          Petr Škoda added a comment - +1
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Pasted fro, chat, to avoid forgetting it. Feel free to answer:

          [10:35:28] <stronk7> David, why do we need access to the MUC to serve yui files? Because you're caching components/plugins ?

          [10:38:56] <stronk7> My point is if we couldn't, instead of requiring the whole setup.php, just get the caches thing inited and available, ie. continue doing the minimum needed.

          [10:41:34] <stronk7> More yet, the only reason we need to include anything.... is to get access to get_component_directory(), isn't it? I really think that we could duplicate some code / move it somewhere else and avoid including anything totally.

          [10:42:44] <stronk7> Also, another possible solution is to avoid using the caches in get_component_directory() if ABORT_AFTER_CONFIG == true.

          [10:43:12] <stronk7> For your consideration, note I'm not an expert, but the switch to execute the whole moodle setup sounds like too much here.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Pasted fro, chat, to avoid forgetting it. Feel free to answer: [10:35:28] <stronk7> David, why do we need access to the MUC to serve yui files? Because you're caching components/plugins ? [10:38:56] <stronk7> My point is if we couldn't, instead of requiring the whole setup.php, just get the caches thing inited and available, ie. continue doing the minimum needed. [10:41:34] <stronk7> More yet, the only reason we need to include anything.... is to get access to get_component_directory(), isn't it? I really think that we could duplicate some code / move it somewhere else and avoid including anything totally. [10:42:44] <stronk7> Also, another possible solution is to avoid using the caches in get_component_directory() if ABORT_AFTER_CONFIG == true. [10:43:12] <stronk7> For your consideration, note I'm not an expert, but the switch to execute the whole moodle setup sounds like too much here. Ciao
          Hide
          David Mudrak added a comment -

          Hi Eloy. Thanks for raising this. We discussed it with Petr during the peer-review, too.

          Yes, full setup is required to be able to start using MUC in get_plugin_types() and get_plugin_list() core functions - it is my current work in progress at MDL-34401. As Petr mentioned above, this was a known issue in these two YUI serving scripts. I believe that full setup is cleaner solution than hacking core and injecting exceptions there. As we are armoured with MUC now, we should try and make full setup performing better anyway.

          Show
          David Mudrak added a comment - Hi Eloy. Thanks for raising this. We discussed it with Petr during the peer-review, too. Yes, full setup is required to be able to start using MUC in get_plugin_types() and get_plugin_list() core functions - it is my current work in progress at MDL-34401 . As Petr mentioned above, this was a known issue in these two YUI serving scripts. I believe that full setup is cleaner solution than hacking core and injecting exceptions there. As we are armoured with MUC now, we should try and make full setup performing better anyway.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated (master), thanks!

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

          Thanks for fixing this David,

          All seems to work fine.

          Show
          Rajesh Taneja added a comment - Thanks for fixing this David, All seems to work fine.
          Hide
          Damyon Wiese added a comment -

          Congratulations this fix has been added to Moodle!

          You may want to dedicate this issue to someone special on this Valentines day.

          Thanks!

          Show
          Damyon Wiese added a comment - Congratulations this fix has been added to Moodle! You may want to dedicate this issue to someone special on this Valentines day. Thanks!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: