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

      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

            Mark Johnson created issue -
            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
            Martin Dougiamas made changes -
            Field Original Value New Value
            Workflow MDL Workflow [ 68840 ] MDL Full Workflow [ 76194 ]
            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.
            Juan Leyva made changes -
            Labels partner
            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
            Juan Leyva made changes -
            Affects Version/s 2.4 [ 12255 ]
            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
            Martin Dougiamas made changes -
            Link This issue has been marked as being related by MDL-38322 [ 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
            Martin Dougiamas made changes -
            Fix Version/s DEV backlog [ 10464 ]
            Martin Dougiamas made changes -
            Assignee moodle.com [ moodle.com ] Petr Škoda [ skodak ]
            Hide
            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
            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!
            Petr Skoda made changes -
            Assignee Petr Škoda [ skodak ] Juan Leyva [ jleyva ]
            Petr Skoda made changes -
            Peer reviewer skodak
            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
            Juan Leyva made changes -
            Martin Dougiamas made changes -
            Status Open [ 1 ] Waiting for integration review [ 10010 ]
            Hide
            Dan Poltawski added a comment -

            We need testing instructions for this

            Show
            Dan Poltawski added a comment - We need testing instructions for this
            Dan Poltawski made changes -
            Status Waiting for integration review [ 10010 ] Reopened [ 4 ]
            Juan Leyva made changes -
            Testing Instructions - Create or reuse an empty local plugin, https://moodle.org/plugins/view.php?plugin=local_wstemplate It's a good choice.
            - Add a new valid subplugins.php file in db/
            - Create the subplugins directory referenced in db/subplugins.php
            - Inside this directory, create one or a couple of subplugins with a valid lang, version.php file and a valid db/access.php, optionallity an install.xml file
            - Install the local plugin
            - Check that the installer detects the subplugin
            - Check that the capabilities defined at db/access.php are created
            - Check that the tables defined in install.xml are created (Optional)

            You can find examples of the subplugins.php file and subplugins structure in https://github.com/moodle/moodle/tree/master/mod/assign
            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?
            Petr Skoda made changes -
            Status Reopened [ 4 ] Waiting for integration review [ 10010 ]
            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
            Eloy Lafuente (stronk7) made changes -
            Status Waiting for integration review [ 10010 ] Reopened [ 4 ]
            Juan Leyva made changes -
            Link This issue will be (partly) resolved by MDL-39087 [ MDL-39087 ]
            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 Skoda added a comment -

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

            Show
            Petr Skoda added a comment - to integrators: this collides with MDL-39854 , please do not integrate yet
            Petr Skoda made changes -
            Link This issue is blocked by MDL-39854 [ MDL-39854 ]
            Petr Skoda made changes -
            Assignee Juan Leyva [ jleyva ] Petr Škoda [ skodak ]
            Petr Skoda made changes -
            Link This issue is blocked by MDL-39088 [ MDL-39088 ]
            Petr Skoda made changes -
            Status Reopened [ 4 ] Waiting for integration review [ 10010 ]
            Pull Master Diff URL https://github.com/jleyva/moodle/compare/master...MDL-26943 https://github.com/skodak/moodle/compare/w27_MDL-39088_m26_uninstall...w27_MDL-26943_m26_localsubplugins
            Pull Master Branch MDL-26943 w27_MDL-26943_m26_localsubplugins
            Pull from Repository git://github.com/jleyva/moodle.git https://github.com/skodak/moodle.git
            Fix Version/s 2.6 [ 12579 ]
            Fix Version/s DEV backlog [ 10464 ]
            Testing Instructions - Create or reuse an empty local plugin, https://moodle.org/plugins/view.php?plugin=local_wstemplate It's a good choice.
            - Add a new valid subplugins.php file in db/
            - Create the subplugins directory referenced in db/subplugins.php
            - Inside this directory, create one or a couple of subplugins with a valid lang, version.php file and a valid db/access.php, optionallity an install.xml file
            - Install the local plugin
            - Check that the installer detects the subplugin
            - Check that the capabilities defined at db/access.php are created
            - Check that the tables defined in install.xml are created (Optional)

            You can find examples of the subplugins.php file and subplugins structure in https://github.com/moodle/moodle/tree/master/mod/assign
            1/ unzip attached localtest.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
            Petr Skoda made changes -
            Attachment testsub.zip [ 33280 ]
            Petr Skoda made changes -
            Testing Instructions 1/ unzip attached localtest.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
            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
            Hide
            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
            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.
            Sam Hemelryk made changes -
            Currently in integration Yes [ 10041 ]
            Damyon Wiese made changes -
            Status Waiting for integration review [ 10010 ] Integration review in progress [ 10004 ]
            Integrator damyon
            Hide
            Damyon Wiese added a comment -

            More complicated test local plugin.

            Show
            Damyon Wiese added a comment - More complicated test local plugin.
            Damyon Wiese made changes -
            Attachment testsub-v2.zip [ 33313 ]
            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...
            Damyon Wiese made changes -
            Link This issue testing discovered MDL-40423 [ MDL-40423 ]
            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.
            Damyon Wiese made changes -
            Status Integration review in progress [ 10004 ] Waiting for testing [ 10005 ]
            Affects Version/s 2.5 [ 12452 ]
            Hide
            Petr Skoda added a comment -

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

            Show
            Petr Skoda added a comment - Thanks a lot, I have added a commit in MDL-39088 to fix the uninstall url typo.
            Michael de Raadt made changes -
            Tester rajeshtaneja
            Rajesh Taneja made changes -
            Status Waiting for testing [ 10005 ] Testing in progress [ 10011 ]
            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.
            Rajesh Taneja made changes -
            Status Testing in progress [ 10011 ] Tested [ 10006 ]
            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
            Damyon Wiese made changes -
            Status Tested [ 10006 ] Closed [ 6 ]
            Resolution Fixed [ 1 ]
            Currently in integration Yes [ 10041 ]
            Integration date 04/Jul/13
            Petr Skoda made changes -
            Link This issue has been marked as being related by MDL-41267 [ MDL-41267 ]

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: