Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-27547

When deleting a local plugin containing a built-in service, the built-in service is not deleted

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0.3, 2.1.3, 2.2.1
    • Fix Version/s: 2.1.6, 2.2.3
    • Component/s: Web Services
    • Labels:
    • Testing Instructions:
      Hide
      • install https://github.com/moodlehq/moodle-local_wstemplate
      • check that the built-in "My service" has been created in the admin
      • add the "Hello world" function to a different service
      • uninstall the plugin from the Admin
      • delete the plugin files
      • check that the pre-built service + Hello World function are not available anymore in the administration
      Show
      install https://github.com/moodlehq/moodle-local_wstemplate check that the built-in "My service" has been created in the admin add the "Hello world" function to a different service uninstall the plugin from the Admin delete the plugin files check that the pre-built service + Hello World function are not available anymore in the administration
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull Master Branch:

      Description

      Check that deleting a plugin built-in service code also does delete this service from the database.

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              jerome Jérôme Mouneyrac created issue -
              jerome Jérôme Mouneyrac made changes -
              Field Original Value New Value
              Priority Minor [ 4 ] Major [ 3 ]
              salvetore Michael de Raadt made changes -
              Fix Version/s STABLE backlog [ 10463 ]
              Testing Instructions - add a built-in service in /lib/db containing some web service function
              - upgrade Moodle
              - check that the built-in service has been created in the admin
              - remove the buil-in service in the code
              - upgrade Moodle
              - check that the built-in service is not listed in the admin (it should have been deleted by the upgrade script)

              Do the same but for a plugin.
              - add a built-in service in /lib/db containing some web service function
              - upgrade Moodle
              - check that the built-in service has been created in the admin
              - remove the built-in service in the code
              - upgrade Moodle
              - check that the built-in service is not listed in the admin (it should have been deleted by the upgrade script)

              Do the same but for a plugin.
              Labels triaged
              jerome Jérôme Mouneyrac made changes -
              Link This issue has a non-specific relationship to MDL-31253 [ MDL-31253 ]
              jerome Jérôme Mouneyrac made changes -
              Pull Master Diff URL https://github.com/mouneyrac/moodle/compare/master...MDL-27547
              Pull Master Branch MDL-27547
              Testing Instructions - add a built-in service in /lib/db containing some web service function
              - upgrade Moodle
              - check that the built-in service has been created in the admin
              - remove the built-in service in the code
              - upgrade Moodle
              - check that the built-in service is not listed in the admin (it should have been deleted by the upgrade script)

              Do the same but for a plugin.
              - install https://github.com/moodlehq/moodle-local_wstemplate
              - check that the built-in "My service" has been created in the admin
              - add the "Hello world" function to a different service
              - uninstall the plugin from the Admin
              - delete the plugin files
              - check that the pre-built service + Hello World function are not available anymore in the administration
              Description Check that deleting a built-in service in core code also does delete this service from the database. (it's probably missing and so it's why it's not even implemented for local plugin) Check that deleting a plugin built-in service code also does delete this service from the database.
              Pull from Repository git://github.com/mouneyrac/moodle.git
              jerome Jérôme Mouneyrac made changes -
              Comment [ actually I just noticed that web service also doesn't get delete for local plugin. I know they do get deleted for core function. Need to be fixed too! ]
              jerome Jérôme Mouneyrac made changes -
              Status Open [ 1 ] Waiting for peer review [ 10012 ]
              Peer reviewer rwijaya
              jerome Jérôme Mouneyrac made changes -
              Affects Version/s 2.2.1 [ 11456 ]
              Affects Version/s 2.1.3 [ 11251 ]
              jerome Jérôme Mouneyrac made changes -
              Priority Major [ 3 ] Minor [ 4 ]
              rwijaya Rossiani Wijaya made changes -
              Original Estimate 0 minutes [ 0 ]
              Remaining Estimate 0 minutes [ 0 ]
              Status Waiting for peer review [ 10012 ] Peer review in progress [ 10013 ]
              Hide
              rwijaya Rossiani Wijaya added a comment -

              Hi Jerome,

              I added custom service that refer to functions from my service and set it with authorize users and token.

              Deleting the plugin didn't delete the custom service data.

              Data from the following tables are still exist:

              1. external_services
              2. external_services_functions
              3. external_services_users
              4. external_tokens

              However, viewing the functions for the custom service from the site page, display no function message.

              I'm sending this issue back to development. I think custom services should also be deleted if the plugin service is removed from the site.

              Show
              rwijaya Rossiani Wijaya added a comment - Hi Jerome, I added custom service that refer to functions from my service and set it with authorize users and token. Deleting the plugin didn't delete the custom service data. Data from the following tables are still exist: external_services external_services_functions external_services_users external_tokens However, viewing the functions for the custom service from the site page, display no function message. I'm sending this issue back to development. I think custom services should also be deleted if the plugin service is removed from the site.
              rwijaya Rossiani Wijaya made changes -
              Status Peer review in progress [ 10013 ] Development in progress [ 3 ]
              Hide
              jerome Jérôme Mouneyrac added a comment - - edited

              I don't know what's wrong, it worked for me. Looking more...
              Functions are removed from all custom services. In my opinion these custom services should not be deleted as they could contain available functions.

              Did you delete the plugin by the administration?

              Show
              jerome Jérôme Mouneyrac added a comment - - edited I don't know what's wrong, it worked for me. Looking more... Functions are removed from all custom services. In my opinion these custom services should not be deleted as they could contain available functions. Did you delete the plugin by the administration?
              rwijaya Rossiani Wijaya made changes -
              Status Development in progress [ 3 ] Peer review in progress [ 10013 ]
              Hide
              rwijaya Rossiani Wijaya added a comment -

              Thank you Jerome from explaining. I agree with you, custom service shouldn't be deleted, just the local plugin function should be deleted.

              I re-tested this again, delete the plugin through admin and noticed that local_wstemplate_hello_world function still exist in external_services_functions table. It doesn't show on the page, just in DB.

              let me know if you can't reproduce this.

              Show
              rwijaya Rossiani Wijaya added a comment - Thank you Jerome from explaining. I agree with you, custom service shouldn't be deleted, just the local plugin function should be deleted. I re-tested this again, delete the plugin through admin and noticed that local_wstemplate_hello_world function still exist in external_services_functions table. It doesn't show on the page, just in DB. let me know if you can't reproduce this.
              rwijaya Rossiani Wijaya made changes -
              Status Peer review in progress [ 10013 ] Development in progress [ 3 ]
              Hide
              jerome Jérôme Mouneyrac added a comment -

              Thanks Rosie, good catch. Tested on 2.1, 2.2 and 2.3 it works fine.

              Show
              jerome Jérôme Mouneyrac added a comment - Thanks Rosie, good catch. Tested on 2.1, 2.2 and 2.3 it works fine.
              jerome Jérôme Mouneyrac made changes -
              Status Development in progress [ 3 ] Waiting for integration review [ 10010 ]
              Hide
              skodak Petr Skoda added a comment -

              to integrators: this has to work even without the php files of the plugin that defined the service, if not reject it

              Show
              skodak Petr Skoda added a comment - to integrators: this has to work even without the php files of the plugin that defined the service, if not reject it
              Hide
              stronk7 Eloy Lafuente (stronk7) 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
              stronk7 Eloy Lafuente (stronk7) 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
              samhemelryk Sam Hemelryk made changes -
              Currently in integration Yes [ 10041 ]
              samhemelryk Sam Hemelryk made changes -
              Status Waiting for integration review [ 10010 ] Integration review in progress [ 10004 ]
              Integrator samhemelryk
              Hide
              samhemelryk Sam Hemelryk added a comment -

              Hi Jerome,

              Code looks fine, just noting Petr's comments I don't think the check for the file's existence is required.
              Certainly those queries could be run for all components regardless of the existence of a services file, the advantage of which would be that it would clean up anything that was created through other code, or was left over after previous operations.
              I'll leave this in review in progress to give you a chance to do this.

              Cheers
              Sam

              Show
              samhemelryk Sam Hemelryk added a comment - Hi Jerome, Code looks fine, just noting Petr's comments I don't think the check for the file's existence is required. Certainly those queries could be run for all components regardless of the existence of a services file, the advantage of which would be that it would clean up anything that was created through other code, or was left over after previous operations. I'll leave this in review in progress to give you a chance to do this. Cheers Sam
              Hide
              jerome Jérôme Mouneyrac added a comment - - edited

              Hi Sam,
              thanks for reviewing. Moodle upgrade system cleans up everything, I don't think we need to try to clean other components when uninstalling one plugin. However it seems to me that there is another issue. When the file service.php file exists, and one of its services has been removed, then we forgot to delete the tokens related to the removed service.

              lib/upgradlib.php:

              // now deal with services
                  $dbservices = $DB->get_records('external_services', array('component'=>$component));
                  foreach ($dbservices as $dbservice) {
                      if (empty($services[$dbservice->name])) {
                          $DB->delete_records('external_services_functions', array('externalserviceid'=>$dbservice->id));
                          $DB->delete_records('external_services_users', array('externalserviceid'=>$dbservice->id));
                          $DB->delete_records('external_services', array('id'=>$dbservice->id));
                          continue;
                      }

              should add:

              $DB->delete_records('external_tokens',array('externalserviceid'=>$dbservice->id));

              You can return the issue to me, I'll fix that too. Cheers.

              Show
              jerome Jérôme Mouneyrac added a comment - - edited Hi Sam, thanks for reviewing. Moodle upgrade system cleans up everything, I don't think we need to try to clean other components when uninstalling one plugin. However it seems to me that there is another issue. When the file service.php file exists, and one of its services has been removed, then we forgot to delete the tokens related to the removed service. lib/upgradlib.php: // now deal with services $dbservices = $DB->get_records('external_services', array('component'=>$component)); foreach ($dbservices as $dbservice) { if (empty($services[$dbservice->name])) { $DB->delete_records('external_services_functions', array('externalserviceid'=>$dbservice->id)); $DB->delete_records('external_services_users', array('externalserviceid'=>$dbservice->id)); $DB->delete_records('external_services', array('id'=>$dbservice->id)); continue; } should add: $DB->delete_records('external_tokens',array('externalserviceid'=>$dbservice->id)); You can return the issue to me, I'll fix that too. Cheers.
              Hide
              jerome Jérôme Mouneyrac added a comment -

              done

              Show
              jerome Jérôme Mouneyrac added a comment - done
              Hide
              samhemelryk Sam Hemelryk added a comment -

              Hi Jerome,

              Thanks for cleaning up the tokens!

              However I've been having a chat with Eloy this morning about check for services.php and we both agree that the check shouldn't be there.
              The queries should be executed regardless of the existence of a services.php file.
              This ensure that when a component is removed any orphan service related records will be cleaned up if for some reason the user has ended up with some.
              Executing the queries when there is no servcies.php or services + friends in the DB should be fine.

              Sending this back so that it can be changed/discussed further.

              Cheers
              Sam

              Show
              samhemelryk Sam Hemelryk added a comment - Hi Jerome, Thanks for cleaning up the tokens! However I've been having a chat with Eloy this morning about check for services.php and we both agree that the check shouldn't be there. The queries should be executed regardless of the existence of a services.php file. This ensure that when a component is removed any orphan service related records will be cleaned up if for some reason the user has ended up with some. Executing the queries when there is no servcies.php or services + friends in the DB should be fine. Sending this back so that it can be changed/discussed further. Cheers Sam
              samhemelryk Sam Hemelryk made changes -
              Status Integration review in progress [ 10004 ] Reopened [ 4 ]
              stronk7 Eloy Lafuente (stronk7) made changes -
              Currently in integration Yes [ 10041 ]
              Hide
              jerome Jérôme Mouneyrac added a comment - - edited

              Hi guys,
              are you saying that we should support the case when the moodle admin deletes manually the services.php file then (s)he presses uninstall? It seems to me it's improbable, and more over on upgrade Moodle would clean the result of this bad admin behavior. That seems adding unnecessary complexity.

              Let me know if you agree, otherwise I'll do more change to please you

              Show
              jerome Jérôme Mouneyrac added a comment - - edited Hi guys, are you saying that we should support the case when the moodle admin deletes manually the services.php file then (s)he presses uninstall? It seems to me it's improbable, and more over on upgrade Moodle would clean the result of this bad admin behavior. That seems adding unnecessary complexity. Let me know if you agree, otherwise I'll do more change to please you
              Hide
              jerome Jérôme Mouneyrac added a comment -

              I submit for integration. Reject it if you don't agree.

              Show
              jerome Jérôme Mouneyrac added a comment - I submit for integration. Reject it if you don't agree.
              jerome Jérôme Mouneyrac made changes -
              Status Reopened [ 4 ] Waiting for integration review [ 10010 ]
              Hide
              stronk7 Eloy Lafuente (stronk7) 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
              stronk7 Eloy Lafuente (stronk7) 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
              skodak Petr Skoda added a comment - - edited

              to integrators: it is a standard procedure to cleanup up as much as necessary when deleting plugin that is gone - such as after upgrade to higher version when plugin was not updated to be compatible and borks any upgrade attempt, otherwise you would not be able to nuke unmaintained 1.9.x contrib from upgraded site. The rules are simple: Moodle must work without source files of any previously installed non-core plugin, it should be possible to upgrade the plugin later when the source code is updated. Deleting removes as much as possible, no exceptions for any subsystems - calling cleanup callbacks first, then purging as much as possible making sure nothing was accidentally left.

              Show
              skodak Petr Skoda added a comment - - edited to integrators: it is a standard procedure to cleanup up as much as necessary when deleting plugin that is gone - such as after upgrade to higher version when plugin was not updated to be compatible and borks any upgrade attempt, otherwise you would not be able to nuke unmaintained 1.9.x contrib from upgraded site. The rules are simple: Moodle must work without source files of any previously installed non-core plugin, it should be possible to upgrade the plugin later when the source code is updated. Deleting removes as much as possible, no exceptions for any subsystems - calling cleanup callbacks first, then purging as much as possible making sure nothing was accidentally left.
              samhemelryk Sam Hemelryk made changes -
              Currently in integration Yes [ 10041 ]
              samhemelryk Sam Hemelryk made changes -
              Status Waiting for integration review [ 10010 ] Integration review in progress [ 10004 ]
              Hide
              samhemelryk Sam Hemelryk added a comment -

              Hi Jerome,

              Petr summed it up pretty well in his comment there.
              While it may be unlikely that someone will wind up with services for a plugin without having services.php file it is still a possibility.
              I certainly think it is worth removing the if (https://github.com/mouneyrac/moodle/compare/master...MDL-27547#L0R274) to help us minimise future problems, and help us ensure a smooth installation/upgrade process for the future.
              Essentially by executing the deletion process regardless of the presence of that file we are cleaning up any widowed information as well.

              Otherwise spot on.

              Cheers
              Sam

              Show
              samhemelryk Sam Hemelryk added a comment - Hi Jerome, Petr summed it up pretty well in his comment there. While it may be unlikely that someone will wind up with services for a plugin without having services.php file it is still a possibility. I certainly think it is worth removing the if ( https://github.com/mouneyrac/moodle/compare/master...MDL-27547#L0R274 ) to help us minimise future problems, and help us ensure a smooth installation/upgrade process for the future. Essentially by executing the deletion process regardless of the presence of that file we are cleaning up any widowed information as well. Otherwise spot on. Cheers Sam
              samhemelryk Sam Hemelryk made changes -
              Status Integration review in progress [ 10004 ] Reopened [ 4 ]
              stronk7 Eloy Lafuente (stronk7) made changes -
              Currently in integration Yes [ 10041 ]
              Hide
              jerome Jérôme Mouneyrac added a comment -

              I think I misunderstood what you meant. You wanted to clean the web services from the removed plugin not from every other plugins. I changed:

                  //delete the web service functions and pre-built services
                  $servicesfile = $plugindirectory . '/db/services.php';
                  if (file_exists($servicesfile)) {
                      require_once($CFG->dirroot.'/lib/externallib.php');
                      external_delete_descriptions($component);	
                  }

              to

              //delete the web service functions and pre-built services
               	require_once($CFG->dirroot.'/lib/externallib.php');
               	external_delete_descriptions($component);

              the uninstall process is now cleaner.

              Show
              jerome Jérôme Mouneyrac added a comment - I think I misunderstood what you meant. You wanted to clean the web services from the removed plugin not from every other plugins. I changed: //delete the web service functions and pre-built services $servicesfile = $plugindirectory . '/db/services.php'; if (file_exists($servicesfile)) { require_once($CFG->dirroot.'/lib/externallib.php'); external_delete_descriptions($component); } to //delete the web service functions and pre-built services require_once($CFG->dirroot.'/lib/externallib.php'); external_delete_descriptions($component); the uninstall process is now cleaner.
              jerome Jérôme Mouneyrac made changes -
              Status Reopened [ 4 ] Waiting for integration review [ 10010 ]
              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
              samhemelryk Sam Hemelryk made changes -
              Currently in integration Yes [ 10041 ]
              samhemelryk Sam Hemelryk made changes -
              Status Waiting for integration review [ 10010 ] Integration review in progress [ 10004 ]
              Hide
              samhemelryk Sam Hemelryk added a comment -

              Thanks Jerome, changes look spot on now

              Show
              samhemelryk Sam Hemelryk added a comment - Thanks Jerome, changes look spot on now
              samhemelryk Sam Hemelryk made changes -
              Status Integration review in progress [ 10004 ] Waiting for testing [ 10005 ]
              Fix Version/s 2.1.6 [ 12052 ]
              Fix Version/s 2.2.3 [ 12053 ]
              Fix Version/s STABLE backlog [ 10463 ]
              salvetore Michael de Raadt made changes -
              Tester andyjdavis
              andyjdavis Andrew Davis made changes -
              Status Waiting for testing [ 10005 ] Testing in progress [ 10011 ]
              Hide
              andyjdavis Andrew Davis added a comment -

              Works as described.

              Show
              andyjdavis Andrew Davis added a comment - Works as described.
              andyjdavis Andrew Davis made changes -
              Status Testing in progress [ 10011 ] Tested [ 10006 ]
              Hide
              poltawski Dan Poltawski added a comment -

              Bonza mate!

              Your changes have made it into the Moodle release! Its time to celebrate! Put a shrimp on the barbie and grab a stubby.

              Hooroo

              Show
              poltawski Dan Poltawski added a comment - Bonza mate! Your changes have made it into the Moodle release! Its time to celebrate! Put a shrimp on the barbie and grab a stubby. Hooroo
              poltawski Dan Poltawski made changes -
              Status Tested [ 10006 ] Closed [ 6 ]
              Resolution Fixed [ 1 ]
              Currently in integration Yes [ 10041 ]
              poltawski Dan Poltawski made changes -
              Integration date 19/Apr/12

                People

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

                  Dates

                  • Created:
                    Updated:
                    Resolved:
                    Fix Release Date:
                    14/May/12