Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-26943

Enable sub-plugins in Local plugins

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: 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

      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.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            marxjohnson 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
            marxjohnson 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
            nickkoeppen 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
            nickkoeppen 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
            jleyva Juan Leyva added a comment -

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

            Show
            jleyva Juan Leyva added a comment - +1 I think that this can improve the quality an number of generic local plugins
            Hide
            jleyva 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
            jleyva 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
            dougiamas 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
            dougiamas 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
            jleyva Juan Leyva added a comment -

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

            Show
            jleyva Juan Leyva added a comment - Now that MDL-38322 is closed, what about this issue? Is ok adding subplugins?
            Hide
            dougiamas 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
            dougiamas 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
            jleyva 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
            jleyva 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
            skodak Petr Skoda 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
            skodak Petr Skoda 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
            jleyva 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
            jleyva 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
            poltawski Dan Poltawski added a comment -

            We need testing instructions for this

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

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

            Show
            jleyva Juan Leyva added a comment - I just added the testing instructions, can anyone submit it to integration review?
            Hide
            stronk7 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
            stronk7 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
            jleyva 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
            jleyva 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
            stronk7 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
            stronk7 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
            jleyva 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
            jleyva 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
            jleyva 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
            jleyva 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
            jleyva Juan Leyva added a comment -

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

            Show
            jleyva Juan Leyva added a comment - Ping... can anyone please submit to integration review?
            Hide
            skodak Petr Skoda added a comment -

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

            Show
            skodak Petr Skoda added a comment - to integrators: this collides with MDL-39854 , please do not integrate yet
            Hide
            skodak Petr Skoda 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
            skodak Petr Skoda 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 Damyon Wiese added a comment -

            More complicated test local plugin.

            Show
            damyon Damyon Wiese added a comment - More complicated test local plugin.
            Hide
            damyon 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 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 Damyon Wiese added a comment -

            The local plugin issue looks related to MDL-39088

            Show
            damyon Damyon Wiese added a comment - The local plugin issue looks related to MDL-39088
            Hide
            damyon 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 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
            skodak Petr Skoda added a comment -

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

            Show
            skodak Petr Skoda added a comment - Thanks a lot, I have added a commit in MDL-39088 to fix the uninstall url typo.
            Hide
            rajeshtaneja 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
            rajeshtaneja 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 Damyon Wiese added a comment -

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

            Cheers, Damyon

            Show
            damyon 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:
                  Fix Release Date:
                  18/Nov/13