Moodle
  1. Moodle
  2. MDL-41245

Multiple issues with plugin installing, upgrading, uninstalling and reinstalling

    Details

    • Testing Instructions:
      Hide
      Test 1
      1. Visit your mod directory and type "git clone https://github.com/markn86/moodle-mod_certificate certificate".
      2. Install the module.
      3. Visit the plugins overview page and click to uninstall it.
      4. Once you get to the success page remove the mod/certificate folder and click continue.
      5. You should be directed to admin/index.php.
      6. Follow the prompts to finish the 'upgrade'.
      7. Visit the plugins overview page and ensure there are no errors about a missing string for certificate plugin.
      Test 2
      1. Visit your plugins overview page.
      2. Uninstall 'mod_book' by following the prompts (but do not remove from your directory).
      3. Go back to your plugins overview page and notice how it says 'To be installed' next to 'Book'.
      4. Visit admin/index.php and ensure you can reinstall the plugin.
      Test 3
      1. Upgrade from 2.2.x and 2.5.x and ensure there are no errors.
      Test 4
      1. Perform a fresh install and ensure there are no errors.
      Test 5
      1. On the upgrades site and fresh installed site ensure you can edit blocks on admin/index.php.
      Test 6
      1. Try add-on installer tool.
      Show
      Test 1 Visit your mod directory and type "git clone https://github.com/markn86/moodle-mod_certificate certificate". Install the module. Visit the plugins overview page and click to uninstall it. Once you get to the success page remove the mod/certificate folder and click continue. You should be directed to admin/index.php. Follow the prompts to finish the 'upgrade'. Visit the plugins overview page and ensure there are no errors about a missing string for certificate plugin. Test 2 Visit your plugins overview page. Uninstall 'mod_book' by following the prompts (but do not remove from your directory). Go back to your plugins overview page and notice how it says 'To be installed' next to 'Book'. Visit admin/index.php and ensure you can reinstall the plugin. Test 3 Upgrade from 2.2.x and 2.5.x and ensure there are no errors. Test 4 Perform a fresh install and ensure there are no errors. Test 5 On the upgrades site and fresh installed site ensure you can edit blocks on admin/index.php. Test 6 Try add-on installer tool.
    • Affected Branches:
      MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_26_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      w35_MDL-41245_m26_pluginuninstall
    • Rank:
      52211
    • Sprint:
      BACKEND Sprint 4

      Description

      1. Visit your mod directory and type "git clone https://github.com/markn86/moodle-mod_certificate certificate"
      2. Install the module.
      3. Visit the plugins overview page and click to uninstall it.
      4. Once you get to the success page remove the mod/certificate folder and click continue.
      5. You will be given an error about a missing language string.
      6. Visit admin/index.php and it takes you to the upgrade page with "No plugins require your attention now".
      7. When clicking "Upgrade Moodle database now" it takes you back to the same page.
      1. Visit your plugins overview page.
      2. Uninstall 'qtype_calculatedmulti' by following the prompts until you get to admin/qtypes.php which lists them.
      3. Go back to your plugins overview page and notice how it says 'To be installed' next to 'Calculated multichoice'.
      4. Visit admin/index.php and notice you are not able to reinstall it, even after clearing cache.

        Issue Links

          Activity

          Hide
          Mark Nelson added a comment -

          I updated the testing instructions so that the steps from MDL-41354 are included.

          Show
          Mark Nelson added a comment - I updated the testing instructions so that the steps from MDL-41354 are included.
          Hide
          Mark Nelson added a comment -

          Hi Petr, the only issue I found was that Step 5 in Test 1 still occurs. When I reach the screen that displays "To prevent the plugin re-installing itself, its folder /var/www/mstorage/sm/moodle/mod/certificate must be manually removed from your server now." I then remove the certificate folder and click continue and am given an error about a missing language string (as the folder no longer exists). Going to the admin/index.php page and going through the upgrade step fixes this.

          Show
          Mark Nelson added a comment - Hi Petr, the only issue I found was that Step 5 in Test 1 still occurs. When I reach the screen that displays "To prevent the plugin re-installing itself, its folder /var/www/mstorage/sm/moodle/mod/certificate must be manually removed from your server now." I then remove the certificate folder and click continue and am given an error about a missing language string (as the folder no longer exists). Going to the admin/index.php page and going through the upgrade step fixes this.
          Hide
          Petr Škoda added a comment -

          Hi Mark, I was unable to reproduce the string issue, I think we can resolve it later because it is not a new issue imho.

          To integrators: do not dare to complain about my use of inline /** @var */, the renderers are a perfect example why you need to fix the code checker to allow it. Anyway submitting to integration without peer review after my latest changes because others do it too.

          Show
          Petr Škoda added a comment - Hi Mark, I was unable to reproduce the string issue, I think we can resolve it later because it is not a new issue imho. To integrators: do not dare to complain about my use of inline /** @var */, the renderers are a perfect example why you need to fix the code checker to allow it. Anyway submitting to integration without peer review after my latest changes because others do it too.
          Hide
          Damyon Wiese added a comment -

          Petr - I have been trying to test the plugin installation side of this (with Aparup) and this patch prevents installations from the plugins db. Note: we had to make some temporary changes on the plugins db side to get that far because you can't install directly to a 2.6dev site yet.

          Once that was sorted - the url for the plugin installation request was:

          http://damyon.moodle.local/integration_master/admin/index.php?installaddonrequest=eyJuYW1lIjoiSW50ZXJhY3RpdmUgbW9kZSB3aXRoIGhpbnRpbmciLCJjb21wb25lbnQiOiJxYmVoYXZpb3VyX2ludGVyYWN0aXZlaGludHMiLCJ2ZXJzaW9uIjoiMjAxMzA4MTMwMCJ9

          Your patch however breaks the plugin installation process.

          I think this is the solution but have not had time to test it properly:

          --- a/admin/index.php
          +++ b/admin/index.php
          @@ -88,7 +88,7 @@ $PAGE->set_url($url);
           unset($url);
           
           // Are we returning from an add-on installation request at moodle.org/plugins?
          -if (!$cache and empty($CFG->disableonclickaddoninstall)) {
          +if (!$cache and empty($CFG->disableonclickaddoninstall) and !is_null($newaddonreq)) {
               $target = new moodle_url('/admin/tool/installaddon/index.php', array(
                   'installaddonrequest' => $newaddonreq,
                   'confirm' => 0));
          

          Can you update your patch to address this?

          Aside - integrators will complain about whatever they want to . I consider that syntax to be pandering to a few specific IDEs - but other integrators disagree. MDLSITE-2322. I would integrate this with those comments - but my spine would tingle and my eye would twitch.

          Thanks!

          Show
          Damyon Wiese added a comment - Petr - I have been trying to test the plugin installation side of this (with Aparup) and this patch prevents installations from the plugins db. Note: we had to make some temporary changes on the plugins db side to get that far because you can't install directly to a 2.6dev site yet. Once that was sorted - the url for the plugin installation request was: http://damyon.moodle.local/integration_master/admin/index.php?installaddonrequest=eyJuYW1lIjoiSW50ZXJhY3RpdmUgbW9kZSB3aXRoIGhpbnRpbmciLCJjb21wb25lbnQiOiJxYmVoYXZpb3VyX2ludGVyYWN0aXZlaGludHMiLCJ2ZXJzaW9uIjoiMjAxMzA4MTMwMCJ9 Your patch however breaks the plugin installation process. I think this is the solution but have not had time to test it properly: --- a/admin/index.php +++ b/admin/index.php @@ -88,7 +88,7 @@ $PAGE->set_url($url); unset($url); // Are we returning from an add-on installation request at moodle.org/plugins? - if (!$cache and empty($CFG->disableonclickaddoninstall)) { + if (!$cache and empty($CFG->disableonclickaddoninstall) and !is_null($newaddonreq)) { $target = new moodle_url('/admin/tool/installaddon/index.php', array( 'installaddonrequest' => $newaddonreq, 'confirm' => 0)); Can you update your patch to address this? Aside - integrators will complain about whatever they want to . I consider that syntax to be pandering to a few specific IDEs - but other integrators disagree. MDLSITE-2322 . I would integrate this with those comments - but my spine would tingle and my eye would twitch. Thanks!
          Hide
          Damyon Wiese added a comment -

          More info on the fail...

          The error is:

          There is a request to install an add-on from the Moodle plugins directory on this
          site. Unfortunately the request is not valid and so the add-on cannot be installed.

          And the url is:

          http://damyon.moodle.local/integration_master/admin/tool/installaddon/index.php?installaddonrequest&confirm=0

          Show
          Damyon Wiese added a comment - More info on the fail... The error is: There is a request to install an add-on from the Moodle plugins directory on this site. Unfortunately the request is not valid and so the add-on cannot be installed. And the url is: http://damyon.moodle.local/integration_master/admin/tool/installaddon/index.php?installaddonrequest&confirm=0
          Hide
          Petr Škoda added a comment -

          I just tried it again, grrr, fixed now, I must have broken the if after I did my fist testing.

          There is one interesting issue on moodle.org itself - it redirects back to wrong URL

          1/ I login to my http://mujmacpro.local/moodle26/, click on add thing
          2/ login to moodle.org select plugin
          3/ I get redirected back to different site!
          http://mujmacpro.local/moodle25/admin/index.php?installaddonrequest=eyJuYW1lIjoiTWVyZ2UgdXNlciBhY2NvdW50cyIsImNvbXBvbmVudCI6InJlcG9ydF9tZXJnZXVzZXJzIiwidmVyc2lvbiI6IjIwMTMwNTAxMDAifQ%3D%3D

          There seems to be something very weird in the add-on plugin database...

          Show
          Petr Škoda added a comment - I just tried it again, grrr, fixed now, I must have broken the if after I did my fist testing. There is one interesting issue on moodle.org itself - it redirects back to wrong URL 1/ I login to my http://mujmacpro.local/moodle26/ , click on add thing 2/ login to moodle.org select plugin 3/ I get redirected back to different site! http://mujmacpro.local/moodle25/admin/index.php?installaddonrequest=eyJuYW1lIjoiTWVyZ2UgdXNlciBhY2NvdW50cyIsImNvbXBvbmVudCI6InJlcG9ydF9tZXJnZXVzZXJzIiwidmVyc2lvbiI6IjIwMTMwNTAxMDAifQ%3D%3D There seems to be something very weird in the add-on plugin database...
          Hide
          Damyon Wiese added a comment -

          On the addon thing - it's because you are installing on master and 26 is not in the list of available versions yet (so it will try and install on your valid 25 site instead). We had to fudge it.

          Show
          Damyon Wiese added a comment - On the addon thing - it's because you are installing on master and 26 is not in the list of available versions yet (so it will try and install on your valid 25 site instead). We had to fudge it.
          Hide
          Petr Škoda added a comment -

          I have added one more commit that fixes notices caused by loading of admin tree during plugin uninstallation.

          Wow, why is it storing the list of my sites? I did not allow anybody to store that info and the help next to install button told me it is sending the info for my convenience only...

          Show
          Petr Škoda added a comment - I have added one more commit that fixes notices caused by loading of admin tree during plugin uninstallation. Wow, why is it storing the list of my sites? I did not allow anybody to store that info and the help next to install button told me it is sending the info for my convenience only...
          Hide
          Damyon Wiese added a comment -

          Spine tingling, eye twitching - integrated to master only.

          Thanks Petr - the plugin installation process works fine for me now - and I tested the plugin with dependency workflow too.

          Show
          Damyon Wiese added a comment - Spine tingling, eye twitching - integrated to master only. Thanks Petr - the plugin installation process works fine for me now - and I tested the plugin with dependency workflow too.
          Hide
          Frédéric Massart added a comment -

          Sorry I have to fail this.

          • Test 1: Failed
            1. The redirect to admin/index.php does not occur, I was redirect to the plugin overview
            2. The notice about a missing language string was displayed
            3. When using Memcache, the system segfaults. This is actually happening in the last weekly release, and not specifically when uninstalling a plugin.
          • Test 2: Successful
          • Test 3: Successful
          • Test 4: Successful
          • Test 5: Successful
          • Test 6: Successful, but could only be tested by dropping a zip file

          Cheers,
          Fred

          Show
          Frédéric Massart added a comment - Sorry I have to fail this. Test 1: Failed The redirect to admin/index.php does not occur, I was redirect to the plugin overview The notice about a missing language string was displayed When using Memcache, the system segfaults. This is actually happening in the last weekly release, and not specifically when uninstalling a plugin. Test 2: Successful Test 3: Successful Test 4: Successful Test 5: Successful Test 6: Successful, but could only be tested by dropping a zip file Cheers, Fred
          Hide
          Aparup Banerjee added a comment -

          Hi , i've moved MDL-41182 as a sub-task under this MDL as Petr is resolving within this issue. Please ensure the test in MDL-41182 works as well.

          Show
          Aparup Banerjee added a comment - Hi , i've moved MDL-41182 as a sub-task under this MDL as Petr is resolving within this issue. Please ensure the test in MDL-41182 works as well.
          Hide
          Mark Nelson added a comment -

          Hi Fred, I did point out that issue in my Peer review but Petr said he was unable to replicate it.

          Show
          Mark Nelson added a comment - Hi Fred, I did point out that issue in my Peer review but Petr said he was unable to replicate it.
          Hide
          Aparup Banerjee added a comment -

          on 'Test 6', please refer to MDL-41182's test (which this issue resolves). There is a description here too if you want a short cut to installing add-ons in master (see MDL-38509)

          Show
          Aparup Banerjee added a comment - on 'Test 6', please refer to MDL-41182 's test (which this issue resolves). There is a description here too if you want a short cut to installing add-ons in master (see MDL-38509 )
          Hide
          Petr Škoda added a comment -

          Test 1: certificate uninstalled fine for me without any warning (debug developer with lang cache enabled)

          Show
          Petr Škoda added a comment - Test 1: certificate uninstalled fine for me without any warning (debug developer with lang cache enabled)
          Hide
          Frédéric Massart added a comment -

          As discussed on Jabber, it seems that the problem occurs when you have to manually remove the plugin directory.

          Show
          Frédéric Massart added a comment - As discussed on Jabber, it seems that the problem occurs when you have to manually remove the plugin directory.
          Hide
          Petr Škoda added a comment -

          yay, I have finally managed to reproduce it, I have added one more commit with fix, please merge it, thanks!

          Show
          Petr Škoda added a comment - yay, I have finally managed to reproduce it, I have added one more commit with fix, please merge it, thanks!
          Hide
          Damyon Wiese added a comment -

          Thanks Petr, I've pulled that fix in - back to testing.

          Show
          Damyon Wiese added a comment - Thanks Petr, I've pulled that fix in - back to testing.
          Hide
          Anthony Borrow added a comment -

          Thanks for the good work on making plugin installing and removal more consistent. As for manual plugin directory removal, with themes the user is prompted about whether to remove the directory from the server. I would like to see that option for all plugin types. I like that you cannot remove a plugin if there is an existing dependency.

          On installation, if we can get it so that it will provide an option to download and install all dependencies (listing what those are) then that would be really slick as it would mean folks could find a plugin they like and start to install it. If dependencies exist, the user does not have to go search for them, they can simply opt to install them. For testing, some of the adaptive question types with multiple dependencies may be useful test cases.

          Peace - Anthony

          Show
          Anthony Borrow added a comment - Thanks for the good work on making plugin installing and removal more consistent. As for manual plugin directory removal, with themes the user is prompted about whether to remove the directory from the server. I would like to see that option for all plugin types. I like that you cannot remove a plugin if there is an existing dependency. On installation, if we can get it so that it will provide an option to download and install all dependencies (listing what those are) then that would be really slick as it would mean folks could find a plugin they like and start to install it. If dependencies exist, the user does not have to go search for them, they can simply opt to install them. For testing, some of the adaptive question types with multiple dependencies may be useful test cases. Peace - Anthony
          Hide
          Frédéric Massart added a comment -

          Test 1 now passes. With or without permissions to remove the directory. Cheers!

          Show
          Frédéric Massart added a comment - Test 1 now passes. With or without permissions to remove the directory. Cheers!
          Hide
          Mark Nelson added a comment -

          Hi Anthony, that is a great idea but should be something addressed in a separate issue.

          Show
          Mark Nelson added a comment - Hi Anthony, that is a great idea but should be something addressed in a separate issue.
          Hide
          Damyon Wiese added a comment -

          There was a young man named McGee
          Who thought squashing bugs was easy
          He tried it one day
          And to his dismay
          The bug guts made his keyboard all greasy

          Thanks!

          This has issue has been fixed and released in Moodle.

          Show
          Damyon Wiese added a comment - There was a young man named McGee Who thought squashing bugs was easy He tried it one day And to his dismay The bug guts made his keyboard all greasy Thanks! This has issue has been fixed and released in Moodle.

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Agile