Moodle Community Sites
  1. Moodle Community Sites
  2. MDLSITE-2261

Do not allow require_once at the top level of lib.php, and other frequently included files

    Details

    • Type: Bug Bug
    • Status: Open
    • Priority: Minor Minor
    • Resolution: Unresolved
    • Component/s: Coding style
    • Labels:
      None

      Description

      This is not exactly coding style, but it should be an aspect of the coding guidelines.

      I think the best practice rules are currenly these:

      1. lib.php should not require_once anything else. It is included from a lot of other places, and needs to be fast.

      2. If a particular function in lib.php needs to use other library code from the plugin to do a particular thing (e.g. cron) the put the necessary require_once inside the function, so that the include only happens when necessary.

      3. setting.php should no do any PHP queries when it is included. If you need to fetch options (or something) from the DB, ensure you only do that when the setting is going to be changed/displayed. admin_setting_configselect has a lazy loading mechanism for this.

      4. If you need library code in setting.php, the put that in a settingslib.php file, and require_once that. settingslib.php should not include anything else. (But see below.)

      5. If you have a lot of other library code in your plugin that should not be in lib.php, you have the following three options:
      a) make a locallib.php file with all the rest of your code.
      b) have a folder called lib (or classes - see below) and put your library code in there. Idealy the code should be object oriented, with one class per file.

      6. version.php and lang files should never require_once anything else. Ditto for things in db folder.

      Comments:

      A) I would now change 4. Given 1., and given that building the navigation already includes lib.php from most plugins, I think we should put library code required by settings.php in lib.php

      B) Both lib and classes subfolders have been proposed, and in some cases implemented. We really should make a decision and clearly recommend one or the other. (I used to prefer classes, but was convinced by the lib. I don't really care which, but clarity would be good).

      C) It occurs to me that once we have agreed the rules here, most of them could be verified by codechecker.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Tim Hunt added a comment -

            Just to make it clear, the purpose of these rules is to optimise:

            • Performance
            • Consistency between plugins, and hence maintainability.
            Show
            Tim Hunt added a comment - Just to make it clear, the purpose of these rules is to optimise: Performance Consistency between plugins, and hence maintainability.
            Hide
            Dan Poltawski added a comment - - edited

            All seems non-controversial advice to me, apart from the classes one which I think is a mistake to tie up in this issue.

            But since you did, can we declare classes the defacto winner, and move on from that bike shed discussion:

            ./admin/tool/installaddon/classes
            ./cache/classes
            ./lib/behat/classes
            ./lib/editor/tinymce/classes
            ./lib/editor/tinymce/plugins/spellchecker/classes
            ./lib/phpunit/classes
            ./lib/testing/classes
            

            ./badges/lib
            ./lib
            

            Show
            Dan Poltawski added a comment - - edited All seems non-controversial advice to me, apart from the classes one which I think is a mistake to tie up in this issue. But since you did, can we declare classes the defacto winner, and move on from that bike shed discussion: ./admin/tool/installaddon/classes ./cache/classes ./lib/behat/classes ./lib/editor/tinymce/classes ./lib/editor/tinymce/plugins/spellchecker/classes ./lib/phpunit/classes ./lib/testing/classes ./badges/lib ./lib
            Hide
            Tim Hunt added a comment -

            I would be very happy with that outcome. +1.

            Note that Petr and I have discussed in the past the possbility of doing auto-loading. So, that, if you refer to an undefinde class franken_style_class_name, then it automatically require_onces get_component_directory('franken_style') . '/classes/' . 'class_name.php'.

            However, that is a separate policy question.

            Just to note that, in this secnario, simply defining a funciton

            function test(franken_style_class_name $thing) {}

            Does not cause the class definition to be loaded. Therefore, auto-loading is probably a performance win, but we would need to be sure.

            Show
            Tim Hunt added a comment - I would be very happy with that outcome. +1. Note that Petr and I have discussed in the past the possbility of doing auto-loading. So, that, if you refer to an undefinde class franken_style_class_name, then it automatically require_onces get_component_directory('franken_style') . '/classes/' . 'class_name.php'. However, that is a separate policy question. Just to note that, in this secnario, simply defining a funciton function test(franken_style_class_name $thing) {} Does not cause the class definition to be loaded. Therefore, auto-loading is probably a performance win, but we would need to be sure.
            Hide
            Dan Poltawski added a comment -

            Does any integrator disagree with these rules?

            Show
            Dan Poltawski added a comment - Does any integrator disagree with these rules?
            Hide
            Gareth J Barnard added a comment - - edited

            Hi,

            Just looked at proposed rule 1 and then at my own plugin's lib.php to find:

            require_once($CFG->dirroot . '/course/format/lib.php'); // For format_base.
            

            Which has to be there because of:

            class format_topcoll extends format_base {
            

            and therefore cannot be in a function (2).

            Cheers,

            Gareth

            Show
            Gareth J Barnard added a comment - - edited Hi, Just looked at proposed rule 1 and then at my own plugin's lib.php to find: require_once($CFG->dirroot . '/course/format/lib.php'); // For format_base. Which has to be there because of: class format_topcoll extends format_base { and therefore cannot be in a function (2). Cheers, Gareth
            Hide
            Tim Hunt added a comment -

            That is why it is better to put classes defined by the plug-in into separate files, like question types do. However, for existing APIs like course formats, we can't change that. We just need to make exeptions to the general rule for legacy cases.

            Show
            Tim Hunt added a comment - That is why it is better to put classes defined by the plug-in into separate files, like question types do. However, for existing APIs like course formats, we can't change that. We just need to make exeptions to the general rule for legacy cases.
            Hide
            Gareth J Barnard added a comment -

            Thanks Tim Hunt,

            So how would new API classes work with '1' when you needed to implement a class hierarchy without including the parent's definition file? Assuming that there is no fast loading mechanism like '.h' files in C and C++ where interfaces are defined but only the linker later on binds them together.

            And will '1' be a warning in 'Code Checker'?

            Cheers,

            Gareth

            Show
            Gareth J Barnard added a comment - Thanks Tim Hunt , So how would new API classes work with '1' when you needed to implement a class hierarchy without including the parent's definition file? Assuming that there is no fast loading mechanism like '.h' files in C and C++ where interfaces are defined but only the linker later on binds them together. And will '1' be a warning in 'Code Checker'? Cheers, Gareth
            Hide
            Petr Skoda added a comment -

            autoloading is good for backwards compatibility too, you can move stuff around easily and even create hacks for bc locations

            Show
            Petr Skoda added a comment - autoloading is good for backwards compatibility too, you can move stuff around easily and even create hacks for bc locations
            Hide
            Tim Hunt added a comment -

            I thought I had covered that:

            Don't put API classes in lib.php. Use separate file per class.

            As I suggested, look at how question types and question behaviours work.

            Show
            Tim Hunt added a comment - I thought I had covered that: Don't put API classes in lib.php. Use separate file per class. As I suggested, look at how question types and question behaviours work.
            Hide
            Petr Skoda added a comment -

            3. "PHP queries" ?

            Show
            Petr Skoda added a comment - 3. "PHP queries" ?
            Hide
            Petr Skoda added a comment -

            6. db/ may contain upgradelib.php which is used strictly from db/upgrade.php, please note plugin upgrade must not use anything from plugin lib.php (including constants).

            Show
            Petr Skoda added a comment - 6. db/ may contain upgradelib.php which is used strictly from db/upgrade.php, please note plugin upgrade must not use anything from plugin lib.php (including constants).
            Hide
            Dan Poltawski added a comment -

            A lot of this could be summarised by using autoloading when possible now I suppose.

            Show
            Dan Poltawski added a comment - A lot of this could be summarised by using autoloading when possible now I suppose.
            Hide
            Dan Poltawski added a comment -

            Tim, if you get a moment, could you review this issue and see if anything is still remaining to be decided here based on the fact we now have autoloading and edit the summary for those things needed?

            Show
            Dan Poltawski added a comment - Tim, if you get a moment, could you review this issue and see if anything is still remaining to be decided here based on the fact we now have autoloading and edit the summary for those things needed?
            Hide
            Tim Hunt added a comment -

            This issue was created around the time the OU was fixing the performance regressions in Moodle 2.4. It was based on idenfiying common performance screw-ups that people were making.

            The thing that remains would be:

            Impose, and get codechcecker to enforce the rule, no require_once at the top level in any of:

            • lib.php
            • version.php
            • settings.php
            • lang/en/*.php
            Show
            Tim Hunt added a comment - This issue was created around the time the OU was fixing the performance regressions in Moodle 2.4. It was based on idenfiying common performance screw-ups that people were making. The thing that remains would be: Impose, and get codechcecker to enforce the rule, no require_once at the top level in any of: lib.php version.php settings.php lang/en/*.php
            Hide
            Tim Hunt added a comment -

            Just to say that, while I created this 1.5 years ago (as the results of some painful profiling to fix performance issues in 2.4) I still stand by most of what I wrote there.

            Since then only one significant thing has changed, auto-loading. That gives a better alternative in some cases, so I would amend some of the points in the description:

            2. If a particular function in lib.php needs to use other library code, then ideally that library code should be in a class that is auto-loaded. If that is not possible, put the necessary require_once inside the function, so that the include only happens when necessary.

            4. If you need a custom admin_settings class in setting.php, define the class in the classes folder so that it can be auto-loaded.

            5. If you have a lot of other library code in your plugin that should not be in lib.php, you have the following three options:
            a) break the library code into classes and objects, which can then be auto-loaded from the classes folder.
            b) make a locallib.php file with all the rest of your code.

            Show
            Tim Hunt added a comment - Just to say that, while I created this 1.5 years ago (as the results of some painful profiling to fix performance issues in 2.4) I still stand by most of what I wrote there. Since then only one significant thing has changed, auto-loading. That gives a better alternative in some cases, so I would amend some of the points in the description: 2. If a particular function in lib.php needs to use other library code, then ideally that library code should be in a class that is auto-loaded. If that is not possible, put the necessary require_once inside the function, so that the include only happens when necessary. 4. If you need a custom admin_settings class in setting.php, define the class in the classes folder so that it can be auto-loaded. 5. If you have a lot of other library code in your plugin that should not be in lib.php, you have the following three options: a) break the library code into classes and objects, which can then be auto-loaded from the classes folder. b) make a locallib.php file with all the rest of your code.
            Hide
            Dan Poltawski added a comment -

            Please can you propose an edit to the coding style doc - it will make it much easier to push this forward.

            Show
            Dan Poltawski added a comment - Please can you propose an edit to the coding style doc - it will make it much easier to push this forward.

              People

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

                Dates

                • Created:
                  Updated:

                  Development