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 Bug
    • Status: Closed
    • Priority: Minor 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:
    • Rank:
      17208

      Description

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

        Issue Links

          Activity

          Hide
          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
          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.
          Hide
          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
          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?
          Hide
          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
          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.
          Hide
          Jérôme Mouneyrac added a comment -

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

          Show
          Jérôme Mouneyrac added a comment - Thanks Rosie, good catch. Tested on 2.1, 2.2 and 2.3 it works fine.
          Hide
          Petr Škoda 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
          Petr Škoda 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
          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
          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
          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
          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
          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
          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
          Jérôme Mouneyrac added a comment -

          done

          Show
          Jérôme Mouneyrac added a comment - done
          Hide
          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
          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
          Hide
          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
          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
          Jérôme Mouneyrac added a comment -

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

          Show
          Jérôme Mouneyrac added a comment - I submit for integration. Reject it if you don't agree.
          Hide
          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
          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
          Petr Škoda 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
          Petr Škoda 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.
          Hide
          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
          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
          Hide
          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
          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.
          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
          Sam Hemelryk added a comment -

          Thanks Jerome, changes look spot on now

          Show
          Sam Hemelryk added a comment - Thanks Jerome, changes look spot on now
          Hide
          Andrew Davis added a comment -

          Works as described.

          Show
          Andrew Davis added a comment - Works as described.
          Hide
          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
          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

            People

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

              Dates

              • Created:
                Updated:
                Resolved: