Uploaded image for project: '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

          Attachments

            Issue Links

              Activity

              Hide
              kabalin Ruslan Kabalin added a comment -

              Can be cleanly cherry-picked to 2.x versions

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

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

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

              Look at the patch ...

              Show
              mudrd8mz David Mudrák added a comment - Look at the patch ...
              Hide
              mudrd8mz David Mudrák 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
              mudrd8mz David Mudrák 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
              kabalin Ruslan Kabalin added a comment -

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

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

              Ready for integration

              Show
              kabalin Ruslan Kabalin added a comment - Ready for integration
              Hide
              poltawski 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
              poltawski 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
              poltawski Dan Poltawski added a comment -

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

              Show
              poltawski Dan Poltawski added a comment - Thanks Ruslan, i've integrated that to 22, 23 and master
              Hide
              poltawski 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
              poltawski 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
              poltawski 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
              poltawski 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
              kabalin Ruslan Kabalin added a comment -

              Shall we keep it on 2.3 and master only?

              Show
              kabalin Ruslan Kabalin added a comment - Shall we keep it on 2.3 and master only?
              Hide
              mudrd8mz David Mudrák 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
              mudrd8mz David Mudrák 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
              poltawski 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
              poltawski 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
              fred 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
              fred 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
              kabalin 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
              kabalin 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
              poltawski Dan Poltawski added a comment -

              Hi Ruslan,

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

              Show
              poltawski Dan Poltawski added a comment - Hi Ruslan, Are you able to come up with a solution? Otherwise we'll need to revert this.
              Hide
              kabalin 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
              kabalin 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
              poltawski 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
              poltawski 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
              poltawski Dan Poltawski added a comment -

              Passing, based on the new issue.

              Show
              poltawski Dan Poltawski added a comment - Passing, based on the new issue.
              Hide
              poltawski 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
              poltawski 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 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 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:
                    Fix Release Date:
                    12/Nov/12