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

We should clarify the rules for certain key files in plugins

    Details

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

      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.

        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 Škoda 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 Škoda 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 Škoda added a comment -

          3. "PHP queries" ?

          Show
          Petr Škoda added a comment - 3. "PHP queries" ?
          Hide
          Petr Škoda 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 Škoda 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.

            People

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

              Dates

              • Created:
                Updated:

                Development