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:
    • Rank:
      43209

      Description

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

        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 Škoda 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 Škoda 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 Škoda 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 Škoda 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 Škoda 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 Škoda 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 Škoda 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 Škoda 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 Škoda 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 Škoda 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: