Moodle
  1. Moodle
  2. MDL-34740

page class supports dynamic class loading but not file inclusions

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3.1
    • Fix Version/s: 2.4
    • Component/s: Libraries
    • Labels:

      Description

      One can specify an alternate moodlepageclass but cannot load the class script without core or config customizations.

        Gliffy Diagrams

          Activity

          Hide
          Michael de Raadt added a comment -

          Thanks for spotting that and providing a patch.

          Show
          Michael de Raadt added a comment - Thanks for spotting that and providing a patch.
          Hide
          Petr Skoda added a comment -

          I can not imagine why would anybody want to overload the moodl_page class, but if they did they would probably want to extend it which would prevent inclusion of the child class from config.php, so my +1.

          Submitting for integration, thanks.

          Show
          Petr Skoda added a comment - I can not imagine why would anybody want to overload the moodl_page class, but if they did they would probably want to extend it which would prevent inclusion of the child class from config.php, so my +1. Submitting for integration, thanks.
          Hide
          Dan Poltawski added a comment -

          Hi,

          1. Couldn't you just do the inclusion from config.php itself?
          2. $CFG>moodlepageclass is documented in config-dist.php so I think we should also add a note about this in there too
          Show
          Dan Poltawski added a comment - Hi, Couldn't you just do the inclusion from config.php itself? $CFG>moodlepageclass is documented in config-dist.php so I think we should also add a note about this in there too
          Hide
          Matt Meisberger (WCW) added a comment -

          Couldn't you just do the inclusion from config.php itself?

          Taking this approach would prevent extensions/modules/themes from overloading the class through code only. Currently you can install a module that will override the config for page class but there is no way to get your file included

          Show
          Matt Meisberger (WCW) added a comment - Couldn't you just do the inclusion from config.php itself? Taking this approach would prevent extensions/modules/themes from overloading the class through code only. Currently you can install a module that will override the config for page class but there is no way to get your file included
          Hide
          Dan Poltawski added a comment -

          Hi,

          I'm afraid I don't really understand why you couldn't do this?

          Wherever you define the $CFG->moodlepageclass you could also do the file inclusion, it has to be early on in order for core to pick it up anyway. Am I missing something?

          So my config.php contains

          <?php
           
          // all the settings.. then:
          require('my-customfile.php');
          $CFG->moodlepageclass = 'dans_great_class';
          

          Show
          Dan Poltawski added a comment - Hi, I'm afraid I don't really understand why you couldn't do this? Wherever you define the $CFG->moodlepageclass you could also do the file inclusion, it has to be early on in order for core to pick it up anyway. Am I missing something? So my config.php contains <?php   // all the settings.. then: require('my-customfile.php'); $CFG->moodlepageclass = 'dans_great_class';
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          Petr Skoda added a comment -

          Dan: there is no way to require stuff before config.php if it "extends" existing moodle class.
          Matt: this $CFG->moodlepageclass is not intended for plugin creators, it is only for local hacks.

          I think it would be better if we did not mention this confusing and mostly useless setting in the config-dist.php at all.

          Show
          Petr Skoda added a comment - Dan: there is no way to require stuff before config.php if it "extends" existing moodle class. Matt: this $CFG->moodlepageclass is not intended for plugin creators, it is only for local hacks. I think it would be better if we did not mention this confusing and mostly useless setting in the config-dist.php at all.
          Hide
          Petr Skoda added a comment -

          Please note if you want to hack PAGE in your plugin scripts only then do it as:

          require('...config.php');
          require_once('locallib.php');
          $PAGE = new mypaclass();
          

          Show
          Petr Skoda added a comment - Please note if you want to hack PAGE in your plugin scripts only then do it as: require('...config.php'); require_once('locallib.php'); $PAGE = new mypaclass();
          Hide
          Petr Skoda added a comment -

          I have created my own patch from scratch that added the two file settings both for page class (affects all pages) and block manager class (affects only pages that do not change the default class). The condig-dist.php is updated too.

          Show
          Petr Skoda added a comment - I have created my own patch from scratch that added the two file settings both for page class (affects all pages) and block manager class (affects only pages that do not change the default class). The condig-dist.php is updated too.
          Hide
          Dan Poltawski added a comment -

          Thanks Petr. A question in my mind is worse - I wonder if we should get rid of the options altogether!

          Show
          Dan Poltawski added a comment - Thanks Petr. A question in my mind is worse - I wonder if we should get rid of the options altogether!
          Hide
          Petr Skoda added a comment -

          Hehe, I'd be happy to remove them, but I guess I have removed too many "features" from moodle already...

          I believe my patch makes both those settings usable which was not the case before imho.

          Show
          Petr Skoda added a comment - Hehe, I'd be happy to remove them, but I guess I have removed too many "features" from moodle already... I believe my patch makes both those settings usable which was not the case before imho.
          Hide
          Dan Poltawski added a comment -

          Integrated, thanks

          Show
          Dan Poltawski added a comment - Integrated, thanks
          Hide
          Rossiani Wijaya added a comment -

          Test passed.

          Show
          Rossiani Wijaya added a comment - Test passed.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          YEAR!*

          CAF*, TOT!*

          • Your effort amazingly resulted. (unbelievable :-P)
          • Closing as fixed.
          • Tons of thanks.
          Show
          Eloy Lafuente (stronk7) added a comment - YEAR!* CAF*, TOT!* Your effort amazingly resulted. (unbelievable :-P) Closing as fixed. Tons of thanks.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: