Moodle
  1. Moodle
  2. MDL-26943

Enable sub-plugins in Local plugins

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0.2, 2.4, 2.5
    • Fix Version/s: 2.6
    • Component/s: Libraries
    • Labels:
    • Testing Instructions:
      Hide

      1/ unzip attached testsub.zip file to your local dir
      2/ make sure one new plugin with two subplugins gets installed
      3/ try to uninstall 1 subplugin from plugins overview page
      4/ install it again
      5/ try to uinstall the local plugin - it should delete own subplugins
      6/ run phpunit tests

      Show
      1/ unzip attached testsub.zip file to your local dir 2/ make sure one new plugin with two subplugins gets installed 3/ try to uninstall 1 subplugin from plugins overview page 4/ install it again 5/ try to uinstall the local plugin - it should delete own subplugins 6/ run phpunit tests
    • Difficulty:
      Easy
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_26_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      w27_MDL-26943_m26_localsubplugins
    • Rank:
      16572

      Description

      Currently, sub-plugins are only available for Activity Modules.
      This makes sense since it's reasonably expensive to check each plug-in of a given type for sub-plugins. However I'd like them to be available for Local plugins too.

      Rationale:
      There wont be many local plugins, so there won't be many to check.
      Local plugins are designed for features that wont fit into the general structure of plugins. Therefore, they should be as flexible as possible - enabling sub-plugins would enhance this.
      In terms of code, it's only a small change.

        Issue Links

          Activity

          Hide
          Mark Johnson added a comment -

          There's a branch on my github fork with a fix against the current weekly for discussion.
          https://github.com/marxjohnson/moodle/tree/MDL-26943

          Show
          Mark Johnson added a comment - There's a branch on my github fork with a fix against the current weekly for discussion. https://github.com/marxjohnson/moodle/tree/MDL-26943
          Hide
          Nicholas Koeppen added a comment -

          I agree with Mark that this should be a behavior of local plugins. The whole point of local plugins was to be the special case that Moodle didn't classify or handle properly.

          I would argue that Moodle seems to be growing its list of plugin types in a static fashion. For instance, look at the list of question, grade, and report plugins:

          qtype, qbehavior, qformat, gradeexport, gradeimport, gradingform, report, coursereport, graderreport

          These aren't subplugins, but my point is that the current model seems to be bloated.

          Show
          Nicholas Koeppen added a comment - I agree with Mark that this should be a behavior of local plugins. The whole point of local plugins was to be the special case that Moodle didn't classify or handle properly. I would argue that Moodle seems to be growing its list of plugin types in a static fashion. For instance, look at the list of question, grade, and report plugins: qtype, qbehavior, qformat, gradeexport, gradeimport, gradingform, report, coursereport, graderreport These aren't subplugins, but my point is that the current model seems to be bloated.
          Hide
          Juan Leyva added a comment -

          +1 I think that this can improve the quality an number of generic local plugins

          Show
          Juan Leyva added a comment - +1 I think that this can improve the quality an number of generic local plugins
          Hide
          Juan Leyva added a comment -

          This seems to be something already requested for other types of plugins, any chance for having this in core?

          See MDL-20378 and MDL-37944, also

          Show
          Juan Leyva added a comment - This seems to be something already requested for other types of plugins, any chance for having this in core? See MDL-20378 and MDL-37944 , also
          Hide
          Martin Dougiamas added a comment -

          Actually /local was for local plugins that you never intend to distribute, and where you don't need to worry about namespace clashes. It's not meant to be the generic plugin place.

          That's not to say that /local shouldn't ever support subplugins, that might be useful in some cases, but I think the better choice might be to develop /other or /general as a place for those sort of plugins.

          I've started that discussion here MDL-38322

          Show
          Martin Dougiamas added a comment - Actually /local was for local plugins that you never intend to distribute, and where you don't need to worry about namespace clashes. It's not meant to be the generic plugin place. That's not to say that /local shouldn't ever support subplugins, that might be useful in some cases, but I think the better choice might be to develop /other or /general as a place for those sort of plugins. I've started that discussion here MDL-38322
          Hide
          Juan Leyva added a comment -

          Now that MDL-38322 is closed, what about this issue? Is ok adding subplugins?

          Show
          Juan Leyva added a comment - Now that MDL-38322 is closed, what about this issue? Is ok adding subplugins?
          Hide
          Martin Dougiamas added a comment -

          Yes totally OK. We just need some code. Has anyone got anything for 2.5 already that can be reviewed? Otherwise shall I ping Petr?

          Show
          Martin Dougiamas added a comment - Yes totally OK. We just need some code. Has anyone got anything for 2.5 already that can be reviewed? Otherwise shall I ping Petr?
          Hide
          Juan Leyva added a comment -

          Mark Johnson made a patch some time ago:

          https://github.com/marxjohnson/moodle/commit/e8e8ef68efcf0dad36e733c575560d74d11a1abf

          But isn't using cache or something like that

          Petr mentioned that it should be done using the new cache system, so I think we need some feedback from him

          Show
          Juan Leyva added a comment - Mark Johnson made a patch some time ago: https://github.com/marxjohnson/moodle/commit/e8e8ef68efcf0dad36e733c575560d74d11a1abf But isn't using cache or something like that Petr mentioned that it should be done using the new cache system, so I think we need some feedback from him
          Hide
          Petr Škoda added a comment - - edited

          Reassinging...

          Juan: please fix the extra line, missing space after if and extra line before else and submit for integration, thanks!

          Show
          Petr Škoda added a comment - - edited Reassinging... Juan: please fix the extra line, missing space after if and extra line before else and submit for integration, thanks!
          Hide
          Juan Leyva added a comment - - edited

          I just cherry picked Marx Johnson commit in a clean branch and fixed the coding style errors.

          As far as I know, I don't have permission for submit to integration. Can anyone with enough permission submit it?

          Regards

          Show
          Juan Leyva added a comment - - edited I just cherry picked Marx Johnson commit in a clean branch and fixed the coding style errors. As far as I know, I don't have permission for submit to integration. Can anyone with enough permission submit it? Regards
          Hide
          Dan Poltawski added a comment -

          We need testing instructions for this

          Show
          Dan Poltawski added a comment - We need testing instructions for this
          Hide
          Juan Leyva added a comment -

          I just added the testing instructions, can anyone submit it to integration review?

          Show
          Juan Leyva added a comment - I just added the testing instructions, can anyone submit it to integration review?
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Uhm,

          while we certainly can add support for local subplugins... I think that we must at least offer to them the same level of support that other subplugins (mod, editor), have right now....

          And that includes, as very, very minimum required:

          • uninstall_plugin() --> missing local handling completely.
          • get_plugin_types() the proposed patch is acceptable there, though surely could be added to the iterator some lines above.
          • plugin_manager: get_plugins(), get_subplugins(),

          And surely not now, but in the future:

          • add plugin_supports() support to local subplugins.
          • add plugin settings support to all subplugins.

          So, because that "very minimum required" is not complete and also because:

          I'm reopening this for some more love... ciao

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Uhm, while we certainly can add support for local subplugins... I think that we must at least offer to them the same level of support that other subplugins (mod, editor), have right now.... And that includes, as very, very minimum required: uninstall_plugin() --> missing local handling completely. get_plugin_types() the proposed patch is acceptable there, though surely could be added to the iterator some lines above. plugin_manager: get_plugins(), get_subplugins(), And surely not now, but in the future: add plugin_supports() support to local subplugins. add plugin settings support to all subplugins. So, because that "very minimum required" is not complete and also because: http://stronk7.doesntexist.com/job/Precheck%20remote%20branch/328/artifact/work/smurf.xml I don't get the modification in get_plugin_list() (unnecessary?) I'm reopening this for some more love... ciao
          Hide
          Juan Leyva added a comment -

          Hi,
          thanks for your feedback Eloy.

          The modification in get_plugin_list is needed to avoid a circular reference:

          get_plugin_types calls get_plugin_list for obtaining the subplugins, if we don't add a special case for local before the "main else" inside get_plugin_list then get_plugin_types will be called again so we will have a circular reference

          Notice that this is done for all the plugins types that supports subplugins (mod and editor), so is something that previously exists. Maybe the original comment should be improved

          Now that it seems that MDL-39087 will be for 2.5, I will wait until that issue is integrated and master updated

          Regards

          Show
          Juan Leyva added a comment - Hi, thanks for your feedback Eloy. The modification in get_plugin_list is needed to avoid a circular reference: get_plugin_types calls get_plugin_list for obtaining the subplugins, if we don't add a special case for local before the "main else" inside get_plugin_list then get_plugin_types will be called again so we will have a circular reference Notice that this is done for all the plugins types that supports subplugins (mod and editor), so is something that previously exists. Maybe the original comment should be improved Now that it seems that MDL-39087 will be for 2.5, I will wait until that issue is integrated and master updated Regards
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          aha, got the circular ref thingy, thanks!

          edited: and yeah, surely worth waiting MDL-39087 to land.

          Show
          Eloy Lafuente (stronk7) added a comment - - edited aha, got the circular ref thingy, thanks! edited: and yeah, surely worth waiting MDL-39087 to land.
          Hide
          Juan Leyva added a comment -

          For the record: I'm using MDL-33041, mainly https://github.com/moodle/moodle/commit/c57fc98b278bb5d1c845581812007c852571a4d6 as an example for this issue

          Show
          Juan Leyva added a comment - For the record: I'm using MDL-33041 , mainly https://github.com/moodle/moodle/commit/c57fc98b278bb5d1c845581812007c852571a4d6 as an example for this issue
          Hide
          Juan Leyva added a comment -

          Hi again,

          So I made all the changes requested by Eloy, and also follow MDL-33041 as a guide for adding subplugins to an existing plugin type.

          I just rebased my changes to master, can anyone submit to integrator review?

          Show
          Juan Leyva added a comment - Hi again, So I made all the changes requested by Eloy, and also follow MDL-33041 as a guide for adding subplugins to an existing plugin type. I just rebased my changes to master, can anyone submit to integrator review?
          Hide
          Juan Leyva added a comment -

          Ping... can anyone please submit to integration review?

          Show
          Juan Leyva added a comment - Ping... can anyone please submit to integration review?
          Hide
          Petr Škoda added a comment -

          to integrators: this collides with MDL-39854, please do not integrate yet

          Show
          Petr Škoda added a comment - to integrators: this collides with MDL-39854 , please do not integrate yet
          Hide
          Petr Škoda added a comment -

          Juan Leyva: thanks for your work on this!

          I have created a new patch that adds general support for new subplugins into core_component class, including unit test and sample local plugin for testing.

          Show
          Petr Škoda added a comment - Juan Leyva : thanks for your work on this! I have created a new patch that adds general support for new subplugins into core_component class, including unit test and sample local plugin for testing.
          Hide
          Damyon Wiese added a comment -

          More complicated test local plugin.

          Show
          Damyon Wiese added a comment - More complicated test local plugin.
          Hide
          Damyon Wiese added a comment - - edited

          Thanks everyone for working on this.

          Looking at the latest patch I found a bug in xmldb editor (creates the wrong default tablename for subplugins). This is minor - I'll add a new issue for this as it is an existing bug.

          I found another existing bug where local plugins cannot be removed from /admin/localplugins.php (the link just goes back to the same page). Will add a tracker for that (if there isn't one already).

          Also I updated the test local plugin to support more than one type of subplugin and have some database tables to get created/removed. Will update the testing instructions to use this one.

          I also noticed that the unit test doesn't test much...

          Show
          Damyon Wiese added a comment - - edited Thanks everyone for working on this. Looking at the latest patch I found a bug in xmldb editor (creates the wrong default tablename for subplugins). This is minor - I'll add a new issue for this as it is an existing bug. I found another existing bug where local plugins cannot be removed from /admin/localplugins.php (the link just goes back to the same page). Will add a tracker for that (if there isn't one already). Also I updated the test local plugin to support more than one type of subplugin and have some database tables to get created/removed. Will update the testing instructions to use this one. I also noticed that the unit test doesn't test much...
          Hide
          Damyon Wiese added a comment -

          The local plugin issue looks related to MDL-39088

          Show
          Damyon Wiese added a comment - The local plugin issue looks related to MDL-39088
          Hide
          Damyon Wiese added a comment -

          Thanks Petr for the updated patch, it all looks fine to me.

          I added one change to mention db/subplugins.php in the /local/readme.txt file
          to make it a little clearer that we can support sub plugins now.

          Integrated to master only.

          Show
          Damyon Wiese added a comment - Thanks Petr for the updated patch, it all looks fine to me. I added one change to mention db/subplugins.php in the /local/readme.txt file to make it a little clearer that we can support sub plugins now. Integrated to master only.
          Hide
          Petr Škoda added a comment -

          Thanks a lot, I have added a commit in MDL-39088 to fix the uninstall url typo.

          Show
          Petr Škoda added a comment - Thanks a lot, I have added a commit in MDL-39088 to fix the uninstall url typo.
          Hide
          Rajesh Taneja added a comment -

          Thanks for fixing this Petr,

          I was able to uninstall one subplugin and by uninstalling local plugin subplugins were also uninstalled.

          Show
          Rajesh Taneja added a comment - Thanks for fixing this Petr, I was able to uninstall one subplugin and by uninstalling local plugin subplugins were also uninstalled.
          Hide
          Damyon Wiese added a comment -

          This issue is fixed! Hurray! Hurray!
          Your issue is fixed, what a wonderful day!

          Cheers, Damyon

          Show
          Damyon Wiese added a comment - This issue is fixed! Hurray! Hurray! Your issue is fixed, what a wonderful day! Cheers, Damyon

            People

            • Votes:
              6 Vote for this issue
              Watchers:
              11 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: