Moodle
  1. Moodle
  2. MDL-35442

Local plugins: add settings and uninstall on Plugin overview page

    Details

    • Testing Instructions:
      Hide

      1. Install any local plugin that contains settings.php
      2. Go to Plugins overview page in Administration
      3. Make sure installed plugin has uninstall and settings links
      4. Click on settings and make sure that plugin settings page is open
      5. Click on uninstall link and go through uninstall process

      Show
      1. Install any local plugin that contains settings.php 2. Go to Plugins overview page in Administration 3. Make sure installed plugin has uninstall and settings links 4. Click on settings and make sure that plugin settings page is open 5. Click on uninstall link and go through uninstall process
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-35442-master

      Description

      While admin/localplugins.php supports plugin deinstallation, there is no link to it on Plugin overview page. No Settings link either.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Ruslan Kabalin added a comment -

            Can be cleanly cherry-picked to 2.x versions

            Show
            Ruslan Kabalin added a comment - Can be cleanly cherry-picked to 2.x versions
            Hide
            Michael de Raadt added a comment -

            Thanks for sharing this. We'll see if we can get it reviewed soon.

            Show
            Michael de Raadt added a comment - Thanks for sharing this. We'll see if we can get it reviewed soon.
            Hide
            David Mudrak added a comment -

            Look at the patch ...

            Show
            David Mudrak added a comment - Look at the patch ...
            Hide
            David Mudrak added a comment -

            Appart from the fact that there should be two empty lines between class blocks, the patch smells ok to me. Thanks Ruslan!

            Show
            David Mudrak added a comment - Appart from the fact that there should be two empty lines between class blocks, the patch smells ok to me. Thanks Ruslan!
            Hide
            Ruslan Kabalin added a comment -

            Thanks David, I have updated the patch having added extra blank line.

            Show
            Ruslan Kabalin added a comment - Thanks David, I have updated the patch having added extra blank line.
            Hide
            Ruslan Kabalin added a comment -

            Ready for integration

            Show
            Ruslan Kabalin added a comment - Ready for integration
            Hide
            Dan Poltawski added a comment -

            The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

            TIA and ciao

            Show
            Dan Poltawski added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
            Hide
            Dan Poltawski added a comment -

            Thanks Ruslan, i've integrated that to 22, 23 and master

            Show
            Dan Poltawski added a comment - Thanks Ruslan, i've integrated that to 22, 23 and master
            Hide
            Dan Poltawski added a comment -

            GRR.

            This breaks 2.2:
            Moodle 2.2.5+ (Build: 20120920) command line installation program
            PHP Fatal error: Class 'plugininfo_base' not found in /Users/Shared/Jenkins/Home/git_repositories/MOODLE_22_STABLE/lib/pluginlib.php on line 1697
            PHP Stack trace:
            PHP 1.

            {main}() /Users/Shared/Jenkins/Home/git_repositories/MOODLE_22_STABLE/admin/cli/install.php:0
            PHP 2. require_once() /Users/Shared/Jenkins/Home/git_repositories/MOODLE_22_STABLE/admin/cli/install.php:679

            Fatal error: Class 'plugininfo_base' not found in /Users/Shared/Jenkins/Home/git_repositories/MOODLE_22_STABLE/lib/pluginlib.php on line 1697

            Call Stack:
            0.0022 992160 1. {main}

            () /Users/Shared/Jenkins/Home/git_repositories/MOODLE_22_STABLE/admin/cli/install.php:0
            0.2458 36174024 2. require_once('/Users/Shared/Jenkins/Home/git_repositories/MOODLE_22_STABLE/lib/pluginlib.php') /Users/Shared/Jenkins/Home/git_repositories/MOODLE_22_STABLE/admin/cli/install.php:679

            Show
            Dan Poltawski added a comment - GRR. This breaks 2.2: Moodle 2.2.5+ (Build: 20120920) command line installation program PHP Fatal error: Class 'plugininfo_base' not found in /Users/Shared/Jenkins/Home/git_repositories/MOODLE_22_STABLE/lib/pluginlib.php on line 1697 PHP Stack trace: PHP 1. {main}() /Users/Shared/Jenkins/Home/git_repositories/MOODLE_22_STABLE/admin/cli/install.php:0 PHP 2. require_once() /Users/Shared/Jenkins/Home/git_repositories/MOODLE_22_STABLE/admin/cli/install.php:679 Fatal error: Class 'plugininfo_base' not found in /Users/Shared/Jenkins/Home/git_repositories/MOODLE_22_STABLE/lib/pluginlib.php on line 1697 Call Stack: 0.0022 992160 1. {main} () /Users/Shared/Jenkins/Home/git_repositories/MOODLE_22_STABLE/admin/cli/install.php:0 0.2458 36174024 2. require_once('/Users/Shared/Jenkins/Home/git_repositories/MOODLE_22_STABLE/lib/pluginlib.php') /Users/Shared/Jenkins/Home/git_repositories/MOODLE_22_STABLE/admin/cli/install.php:679
            Hide
            Dan Poltawski added a comment -

            I've reverted the change to 2.2.

            Lesson: please prepare/test your changes on all the branches to be integrated to..

            Show
            Dan Poltawski added a comment - I've reverted the change to 2.2. Lesson: please prepare/test your changes on all the branches to be integrated to..
            Hide
            Ruslan Kabalin added a comment -

            Shall we keep it on 2.3 and master only?

            Show
            Ruslan Kabalin added a comment - Shall we keep it on 2.3 and master only?
            Hide
            David Mudrak added a comment -

            pluginlib.php has been refactored for 2.3 as per the feedback I got from Tim Hunt on the organisation of the code. This feature is still doable in 2.2 too, although it must be implemented in a slightly modified. Anyway, my +1 to have this for 2.3 only, or even for master only.

            Show
            David Mudrak added a comment - pluginlib.php has been refactored for 2.3 as per the feedback I got from Tim Hunt on the organisation of the code. This feature is still doable in 2.2 too, although it must be implemented in a slightly modified. Anyway, my +1 to have this for 2.3 only, or even for master only.
            Hide
            Dan Poltawski added a comment -

            Yep, lets just leave it for 2.3+master. Strictly speaking I suppose I shouldn't have done it for anything but master since its an improvement really :/

            Show
            Dan Poltawski added a comment - Yep, lets just leave it for 2.3+master. Strictly speaking I suppose I shouldn't have done it for anything but master since its an improvement really :/
            Hide
            Frédéric Massart added a comment - - edited

            Test failed.

            While I was looking for plugins with a setting page, I downloaded Adminer, Flavours and Welcome from the plugins database. The problem is that Adminer and Flavours have a settings page, but do not create a section from what I understood, therefore this error is raised:

            Section error!
             
            More information about this error
             
            Debug info: 
            Error code: sectionerror
            Stack trace:
            line 467 of /lib/setuplib.php: moodle_exception thrown
            line 22 of /admin/settings.php: call to print_error()
            

            We should probably hide the link when the section does not exist, so that users won't think their plugins settings are broken.

            Show
            Frédéric Massart added a comment - - edited Test failed. While I was looking for plugins with a setting page, I downloaded Adminer, Flavours and Welcome from the plugins database. The problem is that Adminer and Flavours have a settings page, but do not create a section from what I understood, therefore this error is raised: Section error!   More information about this error   Debug info: Error code: sectionerror Stack trace: line 467 of /lib/setuplib.php: moodle_exception thrown line 22 of /admin/settings.php: call to print_error() We should probably hide the link when the section does not exist, so that users won't think their plugins settings are broken.
            Hide
            Ruslan Kabalin added a comment -

            @Frédéric Hiding link is difficult as $ADMIN does not exist at the point when get_settings_url() is called... One option obviously is to remove settings link.

            Show
            Ruslan Kabalin added a comment - @Frédéric Hiding link is difficult as $ADMIN does not exist at the point when get_settings_url() is called... One option obviously is to remove settings link.
            Hide
            Dan Poltawski added a comment -

            Hi Ruslan,

            Are you able to come up with a solution? Otherwise we'll need to revert this.

            Show
            Dan Poltawski added a comment - Hi Ruslan, Are you able to come up with a solution? Otherwise we'll need to revert this.
            Hide
            Ruslan Kabalin added a comment - - edited

            I can only suggest to remove settings link, initialisation of whole navigation stuff just to determine if plugin added something to localplugin settings tree or not is too much I think.

            On the other hand, this could happen with any plugin (non local) that adds something to some different settings tree rather than own. May be plugins developers should keep settings within the same plugin type tree rather than spreading them all over the shop?

            Just to clarify what has happened. Adminer plugin has settings.php file, but it adds custom link item to 'server' settings tree instead of creating section within 'local_plugins'. So, on the attempt to display settings using typical approach, it complains that section is not defined.

            Show
            Ruslan Kabalin added a comment - - edited I can only suggest to remove settings link, initialisation of whole navigation stuff just to determine if plugin added something to localplugin settings tree or not is too much I think. On the other hand, this could happen with any plugin (non local) that adds something to some different settings tree rather than own. May be plugins developers should keep settings within the same plugin type tree rather than spreading them all over the shop? Just to clarify what has happened. Adminer plugin has settings.php file, but it adds custom link item to 'server' settings tree instead of creating section within 'local_plugins'. So, on the attempt to display settings using typical approach, it complains that section is not defined.
            Hide
            Dan Poltawski added a comment -

            We've agreed to address this in a seperate issue: MDL-35661 as it looks like it affects other plugin types too.

            Note that I don't think we have a policy for this kind of thing, so I don't think the adminer plugin is doing anythign incorrect.

            Show
            Dan Poltawski added a comment - We've agreed to address this in a seperate issue: MDL-35661 as it looks like it affects other plugin types too. Note that I don't think we have a policy for this kind of thing, so I don't think the adminer plugin is doing anythign incorrect.
            Hide
            Dan Poltawski added a comment -

            Passing, based on the new issue.

            Show
            Dan Poltawski added a comment - Passing, based on the new issue.
            Hide
            Dan Poltawski added a comment -

            Congratulations, you've done it!

            Nf n erjneq sbe fhpprfshy vagrtengvba vagb guvf jrrxf eryrnfr, V pna abj qvfpybfr gb lbh gur rkvfgnapr bs shapgvba fge_ebg13(), gb tb va lbhe gbbyxvg nybat jvgu uggc://cuc.arg/znahny/ra/shapgvba.tmtrgff.cuc

            Cyrnfr qb abg nyybj guvf vasbezngvba gb cnff shegure.

            Show
            Dan Poltawski added a comment - Congratulations, you've done it! Nf n erjneq sbe fhpprfshy vagrtengvba vagb guvf jrrxf eryrnfr, V pna abj qvfpybfr gb lbh gur rkvfgnapr bs shapgvba fge_ebg13(), gb tb va lbhe gbbyxvg nybat jvgu uggc://cuc.arg/znahny/ra/shapgvba.tmtrgff.cuc Cyrnfr qb abg nyybj guvf vasbezngvba gb cnff shegure.
            Hide
            Marina Glancy added a comment -

            Ruslan, I saw this issue because I had merge conflict with my issue.
            Please look at the comment
            http://tracker.moodle.org/browse/MDL-35260?focusedCommentId=180913#comment-180913

            I will correct it myself in my branch. Just to let you know that it seems that you don't use local plugins settings correctly.

            Show
            Marina Glancy added a comment - Ruslan, I saw this issue because I had merge conflict with my issue. Please look at the comment http://tracker.moodle.org/browse/MDL-35260?focusedCommentId=180913#comment-180913 I will correct it myself in my branch. Just to let you know that it seems that you don't use local plugins settings correctly.

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: