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
    • Rank:
      44129

      Description

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

        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: