From 0b733dd9e269d0daf46ec3df2aa8071272fb2c59 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Mudr=C3=A1k?= Date: Wed, 10 Apr 2013 11:31:50 +0200 Subject: [PATCH 01/14] MDL-39087 Clarify plugininfo_base::get_uninstall_url() return value The method now returns null if there should be no 'Uninstall' link at the Plugins management screen. For non-standard add-ons the method now returns URL to a general uninstall tool. Plugin info subclasses can still override the method to provide URL to their own UI for uninstalling. If the plugin type wants to use the general uninstall tool also for standard plugins, it should override this method and explicitly return $this->get_default_uninstall_url(). Otherwise, the 'Uninstall' link will be provided for add-ons only. --- lib/pluginlib.php | 42 ++++++++++++++++++++++++++++++++++++------ 1 file changed, 36 insertions(+), 6 deletions(-) diff --git a/lib/pluginlib.php b/lib/pluginlib.php index de23503162f..40ea274758c 100644 --- a/lib/pluginlib.php +++ b/lib/pluginlib.php @@ -2501,14 +2501,28 @@ abstract class plugininfo_base { * * Visiting that URL must be safe, that is a manual confirmation is needed * for actual uninstallation of the plugin. Null value means that the - * plugin either does not support uninstallation, or does not require any - * database cleanup or the location of the screen is not available via this - * library. + * plugin cannot be uninstalled (such as due to dependencies), or it does + * not support uninstallation, or the location of the screen is not + * available (shortly, the 'Uninstall' link should not be displayed). + * + * By default, URL to a common uninstalling handler is returned for all + * add-ons and null is returned for standard plugins. * * @return null|moodle_url */ public function get_uninstall_url() { - return null; + + if ($this->is_standard()) { + return null; + } + + $pluginman = plugin_manager::instance(); + $requiredby = $pluginman->other_plugins_that_require($this->component); + if (!empty($requiredby)) { + return null; + } + + return $this->get_default_uninstall_url(); } /** @@ -2522,6 +2536,22 @@ abstract class plugininfo_base { return substr($this->rootdir, strlen($CFG->dirroot)); } + /** + * Returns URL to a script that handles common plugin uninstall procedure. + * + * This URL is suitable for plugins that do not have their own UI + * for uninstalling. + * + * @return moodle_url + */ + protected function get_default_uninstall_url() { + return new moodle_url('/admin/plugins.php', array( + 'sesskey' => sesskey(), + 'uninstall' => $this->component, + 'confirm' => 0, + )); + } + /** * Provides access to plugin versions from the {config_plugins} table * @@ -3233,7 +3263,7 @@ class plugininfo_message extends plugininfo_base { if (isset($processors[$this->name])) { return new moodle_url('/admin/message.php', array('uninstall' => $processors[$this->name]->id, 'sesskey' => sesskey())); } else { - return parent::get_uninstall_url(); + return null; } } } @@ -3573,6 +3603,6 @@ class plugininfo_format extends plugininfo_base { return new moodle_url('/admin/courseformats.php', array('sesskey' => sesskey(), 'action' => 'uninstall', 'format' => $this->name)); } - return parent::get_uninstall_url(); + return null; } } From 436d94478d4959cb9fcd2b16e462d7b85dbd1b75 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Mudr=C3=A1k?= Date: Wed, 10 Apr 2013 11:45:12 +0200 Subject: [PATCH 02/14] MDL-39087 Implement a common interface for uninstalling general plugin Plugins may use this general tool for uninstallation and eventually removal of the deployed source code. At the moment, this is implemented as a wrapper for the core function uninstall_plugin() with an extra hook in the relevant plugin info subclass. For non-standard add-ons, the tool can remove the deployed plugin source code as well, if the web server has required write permissions. Ideally, all add-ons installed via the new tool_installaddon should be removable via the web interface as well. --- admin/plugins.php | 83 +++++++++++++++++++- admin/renderer.php | 95 ++++++++++++++++++++++ lang/en/plugin.php | 5 ++ lib/pluginlib.php | 157 +++++++++++++++++++++++++++++++++++++ theme/base/style/admin.css | 2 + 5 files changed, 340 insertions(+), 2 deletions(-) diff --git a/admin/plugins.php b/admin/plugins.php index c6738dfb2f7..35c882df511 100644 --- a/admin/plugins.php +++ b/admin/plugins.php @@ -27,6 +27,7 @@ require_once(dirname(dirname(__FILE__)) . '/config.php'); require_once($CFG->libdir . '/adminlib.php'); require_once($CFG->libdir . '/pluginlib.php'); +require_once($CFG->libdir . '/filelib.php'); admin_externalpage_setup('pluginsoverview'); require_capability('moodle/site:config', context_system::instance()); @@ -34,8 +35,88 @@ require_capability('moodle/site:config', context_system::instance()); $fetchremote = optional_param('fetchremote', false, PARAM_BOOL); $updatesonly = optional_param('updatesonly', false, PARAM_BOOL); $contribonly = optional_param('contribonly', false, PARAM_BOOL); +$uninstall = optional_param('uninstall', '', PARAM_COMPONENT); +$delete = optional_param('delete', '', PARAM_COMPONENT); +$confirmed = optional_param('confirm', false, PARAM_BOOL); + +$output = $PAGE->get_renderer('core', 'admin'); $pluginman = plugin_manager::instance(); + +if ($uninstall) { + require_sesskey(); + $pluginfo = $pluginman->get_plugin_info($uninstall); + + if (is_null($pluginfo)) { + throw new moodle_exception('err_uninstalling_unknown_plugin', 'core_plugin', '', array('plugin' => $uninstall), + 'plugin_manager::get_plugin_info() returned null for the plugin to be uninstalled'); + } + + $requiredby = $pluginman->other_plugins_that_require($pluginfo->component); + if (!empty($requiredby)) { + throw new moodle_exception('err_uninstalling_required_plugin', 'core_plugin', '', + array('plugin' => $pluginfo->component, 'requiredby' => implode(', ', $requiredby)), + 'plugin_manager::other_plugins_that_require() returned non-empty array'); + } + + if (!$confirmed) { + $continueurl = new moodle_url($PAGE->url, array('uninstall' => $pluginfo->component, 'sesskey' => sesskey(), 'confirm' => 1)); + echo $output->plugin_uninstall_confirm_page($pluginman, $pluginfo, $continueurl); + exit(); + + } else { + $messages = array(); // Collect uninstall process messages here. + $pluginman->uninstall_plugin($pluginfo->component, $messages); + + if ($pluginman->is_plugin_folder_removable($pluginfo->component)) { + $continueurl = new moodle_url($PAGE->url, array('delete' => $pluginfo->component, 'sesskey' => sesskey(), 'confirm' => 1)); + echo $output->plugin_uninstall_results_removable_page($pluginman, $pluginfo, $messages, $continueurl); + exit(); + + } else { + echo $output->plugin_uninstall_results_page($pluginman, $pluginfo, $messages); + exit(); + } + } +} + +if ($delete and $confirmed) { + require_sesskey(); + $pluginfo = $pluginman->get_plugin_info($delete); + + // Make sure we know the plugin. + if (is_null($pluginfo)) { + throw new moodle_exception('err_removing_unknown_plugin', 'core_plugin', '', array('plugin' => $delete), + 'plugin_manager::get_plugin_info() returned null for the plugin to be deleted'); + } + + // Make sure it is not installed. + if (!is_null($pluginfo->versiondb)) { + throw new moodle_exception('err_removing_installed_plugin', 'core_plugin', '', + array('plugin' => $pluginfo->component, 'versiondb' => $pluginfo->versiondb), + 'plugin_manager::get_plugin_info() returned not-null versiondb for the plugin to be deleted'); + } + + // Make sure the folder is removable. + if (!$pluginman->is_plugin_folder_removable($pluginfo->component)) { + throw new moodle_exception('err_removing_unremovable_folder', 'core_plugin', '', + array('plugin' => $pluginfo->component, 'rootdir' => $pluginfo->rootdir), + 'plugin root folder is not removable as expected'); + } + + // Make sure the folder is within Moodle installation tree. + if (strpos($pluginfo->rootdir, $CFG->dirroot) !== 0) { + throw new moodle_exception('err_unexpected_plugin_rootdir', 'core_plugin', '', + array('plugin' => $pluginfo->component, 'rootdir' => $pluginfo->rootdir, 'dirroot' => $CFG->dirroot), + 'plugin root folder not in the moodle dirroot'); + } + + // So long, and thanks for all the bugs. + fulldelete($pluginfo->rootdir); + cache::make('core', 'pluginlist')->purge(); + redirect($PAGE->url); +} + $checker = available_update_checker::instance(); // Filtering options. @@ -50,8 +131,6 @@ if ($fetchremote) { redirect(new moodle_url($PAGE->url, $options)); } -$output = $PAGE->get_renderer('core', 'admin'); - $deployer = available_update_deployer::instance(); if ($deployer->enabled()) { $myurl = new moodle_url($PAGE->url, array('updatesonly' => $updatesonly, 'contribonly' => $contribonly)); diff --git a/admin/renderer.php b/admin/renderer.php index b39976d2220..1784de95de2 100644 --- a/admin/renderer.php +++ b/admin/renderer.php @@ -371,6 +371,101 @@ class core_admin_renderer extends plugin_renderer_base { return $output; } + /** + * Display a page to confirm the plugin uninstallation. + * + * @param plugin_manager $pluginman + * @param plugin_info $pluginfo + * @param moodle_url $continueurl URL to continue after confirmation + * @return string + */ + public function plugin_uninstall_confirm_page(plugin_manager $pluginman, plugininfo_base $pluginfo, moodle_url $continueurl) { + $output = ''; + + $pluginname = $pluginman->plugin_name($pluginfo->component); + + $this->page->set_title($pluginname); + $this->page->navbar->add(get_string('uninstalling', 'core_plugin', array('name' => $pluginname))); + + $output .= $this->output->header(); + $output .= $this->output->heading(get_string('uninstalling', 'core_plugin', array('name' => $pluginname))); + $output .= $this->output->confirm(get_string('uninstallconfirm', 'core_plugin', array('name' => $pluginname)), + $continueurl, $this->page->url); + $output .= $this->output->footer(); + + return $output; + } + + /** + * Display a page with results of plugin uninstallation and offer removal of plugin files. + * + * @param plugin_manager $pluginman + * @param plugin_info $pluginfo + * @param array $messages list of strings, the log of the process + * @param moodle_url $continueurl URL to continue to remove the plugin folder + * @return string + */ + public function plugin_uninstall_results_removable_page(plugin_manager $pluginman, plugininfo_base $pluginfo, + array $messages = array(), moodle_url $continueurl) { + $output = ''; + + $pluginname = $pluginman->plugin_name($pluginfo->component); + + $this->page->set_title($pluginname); + $this->page->navbar->add(get_string('uninstalling', 'core_plugin', array('name' => $pluginname))); + + $output .= $this->output->header(); + $output .= $this->output->heading(get_string('uninstalling', 'core_plugin', array('name' => $pluginname))); + + foreach ($messages as $message) { + $output .= $this->output->box($message, 'generalbox uninstallresultmessage'); + } + + $confirm = $this->output->container(get_string('uninstalldeleteconfirm', 'core_plugin', + array('name' => $pluginname, 'rootdir' => $pluginfo->rootdir)), 'uninstalldeleteconfirm'); + + if ($repotype = $pluginman->plugin_external_source($pluginfo->component)) { + $confirm .= $this->output->container(get_string('uninstalldeleteconfirmexternal', 'core_plugin', $repotype), + 'uninstalldeleteconfirmexternal'); + } + + $output .= $this->output->confirm($confirm, $continueurl, $this->page->url); + $output .= $this->output->footer(); + + return $output; + } + + /** + * Display a page with results of plugin uninstallation and inform about the need to remove plugin files manually. + * + * @param plugin_manager $pluginman + * @param plugin_info $pluginfo + * @param array $messages list of strings, the log of the process + * @return string + */ + public function plugin_uninstall_results_page(plugin_manager $pluginman, plugininfo_base $pluginfo, array $messages = array()) { + $output = ''; + + $pluginname = $pluginman->plugin_name($pluginfo->component); + + $this->page->set_title($pluginname); + $this->page->navbar->add(get_string('uninstalling', 'core_plugin', array('name' => $pluginname))); + + $output .= $this->output->header(); + $output .= $this->output->heading(get_string('uninstalling', 'core_plugin', array('name' => $pluginname))); + + foreach ($messages as $message) { + $output .= $this->output->box($message, 'generalbox uninstallresultmessage'); + } + + $output .= $this->output->box(get_string('uninstalldelete', 'core_plugin', + array('name' => $pluginname, 'rootdir' => $pluginfo->rootdir)), 'generalbox uninstalldelete'); + $output .= $this->output->continue_button($this->page->url); + $output .= $this->output->footer(); + + return $output; + } + /** * Display the plugin management page (admin/environment.php). * @param array $versions diff --git a/lang/en/plugin.php b/lang/en/plugin.php index 18c5b7f5d24..69754cd022d 100644 --- a/lang/en/plugin.php +++ b/lang/en/plugin.php @@ -147,6 +147,11 @@ $string['updatepluginconfirminfo'] = 'You are about to install a new version of $string['updatepluginconfirmexternal'] = 'It appears that the current version of the plugin has been obtained via source code management system ({$a}) checkout. If you install this update, you will no longer be able to obtain plugin updates from the source code management system. Please ensure that you definitely want to update the plugin before continuing.'; $string['updatepluginconfirmwarning'] = 'Please note that Moodle will not automatically make a backup of your database before the upgrade. We strongly recommend that you make a full snapshot backup now, to cope with the rare case that the new code has bugs that make your site unavailable or even corrupts your database. Proceed at your own risk.'; $string['uninstall'] = 'Uninstall'; +$string['uninstallconfirm'] = 'You are about to uninstall the plugin {$a->name}. This will completely delete everything in the database associated with this plugin, including its configuration, log records, user files managed by the plugin etc. There is no way back and Moodle itself does not create any recovery backup. Are you SURE you want to continue?'; +$string['uninstalldelete'] = 'All data associated with the plugin {$a->name} has been deleted from the database. To prevent the plugin re-installing itself, its folder {$a->rootdir} must be manually removed from your server now. Moodle itself cannot remove the folder due to write permissions.'; +$string['uninstalldeleteconfirm'] = 'All data associated with the plugin {$a->name} has been deleted from the database. To prevent the plugin re-installing itself, its folder {$a->rootdir} must be removed from your server. Do you want to remove the plugin folder now?'; +$string['uninstalldeleteconfirmexternal'] = 'It appears that the current version of the plugin has been obtained via source code management system ({$a}) checkout. If you remove the plugin folder, you may loose important local modifications of the code. Please ensure that you definitely want to remove the plugin folder before continuing.'; +$string['uninstalling'] = 'Uninstalling {$a->name}'; $string['version'] = 'Version'; $string['versiondb'] = 'Current version'; $string['versiondisk'] = 'New version'; diff --git a/lib/pluginlib.php b/lib/pluginlib.php index 40ea274758c..69557444bcc 100644 --- a/lib/pluginlib.php +++ b/lib/pluginlib.php @@ -301,6 +301,38 @@ class plugin_manager { } } + /** + * Check to see if the current version of the plugin seems to be a checkout of an external repository. + * + * @see available_update_deployer::plugin_external_source() + * @param string $component frankenstyle component name + * @return false|string + */ + public function plugin_external_source($component) { + + $plugininfo = $this->get_plugin_info($component); + + if (is_null($plugininfo)) { + return false; + } + + $pluginroot = $plugininfo->rootdir; + + if (is_dir($pluginroot.'/.git')) { + return 'git'; + } + + if (is_dir($pluginroot.'/CVS')) { + return 'cvs'; + } + + if (is_dir($pluginroot.'/.svn')) { + return 'svn'; + } + + return false; + } + /** * Get a list of any other plugins that require this one. * @param string $component frankenstyle component name. @@ -371,6 +403,42 @@ class plugin_manager { return $return; } + /** + * Uninstall the given plugin. + * + * Automatically cleans-up all remaining configuration data, log records, events, + * files from the file pool etc. + * + * In the future, the functionality of {@link uninstall_plugin()} function may be moved + * into this method and all the code should be refactored to use it. At the moment, we + * mimic this future behaviour by wrapping that function call. + * + * @param string $component + * @param array $messages log of the process is returned via this array + * @return bool true on success, false on errors/problems + */ + public function uninstall_plugin($component, array &$messages) { + + $pluginfo = $this->get_plugin_info($component); + + if (is_null($pluginfo)) { + return false; + } + + // Give the pluginfo class a perform some steps. + $result = $pluginfo->uninstall($messages); + if (!$result) { + return false; + } + + // Call the legacy core function to uninstall the plugin. + ob_start(); + uninstall_plugin($pluginfo->type, $pluginfo->name); + $messages[] = ob_get_clean(); + + return true; + } + /** * Checks if there are some plugins with a known available update * @@ -388,6 +456,36 @@ class plugin_manager { return false; } + /** + * Check to see if the given plugin folder can be removed by the web server process. + * + * This is intended to be used for installed add-ons mainly. For standard plugins, + * false is always returned for now. + * + * @param string $component full frankenstyle component + * @return bool + */ + public function is_plugin_folder_removable($component) { + + $pluginfo = $this->get_plugin_info($component); + + if (is_null($pluginfo)) { + return false; + } + + if ($pluginfo->is_standard()) { + return false; + } + + // To be able to remove the plugin folder, its parent must be writable, too. + if (!is_writable(dirname($pluginfo->rootdir))) { + return false; + } + + // Check that the folder and all its content is writable (thence removable). + return $this->is_directory_removable($pluginfo->rootdir); + } + /** * Defines a list of all plugins that were originally shipped in the standard Moodle distribution, * but are not anymore and are deleted during upgrades. @@ -670,6 +768,50 @@ class plugin_manager { } return $fix; } + + /** + * Check if the given directory can be removed by the web server process. + * + * This recursively checks that the given directory and all its contents + * it writable. + * + * @param string $fullpath + * @return boolean + */ + protected function is_directory_removable($fullpath) { + + if (!is_writable($fullpath)) { + return false; + } + + if (is_dir($fullpath)) { + $handle = opendir($fullpath); + } else { + return false; + } + + $result = true; + + while ($filename = readdir($handle)) { + + if ($filename === '.' or $filename === '..') { + continue; + } + + $subfilepath = $fullpath.'/'.$filename; + + if (is_dir($subfilepath)) { + $result = $result && $this->is_directory_removable($subfilepath); + + } else { + $result = $result && is_writable($subfilepath); + } + } + + closedir($handle); + + return $result; + } } @@ -1662,6 +1804,7 @@ class available_update_deployer { /** * Check to see if the current version of the plugin seems to be a checkout of an external repository. * + * @see plugin_manager::plugin_external_source() * @param available_update_info $info * @return false|string */ @@ -2536,6 +2679,20 @@ abstract class plugininfo_base { return substr($this->rootdir, strlen($CFG->dirroot)); } + /** + * Hook method to implement certain steps when uninstalling the plugin. + * + * This hook is called by {@link plugin_manager::uninstall_plugin()} so + * it is basically usable only for those plugin types that use the default + * uninstall tool provided by {@link self::get_default_uninstall_url()}. + * + * @param array $messages list of uninstall log messages + * @return bool true on success, false on failure + */ + public function uninstall(array &$messages) { + return true; + } + /** * Returns URL to a script that handles common plugin uninstall procedure. * diff --git a/theme/base/style/admin.css b/theme/base/style/admin.css index a43b6e5d42a..e4b3adf583f 100644 --- a/theme/base/style/admin.css +++ b/theme/base/style/admin.css @@ -152,6 +152,8 @@ #page-admin-index .updateplugin .updatepluginconfirmexternal, #page-admin-plugins .updateplugin .updatepluginconfirmexternal {padding:1em;background-color:#ffd3d9;border:1px solid #EEAAAA} +#page-admin-plugins .uninstalldeleteconfirmexternal {margin:1em auto;padding:1em;background-color:#ffd3d9;border:1px solid #EEAAAA} + #page-admin-user-user_bulk #users .fgroup {white-space: nowrap;} #page-admin-report-stats-index .graph {text-align: center;margin-bottom: 1em;} #page-admin-report-courseoverview-index .graph {text-align: center;margin-bottom: 1em;} From 5718a123132c4525a2c0de84c0a41ac723dd1064 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Mudr=C3=A1k?= Date: Wed, 10 Apr 2013 11:53:21 +0200 Subject: [PATCH 03/14] MDL-39087 Purge all caches at the end of uninstall_plugin() This is necessary now as many plugins management related features started to use MUC intensively recently. During the development of this issue, we realized that the plugin was still considered as installed if caches were not purged. --- lib/adminlib.php | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/adminlib.php b/lib/adminlib.php index 3156374c2df..89fa8e8f9d4 100644 --- a/lib/adminlib.php +++ b/lib/adminlib.php @@ -321,6 +321,9 @@ function uninstall_plugin($type, $name) { // remove event handlers and dequeue pending events events_uninstall($component); + // Finally purge all caches. + purge_all_caches(); + echo $OUTPUT->notification(get_string('success'), 'notifysuccess'); } From 546b88641635fc1f213e4d9ce90d275be8ef606a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Mudr=C3=A1k?= Date: Wed, 10 Apr 2013 13:20:02 +0200 Subject: [PATCH 04/14] MDL-39087 Delete all component files in uninstall_plugin() --- lib/adminlib.php | 4 ++++ lib/filestorage/file_storage.php | 15 +++++++++++++++ lib/filestorage/tests/file_storage_test.php | 11 +++++++++++ 3 files changed, 30 insertions(+) diff --git a/lib/adminlib.php b/lib/adminlib.php index 89fa8e8f9d4..cf5af72c457 100644 --- a/lib/adminlib.php +++ b/lib/adminlib.php @@ -321,6 +321,10 @@ function uninstall_plugin($type, $name) { // remove event handlers and dequeue pending events events_uninstall($component); + // Delete all remaining files in the filepool owned by the component. + $fs = get_file_storage(); + $fs->delete_component_files($component); + // Finally purge all caches. purge_all_caches(); diff --git a/lib/filestorage/file_storage.php b/lib/filestorage/file_storage.php index de8be240dfe..f22635fc5bd 100644 --- a/lib/filestorage/file_storage.php +++ b/lib/filestorage/file_storage.php @@ -743,6 +743,21 @@ class file_storage { $filerecords->close(); } + /** + * Delete all files associated with the given component. + * + * @param string $component the component owning the file + */ + public function delete_component_files($component) { + global $DB; + + $filerecords = $DB->get_recordset('files', array('component' => $component)); + foreach ($filerecords as $filerecord) { + $this->get_file_instance($filerecord)->delete(); + } + $filerecords->close(); + } + /** * Move all the files in a file area from one context to another. * diff --git a/lib/filestorage/tests/file_storage_test.php b/lib/filestorage/tests/file_storage_test.php index 38e0eb1f59f..b895e578bb5 100644 --- a/lib/filestorage/tests/file_storage_test.php +++ b/lib/filestorage/tests/file_storage_test.php @@ -688,6 +688,17 @@ class filestoragelib_testcase extends advanced_testcase { $this->assertEquals(0, count($areafiles)); } + public function test_delete_component_files() { + $user = $this->setup_three_private_files(); + $fs = get_file_storage(); + + $areafiles = $fs->get_area_files($user->ctxid, 'user', 'private'); + $this->assertEquals(4, count($areafiles)); + $fs->delete_component_files('user'); + $areafiles = $fs->get_area_files($user->ctxid, 'user', 'private'); + $this->assertEquals(0, count($areafiles)); + } + public function test_create_file_from_url() { $this->resetAfterTest(true); From bfaed432149bee3259d65efd710bf32d06e99f3f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Mudr=C3=A1k?= Date: Wed, 10 Apr 2013 13:20:52 +0200 Subject: [PATCH 05/14] MDL-39087 Fix missing cronlib inclusion in file_storage::cron() This is not actually related to MDL-38259 but it was discovered while running unit tests for file_storage. When running the tests for this class separately, the cronlib.php was not included (it is included when this method is normally called during cron execution). --- lib/filestorage/file_storage.php | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/filestorage/file_storage.php b/lib/filestorage/file_storage.php index f22635fc5bd..ee29b75f6a1 100644 --- a/lib/filestorage/file_storage.php +++ b/lib/filestorage/file_storage.php @@ -2015,6 +2015,7 @@ class file_storage { */ public function cron() { global $CFG, $DB; + require_once($CFG->libdir.'/cronlib.php'); // find out all stale draft areas (older than 4 days) and purge them // those are identified by time stamp of the /. root dir From 7a46a55d004e28b75cfde1831873397da05db844 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Mudr=C3=A1k?= Date: Thu, 11 Apr 2013 14:20:22 +0200 Subject: [PATCH 06/14] MDL-39087 Fix plugin_manager::plugin_name() implementation This is not directly related to the issue. However, it turned out that if this method was called on plugin_manager without loaded plugins, it would throw an error. This new implementation uses cleaner access to the plugininfo subclass. --- lib/pluginlib.php | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/lib/pluginlib.php b/lib/pluginlib.php index 69557444bcc..d8332421cfa 100644 --- a/lib/pluginlib.php +++ b/lib/pluginlib.php @@ -221,12 +221,18 @@ class plugin_manager { /** * Returns a localized name of a given plugin * - * @param string $plugin name of the plugin, eg mod_workshop or auth_ldap + * @param string $component name of the plugin, eg mod_workshop or auth_ldap * @return string */ - public function plugin_name($plugin) { - list($type, $name) = normalize_component($plugin); - return $this->pluginsinfo[$type][$name]->displayname; + public function plugin_name($component) { + + $pluginfo = $this->get_plugin_info($component); + + if (is_null($pluginfo)) { + throw new moodle_exception('err_unknown_plugin', 'core_plugin', '', array('plugin' => $component)); + } + + return $pluginfo->displayname; } /** From 86a862cdc232d982953c1dafa8163f59e51d0673 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Mudr=C3=A1k?= Date: Thu, 11 Apr 2013 14:24:41 +0200 Subject: [PATCH 07/14] MDL-39087 Add missing unit tests for the plugin_manager This patch improves and adds unit tests for the plugin_manager class. These unit tests cover the existing functionalities. Tests for the new features related directly with MDL-38259 will be added in a separate commit (to make it clear what's related to it). --- lib/pluginlib.php | 14 +- .../fixtures/mockplugins/mod/bar/version.php | 4 + .../mockplugins/mod/foo/lish/frog/version.php | 6 + .../fixtures/mockplugins/mod/foo/version.php | 9 + lib/tests/pluginlib_test.php | 216 ++++++++++++++++-- 5 files changed, 235 insertions(+), 14 deletions(-) create mode 100644 lib/tests/fixtures/mockplugins/mod/bar/version.php create mode 100644 lib/tests/fixtures/mockplugins/mod/foo/lish/frog/version.php create mode 100644 lib/tests/fixtures/mockplugins/mod/foo/version.php diff --git a/lib/pluginlib.php b/lib/pluginlib.php index d8332421cfa..e92f236b311 100644 --- a/lib/pluginlib.php +++ b/lib/pluginlib.php @@ -298,7 +298,7 @@ class plugin_manager { * @return plugininfo_base|null the corresponding plugin information. */ public function get_plugin_info($component) { - list($type, $name) = normalize_component($component); + list($type, $name) = $this->normalize_component($component); $plugins = $this->get_plugins(); if (isset($plugins[$type][$name])) { return $plugins[$type][$name]; @@ -744,6 +744,18 @@ class plugin_manager { } } + /** + * Wrapper for the core function {@link normalize_component()}. + * + * This is here just to make it possible to mock it in unit tests. + * + * @param string $component + * @return array + */ + protected function normalize_component($component) { + return normalize_component($component); + } + /** * Reorders plugin types into a sequence to be displayed * diff --git a/lib/tests/fixtures/mockplugins/mod/bar/version.php b/lib/tests/fixtures/mockplugins/mod/bar/version.php new file mode 100644 index 00000000000..47613f65a4c --- /dev/null +++ b/lib/tests/fixtures/mockplugins/mod/bar/version.php @@ -0,0 +1,4 @@ +version = 2012030500; +$module->requires = 2012010100; diff --git a/lib/tests/fixtures/mockplugins/mod/foo/lish/frog/version.php b/lib/tests/fixtures/mockplugins/mod/foo/lish/frog/version.php new file mode 100644 index 00000000000..b5dd0eeba04 --- /dev/null +++ b/lib/tests/fixtures/mockplugins/mod/foo/lish/frog/version.php @@ -0,0 +1,6 @@ +version = 2013041103; +$plugin->requires = 2013010100; +$plugin->component = 'foolish_frog'; +$plugin->dependencies = array('mod_foo' => 2012030500); diff --git a/lib/tests/fixtures/mockplugins/mod/foo/version.php b/lib/tests/fixtures/mockplugins/mod/foo/version.php new file mode 100644 index 00000000000..5bcd9ed2c4e --- /dev/null +++ b/lib/tests/fixtures/mockplugins/mod/foo/version.php @@ -0,0 +1,9 @@ +version = 2012030500; +$module->requires = 2012010100; +$module->component = 'mod_foo'; +$module->dependencies = array( + 'mod_bar' => 2012030500, + 'mod_missing' => ANY_VERSION, +); diff --git a/lib/tests/pluginlib_test.php b/lib/tests/pluginlib_test.php index 141191386a3..237c6481c29 100644 --- a/lib/tests/pluginlib_test.php +++ b/lib/tests/pluginlib_test.php @@ -17,6 +17,10 @@ /** * Unit tests for the lib/pluginlib.php library * + * Execute the core_plugin group to run all tests in this file: + * + * $ phpunit --group core_plugin + * * @package core * @category phpunit * @copyright 2012 David Mudrak @@ -31,9 +35,15 @@ require_once($CFG->libdir.'/pluginlib.php'); /** * Tests of the basic API of the plugin manager + * + * @group core_plugin */ class plugin_manager_test extends advanced_testcase { + public function setUp() { + $this->resetAfterTest(); + } + public function test_plugin_manager_instance() { $pluginman = testable_plugin_manager::instance(); $this->assertTrue($pluginman instanceof testable_plugin_manager); @@ -43,7 +53,96 @@ class plugin_manager_test extends advanced_testcase { $pluginman = testable_plugin_manager::instance(); $plugins = $pluginman->get_plugins(); $this->assertTrue(isset($plugins['mod']['foo'])); + $this->assertTrue(isset($plugins['mod']['bar'])); + $this->assertTrue(isset($plugins['foolish']['frog'])); $this->assertTrue($plugins['mod']['foo'] instanceof testable_plugininfo_mod); + $this->assertTrue($plugins['mod']['bar'] instanceof testable_plugininfo_mod); + $this->assertTrue($plugins['foolish']['frog'] instanceof testable_pluginfo_foolish); + } + + public function test_get_subplugins() { + $pluginman = testable_plugin_manager::instance(); + $subplugins = $pluginman->get_subplugins(); + $this->assertTrue(isset($subplugins['mod_foo']['foolish'])); + } + + public function test_get_parent_of_subplugin() { + $pluginman = testable_plugin_manager::instance(); + $this->assertEquals('mod_foo', $pluginman->get_parent_of_subplugin('foolish')); + $this->assertSame(false, $pluginman->get_parent_of_subplugin('mod')); + $this->assertSame(false, $pluginman->get_parent_of_subplugin('unknown')); + } + + public function test_plugin_name() { + $pluginman = testable_plugin_manager::instance(); + $this->assertEquals('Foo', $pluginman->plugin_name('mod_foo')); + $this->assertEquals('Bar', $pluginman->plugin_name('mod_bar')); + $this->assertEquals('Frog', $pluginman->plugin_name('foolish_frog')); + } + + public function test_get_plugin_info() { + $pluginman = testable_plugin_manager::instance(); + $this->assertTrue($pluginman->get_plugin_info('mod_foo') instanceof testable_plugininfo_mod); + $this->assertTrue($pluginman->get_plugin_info('foolish_frog') instanceof testable_pluginfo_foolish); + } + + public function test_other_plugins_that_require() { + $pluginman = testable_plugin_manager::instance(); + $this->assertEquals(array('foolish_frog'), $pluginman->other_plugins_that_require('mod_foo')); + $this->assertEquals(array(), $pluginman->other_plugins_that_require('foolish_frog')); + $this->assertEquals(array('mod_foo'), $pluginman->other_plugins_that_require('mod_bar')); + $this->assertEquals(array('mod_foo'), $pluginman->other_plugins_that_require('mod_missing')); + } + + public function test_are_dependencies_satisfied() { + $pluginman = testable_plugin_manager::instance(); + $this->assertTrue($pluginman->are_dependencies_satisfied(array())); + $this->assertTrue($pluginman->are_dependencies_satisfied(array( + 'mod_bar' => 2012030500, + ))); + $this->assertTrue($pluginman->are_dependencies_satisfied(array( + 'mod_bar' => ANY_VERSION, + ))); + $this->assertFalse($pluginman->are_dependencies_satisfied(array( + 'mod_bar' => 2099010000, + ))); + $this->assertFalse($pluginman->are_dependencies_satisfied(array( + 'mod_bar' => 2012030500, + 'mod_missing' => ANY_VERSION, + ))); + } + + public function test_all_plugins_ok() { + $pluginman = testable_plugin_manager::instance(); + $failedplugins = array(); + $this->assertFalse($pluginman->all_plugins_ok(2013010100, $failedplugins)); + $this->assertTrue(in_array('mod_foo', $failedplugins)); // Requires mod_missing + $this->assertFalse(in_array('mod_bar', $failedplugins)); + $this->assertFalse(in_array('foolish_frog', $failedplugins)); + + $failedplugins = array(); + $this->assertFalse($pluginman->all_plugins_ok(2012010100, $failedplugins)); + $this->assertTrue(in_array('mod_foo', $failedplugins)); // Requires mod_missing + $this->assertFalse(in_array('mod_bar', $failedplugins)); + $this->assertTrue(in_array('foolish_frog', $failedplugins)); // Requires Moodle 2013010100 + + $failedplugins = array(); + $this->assertFalse($pluginman->all_plugins_ok(2011010100, $failedplugins)); + $this->assertTrue(in_array('mod_foo', $failedplugins)); // Requires mod_missing and Moodle 2012010100 + $this->assertTrue(in_array('mod_bar', $failedplugins)); // Requires Moodle 2012010100 + $this->assertTrue(in_array('foolish_frog', $failedplugins)); // Requires Moodle 2013010100 + } + + public function test_some_plugins_updatable() { + $pluginman = testable_plugin_manager::instance(); + $this->assertTrue($pluginman->some_plugins_updatable()); // We have available update for mod_foo. + } + + public function test_is_standard() { + $pluginman = testable_plugin_manager::instance(); + $this->assertTrue($pluginman->get_plugin_info('mod_bar')->is_standard()); + $this->assertFalse($pluginman->get_plugin_info('mod_foo')->is_standard()); + $this->assertFalse($pluginman->get_plugin_info('foolish_frog')->is_standard()); } public function test_get_status() { @@ -67,6 +166,8 @@ class plugin_manager_test extends advanced_testcase { /** * Tests of the basic API of the available update checker + * + * @group core_plugin */ class available_update_checker_test extends advanced_testcase { @@ -334,15 +435,31 @@ class testable_plugininfo_mod extends plugininfo_mod { $this->displayname = ucfirst($this->name); } - public function load_disk_version() { - $this->versiondisk = 2012030500; + public function is_standard() { + if ($this->component === 'mod_foo') { + return false; + } else { + return true; + } } - protected function load_version_php($disablecache=false) { - return (object)array( - 'version' => 2012030500, - 'requires' => 2012010100, - 'component' => $this->type.'_'.$this->name); + public function load_db_version() { + $this->versiondb = 2012022900; + } +} + + +/** + * Testable class representing subplugins of testable mod_foo + */ +class testable_pluginfo_foolish extends plugininfo_base { + + public function init_display_name() { + $this->displayname = ucfirst($this->name); + } + + public function is_standard() { + return false; } public function load_db_version() { @@ -378,15 +495,20 @@ class testable_plugin_manager extends plugin_manager { * @return array */ public function get_plugins($disablecache=false) { - global $CFG; + + $dirroot = dirname(__FILE__).'/fixtures/mockplugins'; $this->pluginsinfo = array( 'mod' => array( - 'foo' => plugininfo_default_factory::make('mod', $CFG->dirroot.'/mod', 'foo', - $CFG->dirroot.'/mod/foo', 'testable_plugininfo_mod'), - 'bar' => plugininfo_default_factory::make('mod', $CFG->dirroot.'/bar', 'bar', - $CFG->dirroot.'/mod/bar', 'testable_plugininfo_mod'), - ) + 'foo' => plugininfo_default_factory::make('mod', $dirroot.'/mod', 'foo', + $dirroot.'/mod/foo', 'testable_plugininfo_mod'), + 'bar' => plugininfo_default_factory::make('mod', $dirroot.'/bar', 'bar', + $dirroot.'/mod/bar', 'testable_plugininfo_mod'), + ), + 'foolish' => array( + 'frog' => plugininfo_default_factory::make('foolish', $dirroot.'/mod/foo/foolish', 'frog', + $dirroot.'/mod/foo/lish/frog', 'testable_pluginfo_foolish'), + ), ); $checker = testable_available_update_checker::instance(); @@ -395,6 +517,72 @@ class testable_plugin_manager extends plugin_manager { return $this->pluginsinfo; } + + /** + * Testable version of {@link plugin_manager::get_subplugins()} that works with + * the simulated environment. + * + * In this case, the mod_foo fake module provides subplugins of type 'foolish'. + * + * @param bool $disablecache ignored in this class + * @return array + */ + public function get_subplugins($disablecache=false) { + return array( + 'mod_foo' => array( + 'foolish' => (object)array( + 'type' => 'foolish', + 'typerootdir' => 'mod/foo/lish', + ), + ), + ); + } + + /** + * Adds support for mock plugin types. + */ + protected function normalize_component($component) { + + // List of mock plugin types used in these unit tests. + $faketypes = array('foolish'); + + foreach ($faketypes as $faketype) { + if (strpos($component, $faketype.'_') === 0) { + return explode('_', $component, 2); + } + } + + return parent::normalize_component($component); + } + + public function plugintype_name($type) { + return ucfirst($type); + } + + public function plugintype_name_plural($type) { + return ucfirst($type).'s'; // Simple, isn't it? ;-) + } + + public function plugin_external_source($component) { + if ($component === 'foolish_frog') { + return true; + } + return false; + } + + public static function standard_plugins_list($type) { + $standard_plugins = array( + 'mod' => array( + 'bar', + ), + ); + + if (isset($standard_plugins[$type])) { + return $standard_plugins[$type]; + } else { + return false; + } + } } @@ -555,6 +743,8 @@ class testable_available_update_deployer extends available_update_deployer { /** * Test cases for {@link available_update_deployer} class + * + * @group core_plugin */ class available_update_deployer_test extends advanced_testcase { From c6f4c88ffb5129779a174dd0f171c139db2ee78c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Mudr=C3=A1k?= Date: Thu, 11 Apr 2013 14:39:51 +0200 Subject: [PATCH 08/14] MDL-39087 Offer deleting for standard plugins if possible, too The plugin_manager::is_plugin_folder_removable() method should do just one thing and do it well. Also, as was raised during the peer-review, there should not be technical differences between standard plugins and add-ons. --- lib/pluginlib.php | 7 ------- 1 file changed, 7 deletions(-) diff --git a/lib/pluginlib.php b/lib/pluginlib.php index e92f236b311..716a3106554 100644 --- a/lib/pluginlib.php +++ b/lib/pluginlib.php @@ -465,9 +465,6 @@ class plugin_manager { /** * Check to see if the given plugin folder can be removed by the web server process. * - * This is intended to be used for installed add-ons mainly. For standard plugins, - * false is always returned for now. - * * @param string $component full frankenstyle component * @return bool */ @@ -479,10 +476,6 @@ class plugin_manager { return false; } - if ($pluginfo->is_standard()) { - return false; - } - // To be able to remove the plugin folder, its parent must be writable, too. if (!is_writable(dirname($pluginfo->rootdir))) { return false; From d7d48b40910092f0e4f1e7851980776c5e5cf8d9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Mudr=C3=A1k?= Date: Thu, 11 Apr 2013 17:13:18 +0200 Subject: [PATCH 09/14] MDL-39087 Add new helper methods to the plugin_manager API These are mainly intended for callers that had to iterate over get_plugins() result manually. --- admin/courseformats.php | 3 +- lib/adminlib.php | 6 ++-- lib/editor/tinymce/settings.php | 3 +- lib/pluginlib.php | 63 +++++++++++++++++++++++++++++++-- lib/tests/pluginlib_test.php | 25 +++++++++++++ lib/upgrade.txt | 2 ++ 6 files changed, 92 insertions(+), 10 deletions(-) diff --git a/admin/courseformats.php b/admin/courseformats.php index a8377dec7a7..fddcdda750f 100644 --- a/admin/courseformats.php +++ b/admin/courseformats.php @@ -40,8 +40,7 @@ require_sesskey(); $return = new moodle_url('/admin/settings.php', array('section' => 'manageformats')); -$allplugins = plugin_manager::instance()->get_plugins(); -$formatplugins = $allplugins['format']; +$formatplugins = plugin_manager::instance()->get_plugins_of_type('format'); $sortorder = array_flip(array_keys($formatplugins)); if (!isset($formatplugins[$formatname])) { diff --git a/lib/adminlib.php b/lib/adminlib.php index cf5af72c457..2abbe7af100 100644 --- a/lib/adminlib.php +++ b/lib/adminlib.php @@ -6071,8 +6071,7 @@ class admin_setting_manageformats extends admin_setting { if (parent::is_related($query)) { return true; } - $allplugins = plugin_manager::instance()->get_plugins(); - $formats = $allplugins['format']; + $formats = plugin_manager::instance()->get_plugins_of_type('format'); foreach ($formats as $format) { if (strpos($format->component, $query) !== false || strpos(textlib::strtolower($format->displayname), $query) !== false) { @@ -6095,8 +6094,7 @@ class admin_setting_manageformats extends admin_setting { $return = $OUTPUT->heading(new lang_string('courseformats'), 3, 'main'); $return .= $OUTPUT->box_start('generalbox formatsui'); - $allplugins = plugin_manager::instance()->get_plugins(); - $formats = $allplugins['format']; + $formats = plugin_manager::instance()->get_plugins_of_type('format'); // display strings $txt = get_strings(array('settings', 'name', 'enable', 'disable', 'up', 'down', 'default', 'delete')); diff --git a/lib/editor/tinymce/settings.php b/lib/editor/tinymce/settings.php index 1e8d3f4a004..6fbe2671bd8 100644 --- a/lib/editor/tinymce/settings.php +++ b/lib/editor/tinymce/settings.php @@ -48,8 +48,7 @@ $ADMIN->add('editortinymce', $settings); unset($settings); require_once("$CFG->libdir/pluginlib.php"); -$allplugins = plugin_manager::instance()->get_plugins(); -foreach ($allplugins['tinymce'] as $plugin) { +foreach (plugin_manager::instance()->get_plugins_of_type('tinymce') as $plugin) { $plugin->load_settings($ADMIN, 'editortinymce', $hassiteconfig); } diff --git a/lib/pluginlib.php b/lib/pluginlib.php index 716a3106554..2522e225ea5 100644 --- a/lib/pluginlib.php +++ b/lib/pluginlib.php @@ -107,6 +107,27 @@ class plugin_manager { return $this->reorder_plugin_types(get_plugin_types($fullpaths)); } + /** + * Returns list of known plugins of the given type + * + * This method returns the subset of the tree returned by {@link self::get_plugins()}. + * If the given type is not known, empty array is returned. + * + * @param string $type plugin type, e.g. 'mod' or 'workshopallocation' + * @param bool $disablecache force reload, cache can be used otherwise + * @return array (string)plugin name (e.g. 'workshop') => corresponding subclass of {@link plugininfo_base} + */ + public function get_plugins_of_type($type, $disablecache=false) { + + $plugins = $this->get_plugins($disablecache); + + if (!isset($plugins[$type])) { + return array(); + } + + return $plugins[$type]; + } + /** * Returns a tree of known plugins and information about them * @@ -161,6 +182,41 @@ class plugin_manager { return $this->pluginsinfo; } + /** + * Returns list of all known subplugins of the given plugin + * + * For plugins that do not provide subplugins (i.e. there is no support for it), + * empty array is returned. + * + * @param string $component full component name, e.g. 'mod_workshop' + * @param bool $disablecache force reload, cache can be used otherwise + * @return array (string) component name (e.g. 'workshopallocation_random') => subclass of {@link plugininfo_base} + */ + public function get_subplugins_of_plugin($component, $disablecache=false) { + + $pluginfo = $this->get_plugin_info($component, $disablecache); + + if (is_null($pluginfo)) { + return array(); + } + + $subplugins = $this->get_subplugins($disablecache); + + if (!isset($subplugins[$pluginfo->component])) { + return array(); + } + + $list = array(); + + foreach ($subplugins[$pluginfo->component] as $subdata) { + foreach ($this->get_plugins_of_type($subdata->type) as $subpluginfo) { + $list[$subpluginfo->component] = $subpluginfo; + } + } + + return $list; + } + /** * Returns list of plugins that define their subplugins and the information * about them from the db/subplugins.php file. @@ -294,12 +350,15 @@ class plugin_manager { } /** + * Returns information about the known plugin, or null + * * @param string $component frankenstyle component name. + * @param bool $disablecache force reload, cache can be used otherwise * @return plugininfo_base|null the corresponding plugin information. */ - public function get_plugin_info($component) { + public function get_plugin_info($component, $disablecache=false) { list($type, $name) = $this->normalize_component($component); - $plugins = $this->get_plugins(); + $plugins = $this->get_plugins($disablecache); if (isset($plugins[$type][$name])) { return $plugins[$type][$name]; } else { diff --git a/lib/tests/pluginlib_test.php b/lib/tests/pluginlib_test.php index 237c6481c29..39a1db0a7f9 100644 --- a/lib/tests/pluginlib_test.php +++ b/lib/tests/pluginlib_test.php @@ -49,9 +49,24 @@ class plugin_manager_test extends advanced_testcase { $this->assertTrue($pluginman instanceof testable_plugin_manager); } + public function test_get_plugins_of_type() { + $pluginman = testable_plugin_manager::instance(); + $mods = $pluginman->get_plugins_of_type('mod'); + $this->assertEquals('array', gettype($mods)); + $this->assertEquals(2, count($mods)); + $this->assertTrue($mods['foo'] instanceof testable_plugininfo_mod); + $this->assertTrue($mods['bar'] instanceof testable_plugininfo_mod); + $foolishes = $pluginman->get_plugins_of_type('foolish'); + $this->assertEquals(1, count($foolishes)); + $this->assertTrue($foolishes['frog'] instanceof testable_pluginfo_foolish); + $unknown = $pluginman->get_plugins_of_type('muhehe'); + $this->assertSame(array(), $unknown); + } + public function test_get_plugins() { $pluginman = testable_plugin_manager::instance(); $plugins = $pluginman->get_plugins(); + $this->assertEquals('array', gettype($plugins)); $this->assertTrue(isset($plugins['mod']['foo'])); $this->assertTrue(isset($plugins['mod']['bar'])); $this->assertTrue(isset($plugins['foolish']['frog'])); @@ -60,6 +75,16 @@ class plugin_manager_test extends advanced_testcase { $this->assertTrue($plugins['foolish']['frog'] instanceof testable_pluginfo_foolish); } + public function test_get_subplugins_of_plugin() { + $pluginman = testable_plugin_manager::instance(); + $this->assertSame(array(), $pluginman->get_subplugins_of_plugin('mod_missing')); + $this->assertSame(array(), $pluginman->get_subplugins_of_plugin('mod_bar')); + $foosubs = $pluginman->get_subplugins_of_plugin('mod_foo'); + $this->assertEquals('array', gettype($foosubs)); + $this->assertEquals(1, count($foosubs)); + $this->assertTrue($foosubs['foolish_frog'] instanceof testable_pluginfo_foolish); + } + public function test_get_subplugins() { $pluginman = testable_plugin_manager::instance(); $subplugins = $pluginman->get_subplugins(); diff --git a/lib/upgrade.txt b/lib/upgrade.txt index 3af68c17f07..eedc6903c55 100644 --- a/lib/upgrade.txt +++ b/lib/upgrade.txt @@ -51,6 +51,8 @@ information provided here is intended especially for developers. * Additional (optional) param $onlyactive has been added to get_enrolled_users, count_enrolled_users functions to get information for only active (excluding suspended enrolments) users. Included two helper functions extract_suspended_users, get_suspended_userids to extract suspended user information. +* The plugin_manager class now provides two new helper methods for getting information + about known plugins: get_plugins_of_type() and get_subplugins_of_plugin(). Database (DML) layer: * $DB->sql_empty() is deprecated, you have to use sql parameters with empty values instead, From 73658371eb3a7f6569e0d9d9cafff1dfb4beea02 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Mudr=C3=A1k?= Date: Fri, 12 Apr 2013 01:00:51 +0200 Subject: [PATCH 10/14] MDL-39087 Simplify get_uninstall_url() interpretation The get_uninstall_url() method of all subclasses of plugininfo_base class is now expected to always return moodle_url. Subclasses can use the new method is_uninstall_allowed() to control the availability of the 'Uninstall' link at the Plugins overview page (previously they would do it by get_uninstall_url() returning null). By default, URL to a new general plugin uninstall tool is returned. Unless the plugin type needs extra steps that can't be handled by plugininfo_xxx::uninstall() method or xmldb_xxx_uninstall() function, this default URL should satisfy all plugin types. The overall logic is implemented in plugin_manager::can_install_plugin() that respects the plugininfo class decision and vetoes it in certain cases (typically when plugin or its subplugin is required by some other plugin). --- admin/renderer.php | 4 +- lib/editor/tinymce/adminlib.php | 5 + lib/pluginlib.php | 219 +++++++++++++++--- .../mockplugins/mod/baz/meg/one/version.php | 5 + .../fixtures/mockplugins/mod/baz/version.php | 4 + .../mod/foo/lish/hippo/version.php | 6 + .../fixtures/mockplugins/mod/foo/version.php | 1 + .../mockplugins/mod/qux/cat/one/version.php | 6 + .../fixtures/mockplugins/mod/qux/version.php | 5 + lib/tests/pluginlib_test.php | 195 ++++++++++++++-- lib/upgrade.txt | 7 + 11 files changed, 396 insertions(+), 61 deletions(-) create mode 100644 lib/tests/fixtures/mockplugins/mod/baz/meg/one/version.php create mode 100644 lib/tests/fixtures/mockplugins/mod/baz/version.php create mode 100644 lib/tests/fixtures/mockplugins/mod/foo/lish/hippo/version.php create mode 100644 lib/tests/fixtures/mockplugins/mod/qux/cat/one/version.php create mode 100644 lib/tests/fixtures/mockplugins/mod/qux/version.php diff --git a/admin/renderer.php b/admin/renderer.php index 1784de95de2..c215a898e3a 100644 --- a/admin/renderer.php +++ b/admin/renderer.php @@ -1229,8 +1229,8 @@ class core_admin_renderer extends plugin_renderer_base { $actions[] = html_writer::link($settingsurl, get_string('settings', 'core_plugin'), array('class' => 'settings')); } - $uninstallurl = $plugin->get_uninstall_url(); - if (!is_null($uninstallurl)) { + if ($pluginman->can_uninstall_plugin($plugin->component)) { + $uninstallurl = $plugin->get_uninstall_url(); $actions[] = html_writer::link($uninstallurl, get_string('uninstall', 'core_plugin'), array('class' => 'uninstall')); } diff --git a/lib/editor/tinymce/adminlib.php b/lib/editor/tinymce/adminlib.php index 36eb494e803..6b4b23242e8 100644 --- a/lib/editor/tinymce/adminlib.php +++ b/lib/editor/tinymce/adminlib.php @@ -35,6 +35,11 @@ require_once("$CFG->libdir/pluginlib.php"); * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ class plugininfo_tinymce extends plugininfo_base { + + public function is_uninstall_allowed() { + return true; + } + public function get_uninstall_url() { return new moodle_url('/lib/editor/tinymce/subplugins.php', array('delete' => $this->name, 'sesskey' => sesskey())); } diff --git a/lib/pluginlib.php b/lib/pluginlib.php index 2522e225ea5..354cc4370cc 100644 --- a/lib/pluginlib.php +++ b/lib/pluginlib.php @@ -468,6 +468,70 @@ class plugin_manager { return $return; } + /** + * Is it possible to uninstall the given plugin? + * + * False is returned if the plugininfo subclass declares the uninstall should + * not be allowed via {@link plugininfo_base::is_uninstall_allowed()} or if the + * core vetoes it (e.g. becase the plugin or some of its subplugins is required + * by some other installed plugin). + * + * @param string $component full frankenstyle name, e.g. mod_foobar + * @return bool + */ + public function can_uninstall_plugin($component) { + + $pluginfo = $this->get_plugin_info($component); + + if (is_null($pluginfo)) { + return false; + } + + // Backwards compatibility check. + if (is_null($pluginfo->get_uninstall_url())) { + debugging('plugininfo_base subclasses should use is_uninstall_allowed() instead of returning null in get_uninstall_url()', + DEBUG_DEVELOPER); + return false; + } + + // In case the $component has subplugins, get their list. + $mysubplugins = $this->get_subplugins_of_plugin($pluginfo->component); + + // In case the $component is a subplugin, get all subplugins of its parent (i.e. siblings). + $myparent = $this->get_parent_of_subplugin($pluginfo->type); + if ($myparent === false) { + $mysiblings = array(); + } else { + $mysiblings = $this->get_subplugins_of_plugin($myparent); + } + + // If the plugin has subplugins, check we can uninstall them first. + foreach ($mysubplugins as $subpluginfo) { + if (!$this->can_uninstall_plugin($subpluginfo->component)) { + return false; + } + } + + // Check there are no other plugins (but eventual subplugins or siblings) that + // require us. Subplugins would be uninstalled together with the parent plugin + // without the need to uninstall each of them individually. + foreach ($this->other_plugins_that_require($pluginfo->component) as $requiresme) { + $ismyparent = ($myparent === $requiresme); + $ismysubplugin = in_array($requiresme, array_keys($mysubplugins)); + $ismysibling = in_array($requiresme, array_keys($mysiblings)); + if (!$ismyparent and !$ismysubplugin and !$ismysibling) { + return false; + } + } + + // Finally give the plugin plugininfo subclass a chance to prevent uninstallation. + if (!$pluginfo->is_uninstall_allowed()) { + return false; + } + + return true; + } + /** * Uninstall the given plugin. * @@ -2500,6 +2564,24 @@ abstract class plugininfo_base { return $this->dependencies; } + /** + * Is this is a subplugin? + * + * @return boolean + */ + public function is_subplugin() { + return ($this->get_parent_plugin() !== false); + } + + /** + * If I am a subplugin, return the name of my parent plugin. + * + * @return string|bool false if not a subplugin, name of the parent otherwise + */ + public function get_parent_plugin() { + return $this->get_plugin_manager()->get_parent_of_subplugin($this->type); + } + /** * Sets {@link $versiondb} property to a numerical value representing the * currently installed version of the plugin. @@ -2709,32 +2791,42 @@ abstract class plugininfo_base { public function load_settings(part_of_admin_tree $adminroot, $parentnodename, $hassiteconfig) { } + /** + * Should there be a way to uninstall the plugin via the administration UI + * + * By default, uninstallation is allowed for all non-standard add-ons. Subclasses + * may want to override this to allow uninstallation of all plugins (simply by + * returning true unconditionally). Subplugins follow their parent plugin's + * decision by default. + * + * Note that even if true is returned, the core may still prohibit the uninstallation, + * e.g. in case there are other plugins that depend on this one. + * + * @return boolean + */ + public function is_uninstall_allowed() { + + if ($this->is_subplugin()) { + return $this->get_plugin_manager()->get_plugin_info($this->get_parent_plugin())->is_uninstall_allowed(); + } + + if ($this->is_standard()) { + return false; + } + + return true; + } + /** * Returns the URL of the screen where this plugin can be uninstalled * * Visiting that URL must be safe, that is a manual confirmation is needed - * for actual uninstallation of the plugin. Null value means that the - * plugin cannot be uninstalled (such as due to dependencies), or it does - * not support uninstallation, or the location of the screen is not - * available (shortly, the 'Uninstall' link should not be displayed). + * for actual uninstallation of the plugin. By default, URL to a common + * uninstalling tool is returned. * - * By default, URL to a common uninstalling handler is returned for all - * add-ons and null is returned for standard plugins. - * - * @return null|moodle_url + * @return moodle_url */ public function get_uninstall_url() { - - if ($this->is_standard()) { - return null; - } - - $pluginman = plugin_manager::instance(); - $requiredby = $pluginman->other_plugins_that_require($this->component); - if (!empty($requiredby)) { - return null; - } - return $this->get_default_uninstall_url(); } @@ -2771,7 +2863,7 @@ abstract class plugininfo_base { * * @return moodle_url */ - protected function get_default_uninstall_url() { + protected final function get_default_uninstall_url() { return new moodle_url('/admin/plugins.php', array( 'sesskey' => sesskey(), 'uninstall' => $this->component, @@ -2809,6 +2901,15 @@ abstract class plugininfo_base { return false; } } + + /** + * Provides access to the plugin_manager singleton. + * + * @return plugin_manmager + */ + protected function get_plugin_manager() { + return plugin_manager::instance(); + } } @@ -2935,8 +3036,11 @@ class plugininfo_block extends plugininfo_base { } } - public function get_uninstall_url() { + public function is_uninstall_allowed() { + return true; + } + public function get_uninstall_url() { $blocksinfo = self::get_blocks_info(); return new moodle_url('/admin/blocks.php', array('delete' => $blocksinfo[$this->name]->id, 'sesskey' => sesskey())); } @@ -3071,6 +3175,10 @@ class plugininfo_filter extends plugininfo_base { } } + public function is_uninstall_allowed() { + return true; + } + public function get_uninstall_url() { return new moodle_url('/admin/filters.php', array('sesskey' => sesskey(), 'filterpath' => $this->name, 'action' => 'delete')); } @@ -3254,15 +3362,24 @@ class plugininfo_mod extends plugininfo_base { } } - public function get_uninstall_url() { + /** + * Allow all activity modules but Forum to be uninstalled. - if ($this->name !== 'forum') { - return new moodle_url('/admin/modules.php', array('delete' => $this->name, 'sesskey' => sesskey())); + * This exception for the Forum has been hard-coded in Moodle since ages, + * we may want to re-think it one day. + */ + public function is_uninstall_allowed() { + if ($this->name === 'forum') { + return false; } else { - return null; + return true; } } + public function get_uninstall_url() { + return new moodle_url('/admin/modules.php', array('delete' => $this->name, 'sesskey' => sesskey())); + } + /** * Provides access to the records in {modules} table * @@ -3296,6 +3413,10 @@ class plugininfo_mod extends plugininfo_base { */ class plugininfo_qbehaviour extends plugininfo_base { + public function is_uninstall_allowed() { + return true; + } + public function get_uninstall_url() { return new moodle_url('/admin/qbehaviours.php', array('delete' => $this->name, 'sesskey' => sesskey())); @@ -3308,6 +3429,10 @@ class plugininfo_qbehaviour extends plugininfo_base { */ class plugininfo_qtype extends plugininfo_base { + public function is_uninstall_allowed() { + return true; + } + public function get_uninstall_url() { return new moodle_url('/admin/qtypes.php', array('delete' => $this->name, 'sesskey' => sesskey())); @@ -3432,6 +3557,10 @@ class plugininfo_enrol extends plugininfo_base { } } + public function is_uninstall_allowed() { + return true; + } + public function get_uninstall_url() { return new moodle_url('/admin/enrol.php', array('action' => 'uninstall', 'enrol' => $this->name, 'sesskey' => sesskey())); } @@ -3482,16 +3611,21 @@ class plugininfo_message extends plugininfo_base { } } + public function is_uninstall_allowed() { + $processors = get_message_processors(); + if (isset($processors[$this->name])) { + return true; + } else { + return false; + } + } + /** * @see plugintype_interface::get_uninstall_url() */ public function get_uninstall_url() { $processors = get_message_processors(); - if (isset($processors[$this->name])) { - return new moodle_url('/admin/message.php', array('uninstall' => $processors[$this->name]->id, 'sesskey' => sesskey())); - } else { - return null; - } + return new moodle_url('/admin/message.php', array('uninstall' => $processors[$this->name]->id, 'sesskey' => sesskey())); } } @@ -3636,6 +3770,10 @@ class plugininfo_mnetservice extends plugininfo_base { */ class plugininfo_tool extends plugininfo_base { + public function is_uninstall_allowed() { + return true; + } + public function get_uninstall_url() { return new moodle_url('/admin/tools.php', array('delete' => $this->name, 'sesskey' => sesskey())); } @@ -3647,6 +3785,10 @@ class plugininfo_tool extends plugininfo_base { */ class plugininfo_report extends plugininfo_base { + public function is_uninstall_allowed() { + return true; + } + public function get_uninstall_url() { return new moodle_url('/admin/reports.php', array('delete' => $this->name, 'sesskey' => sesskey())); } @@ -3770,6 +3912,10 @@ class plugininfo_webservice extends plugininfo_base { return false; } + public function is_uninstall_allowed() { + return true; + } + public function get_uninstall_url() { return new moodle_url('/admin/webservice/protocols.php', array('sesskey' => sesskey(), 'action' => 'uninstall', 'webservice' => $this->name)); @@ -3825,11 +3971,16 @@ class plugininfo_format extends plugininfo_base { return !get_config($this->component, 'disabled'); } - public function get_uninstall_url() { + public function is_uninstall_allowed() { if ($this->name !== get_config('moodlecourse', 'format') && $this->name !== 'site') { - return new moodle_url('/admin/courseformats.php', - array('sesskey' => sesskey(), 'action' => 'uninstall', 'format' => $this->name)); + return true; + } else { + return false; } - return null; + } + + public function get_uninstall_url() { + return new moodle_url('/admin/courseformats.php', + array('sesskey' => sesskey(), 'action' => 'uninstall', 'format' => $this->name)); } } diff --git a/lib/tests/fixtures/mockplugins/mod/baz/meg/one/version.php b/lib/tests/fixtures/mockplugins/mod/baz/meg/one/version.php new file mode 100644 index 00000000000..b099a779bbf --- /dev/null +++ b/lib/tests/fixtures/mockplugins/mod/baz/meg/one/version.php @@ -0,0 +1,5 @@ +version = 2013041103; +$plugin->requires = 2013010100; +$plugin->component = 'bazmeg_one'; diff --git a/lib/tests/fixtures/mockplugins/mod/baz/version.php b/lib/tests/fixtures/mockplugins/mod/baz/version.php new file mode 100644 index 00000000000..47613f65a4c --- /dev/null +++ b/lib/tests/fixtures/mockplugins/mod/baz/version.php @@ -0,0 +1,4 @@ +version = 2012030500; +$module->requires = 2012010100; diff --git a/lib/tests/fixtures/mockplugins/mod/foo/lish/hippo/version.php b/lib/tests/fixtures/mockplugins/mod/foo/lish/hippo/version.php new file mode 100644 index 00000000000..f64b02d59ff --- /dev/null +++ b/lib/tests/fixtures/mockplugins/mod/foo/lish/hippo/version.php @@ -0,0 +1,6 @@ +version = 2013041103; +$plugin->requires = 2012010100; +$plugin->component = 'foolish_hippo'; +$plugin->dependencies = array('foolish_frog' => ANY_VERSION); diff --git a/lib/tests/fixtures/mockplugins/mod/foo/version.php b/lib/tests/fixtures/mockplugins/mod/foo/version.php index 5bcd9ed2c4e..a12a76a40dc 100644 --- a/lib/tests/fixtures/mockplugins/mod/foo/version.php +++ b/lib/tests/fixtures/mockplugins/mod/foo/version.php @@ -6,4 +6,5 @@ $module->component = 'mod_foo'; $module->dependencies = array( 'mod_bar' => 2012030500, 'mod_missing' => ANY_VERSION, + 'foolish_frog' => ANY_VERSION, ); diff --git a/lib/tests/fixtures/mockplugins/mod/qux/cat/one/version.php b/lib/tests/fixtures/mockplugins/mod/qux/cat/one/version.php new file mode 100644 index 00000000000..4c9e9781270 --- /dev/null +++ b/lib/tests/fixtures/mockplugins/mod/qux/cat/one/version.php @@ -0,0 +1,6 @@ +version = 2013041103; +$plugin->requires = 2013010100; +$plugin->component = 'quxcat_one'; +$plugin->dependencies = array('bazmeg_one' => 2013010100); diff --git a/lib/tests/fixtures/mockplugins/mod/qux/version.php b/lib/tests/fixtures/mockplugins/mod/qux/version.php new file mode 100644 index 00000000000..9c1c5f01a28 --- /dev/null +++ b/lib/tests/fixtures/mockplugins/mod/qux/version.php @@ -0,0 +1,5 @@ +version = 2013041103; +$plugin->requires = 2013010100; +$plugin->component = 'mod_qux'; diff --git a/lib/tests/pluginlib_test.php b/lib/tests/pluginlib_test.php index 39a1db0a7f9..0556b2ed333 100644 --- a/lib/tests/pluginlib_test.php +++ b/lib/tests/pluginlib_test.php @@ -53,12 +53,21 @@ class plugin_manager_test extends advanced_testcase { $pluginman = testable_plugin_manager::instance(); $mods = $pluginman->get_plugins_of_type('mod'); $this->assertEquals('array', gettype($mods)); - $this->assertEquals(2, count($mods)); + $this->assertEquals(4, count($mods)); $this->assertTrue($mods['foo'] instanceof testable_plugininfo_mod); $this->assertTrue($mods['bar'] instanceof testable_plugininfo_mod); + $this->assertTrue($mods['baz'] instanceof testable_plugininfo_mod); + $this->assertTrue($mods['qux'] instanceof testable_plugininfo_mod); $foolishes = $pluginman->get_plugins_of_type('foolish'); - $this->assertEquals(1, count($foolishes)); + $this->assertEquals(2, count($foolishes)); $this->assertTrue($foolishes['frog'] instanceof testable_pluginfo_foolish); + $this->assertTrue($foolishes['hippo'] instanceof testable_pluginfo_foolish); + $bazmegs = $pluginman->get_plugins_of_type('bazmeg'); + $this->assertEquals(1, count($bazmegs)); + $this->assertTrue($bazmegs['one'] instanceof testable_pluginfo_bazmeg); + $quxcats = $pluginman->get_plugins_of_type('quxcat'); + $this->assertEquals(1, count($quxcats)); + $this->assertTrue($quxcats['one'] instanceof testable_pluginfo_quxcat); $unknown = $pluginman->get_plugins_of_type('muhehe'); $this->assertSame(array(), $unknown); } @@ -69,10 +78,16 @@ class plugin_manager_test extends advanced_testcase { $this->assertEquals('array', gettype($plugins)); $this->assertTrue(isset($plugins['mod']['foo'])); $this->assertTrue(isset($plugins['mod']['bar'])); + $this->assertTrue(isset($plugins['mod']['baz'])); $this->assertTrue(isset($plugins['foolish']['frog'])); + $this->assertTrue(isset($plugins['foolish']['hippo'])); $this->assertTrue($plugins['mod']['foo'] instanceof testable_plugininfo_mod); $this->assertTrue($plugins['mod']['bar'] instanceof testable_plugininfo_mod); + $this->assertTrue($plugins['mod']['baz'] instanceof testable_plugininfo_mod); $this->assertTrue($plugins['foolish']['frog'] instanceof testable_pluginfo_foolish); + $this->assertTrue($plugins['foolish']['hippo'] instanceof testable_pluginfo_foolish); + $this->assertTrue($plugins['bazmeg']['one'] instanceof testable_pluginfo_bazmeg); + $this->assertTrue($plugins['quxcat']['one'] instanceof testable_pluginfo_quxcat); } public function test_get_subplugins_of_plugin() { @@ -81,21 +96,39 @@ class plugin_manager_test extends advanced_testcase { $this->assertSame(array(), $pluginman->get_subplugins_of_plugin('mod_bar')); $foosubs = $pluginman->get_subplugins_of_plugin('mod_foo'); $this->assertEquals('array', gettype($foosubs)); - $this->assertEquals(1, count($foosubs)); + $this->assertEquals(2, count($foosubs)); $this->assertTrue($foosubs['foolish_frog'] instanceof testable_pluginfo_foolish); + $this->assertTrue($foosubs['foolish_hippo'] instanceof testable_pluginfo_foolish); + $bazsubs = $pluginman->get_subplugins_of_plugin('mod_baz'); + $this->assertEquals('array', gettype($bazsubs)); + $this->assertEquals(1, count($bazsubs)); + $this->assertTrue($bazsubs['bazmeg_one'] instanceof testable_pluginfo_bazmeg); + $quxsubs = $pluginman->get_subplugins_of_plugin('mod_qux'); + $this->assertEquals('array', gettype($quxsubs)); + $this->assertEquals(1, count($quxsubs)); + $this->assertTrue($quxsubs['quxcat_one'] instanceof testable_pluginfo_quxcat); } public function test_get_subplugins() { $pluginman = testable_plugin_manager::instance(); $subplugins = $pluginman->get_subplugins(); $this->assertTrue(isset($subplugins['mod_foo']['foolish'])); + $this->assertTrue(isset($subplugins['mod_baz']['bazmeg'])); + $this->assertTrue(isset($subplugins['mod_qux']['quxcat'])); } public function test_get_parent_of_subplugin() { $pluginman = testable_plugin_manager::instance(); $this->assertEquals('mod_foo', $pluginman->get_parent_of_subplugin('foolish')); + $this->assertEquals('mod_baz', $pluginman->get_parent_of_subplugin('bazmeg')); + $this->assertEquals('mod_qux', $pluginman->get_parent_of_subplugin('quxcat')); $this->assertSame(false, $pluginman->get_parent_of_subplugin('mod')); $this->assertSame(false, $pluginman->get_parent_of_subplugin('unknown')); + $plugins = $pluginman->get_plugins(); + $this->assertFalse($plugins['mod']['foo']->is_subplugin()); + $this->assertSame(false, $plugins['mod']['foo']->get_parent_plugin()); + $this->assertTrue($plugins['foolish']['frog']->is_subplugin()); + $this->assertEquals('mod_foo', $plugins['foolish']['frog']->get_parent_plugin()); } public function test_plugin_name() { @@ -103,6 +136,9 @@ class plugin_manager_test extends advanced_testcase { $this->assertEquals('Foo', $pluginman->plugin_name('mod_foo')); $this->assertEquals('Bar', $pluginman->plugin_name('mod_bar')); $this->assertEquals('Frog', $pluginman->plugin_name('foolish_frog')); + $this->assertEquals('Hippo', $pluginman->plugin_name('foolish_hippo')); + $this->assertEquals('One', $pluginman->plugin_name('bazmeg_one')); + $this->assertEquals('One', $pluginman->plugin_name('quxcat_one')); } public function test_get_plugin_info() { @@ -114,9 +150,13 @@ class plugin_manager_test extends advanced_testcase { public function test_other_plugins_that_require() { $pluginman = testable_plugin_manager::instance(); $this->assertEquals(array('foolish_frog'), $pluginman->other_plugins_that_require('mod_foo')); - $this->assertEquals(array(), $pluginman->other_plugins_that_require('foolish_frog')); + $this->assertEquals(2, count($pluginman->other_plugins_that_require('foolish_frog'))); + $this->assertTrue(in_array('foolish_hippo', $pluginman->other_plugins_that_require('foolish_frog'))); + $this->assertTrue(in_array('mod_foo', $pluginman->other_plugins_that_require('foolish_frog'))); + $this->assertEquals(array(), $pluginman->other_plugins_that_require('foolish_hippo')); $this->assertEquals(array('mod_foo'), $pluginman->other_plugins_that_require('mod_bar')); $this->assertEquals(array('mod_foo'), $pluginman->other_plugins_that_require('mod_missing')); + $this->assertEquals(array('quxcat_one'), $pluginman->other_plugins_that_require('bazmeg_one')); } public function test_are_dependencies_satisfied() { @@ -144,18 +184,21 @@ class plugin_manager_test extends advanced_testcase { $this->assertTrue(in_array('mod_foo', $failedplugins)); // Requires mod_missing $this->assertFalse(in_array('mod_bar', $failedplugins)); $this->assertFalse(in_array('foolish_frog', $failedplugins)); + $this->assertFalse(in_array('foolish_hippo', $failedplugins)); $failedplugins = array(); $this->assertFalse($pluginman->all_plugins_ok(2012010100, $failedplugins)); $this->assertTrue(in_array('mod_foo', $failedplugins)); // Requires mod_missing $this->assertFalse(in_array('mod_bar', $failedplugins)); $this->assertTrue(in_array('foolish_frog', $failedplugins)); // Requires Moodle 2013010100 + $this->assertFalse(in_array('foolish_hippo', $failedplugins)); $failedplugins = array(); $this->assertFalse($pluginman->all_plugins_ok(2011010100, $failedplugins)); $this->assertTrue(in_array('mod_foo', $failedplugins)); // Requires mod_missing and Moodle 2012010100 $this->assertTrue(in_array('mod_bar', $failedplugins)); // Requires Moodle 2012010100 $this->assertTrue(in_array('foolish_frog', $failedplugins)); // Requires Moodle 2013010100 + $this->assertTrue(in_array('foolish_hippo', $failedplugins)); // Requires Moodle 2012010100 } public function test_some_plugins_updatable() { @@ -173,8 +216,9 @@ class plugin_manager_test extends advanced_testcase { public function test_get_status() { $pluginman = testable_plugin_manager::instance(); $plugins = $pluginman->get_plugins(); - $modfoo = $plugins['mod']['foo']; - $this->assertEquals($modfoo->get_status(), plugin_manager::PLUGIN_STATUS_UPGRADE); + $this->assertEquals(plugin_manager::PLUGIN_STATUS_UPGRADE, $plugins['mod']['foo']->get_status()); + $this->assertEquals(plugin_manager::PLUGIN_STATUS_NEW, $plugins['bazmeg']['one']->get_status()); + $this->assertEquals(plugin_manager::PLUGIN_STATUS_UPTODATE, $plugins['quxcat']['one']->get_status()); } public function test_available_update() { @@ -186,6 +230,27 @@ class plugin_manager_test extends advanced_testcase { $this->assertInstanceOf('available_update_info', $availableupdate); } } + + public function test_can_uninstall_plugin() { + $pluginman = testable_plugin_manager::instance(); + $this->assertFalse($pluginman->can_uninstall_plugin('mod_missing')); + $this->assertTrue($pluginman->can_uninstall_plugin('mod_foo')); // Because mod_foo is required by foolish_frog only + // and foolish_frog is required by mod_foo and foolish_hippo only. + $this->assertFalse($pluginman->can_uninstall_plugin('mod_bar')); // Because mod_bar is required by mod_foo. + $this->assertFalse($pluginman->can_uninstall_plugin('mod_qux')); // Because even if no plugin (not even subplugins) declare + // dependency on it, but its subplugin can't be uninstalled. + $this->assertFalse($pluginman->can_uninstall_plugin('mod_baz')); // Because it's subplugin bazmeg_one is required by quxcat_one. + $this->assertFalse($pluginman->can_uninstall_plugin('quxcat_one')); // Because of testable_pluginfo_quxcat::is_uninstall_allowed(). + } + + public function test_get_uninstall_url() { + $pluginman = testable_plugin_manager::instance(); + foreach ($pluginman->get_plugins() as $plugintype => $plugininfos) { + foreach ($plugininfos as $plugininfo) { + $this->assertTrue($plugininfo->get_uninstall_url() instanceof moodle_url); + } + } + } } @@ -451,6 +516,17 @@ class available_update_checker_test extends advanced_testcase { } +/** + * Base class for testable plugininfo classes. + */ +class testable_plugininfo_base extends plugininfo_base { + + protected function get_plugin_manager() { + return testable_plugin_manager::instance(); + } +} + + /** * Modified {@link plugininfo_mod} suitable for testing purposes */ @@ -471,13 +547,21 @@ class testable_plugininfo_mod extends plugininfo_mod { public function load_db_version() { $this->versiondb = 2012022900; } + + public function is_uninstall_allowed() { + return true; // Allow uninstall for standard plugins too. + } + + protected function get_plugin_manager() { + return testable_plugin_manager::instance(); + } } /** * Testable class representing subplugins of testable mod_foo */ -class testable_pluginfo_foolish extends plugininfo_base { +class testable_pluginfo_foolish extends testable_plugininfo_base { public function init_display_name() { $this->displayname = ucfirst($this->name); @@ -493,6 +577,48 @@ class testable_pluginfo_foolish extends plugininfo_base { } +/** + * Testable class representing subplugins of testable mod_baz + */ +class testable_pluginfo_bazmeg extends testable_plugininfo_base { + + public function init_display_name() { + $this->displayname = ucfirst($this->name); + } + + public function is_standard() { + return false; + } + + public function load_db_version() { + $this->versiondb = null; + } +} + + +/** + * Testable class representing subplugins of testable mod_qux + */ +class testable_pluginfo_quxcat extends testable_plugininfo_base { + + public function init_display_name() { + $this->displayname = ucfirst($this->name); + } + + public function is_standard() { + return false; + } + + public function load_db_version() { + $this->versiondb = 2013041103; + } + + public function is_uninstall_allowed() { + return false; + } +} + + /** * Modified {@link plugin_manager} suitable for testing purposes */ @@ -529,16 +655,33 @@ class testable_plugin_manager extends plugin_manager { $dirroot.'/mod/foo', 'testable_plugininfo_mod'), 'bar' => plugininfo_default_factory::make('mod', $dirroot.'/bar', 'bar', $dirroot.'/mod/bar', 'testable_plugininfo_mod'), + 'baz' => plugininfo_default_factory::make('mod', $dirroot.'/baz', 'baz', + $dirroot.'/mod/baz', 'testable_plugininfo_mod'), + 'qux' => plugininfo_default_factory::make('mod', $dirroot.'/qux', 'qux', + $dirroot.'/mod/qux', 'testable_plugininfo_mod'), ), 'foolish' => array( - 'frog' => plugininfo_default_factory::make('foolish', $dirroot.'/mod/foo/foolish', 'frog', + 'frog' => plugininfo_default_factory::make('foolish', $dirroot.'/mod/foo/lish', 'frog', $dirroot.'/mod/foo/lish/frog', 'testable_pluginfo_foolish'), + 'hippo' => plugininfo_default_factory::make('foolish', $dirroot.'/mod/foo/lish', 'hippo', + $dirroot.'/mod/foo/lish/hippo', 'testable_pluginfo_foolish'), + ), + 'bazmeg' => array( + 'one' => plugininfo_default_factory::make('bazmeg', $dirroot.'/mod/baz/meg', 'one', + $dirroot.'/mod/baz/meg/one', 'testable_pluginfo_bazmeg'), + ), + 'quxcat' => array( + 'one' => plugininfo_default_factory::make('quxcat', $dirroot.'/mod/qux/cat', 'one', + $dirroot.'/mod/qux/cat/one', 'testable_pluginfo_quxcat'), ), ); $checker = testable_available_update_checker::instance(); $this->pluginsinfo['mod']['foo']->check_available_updates($checker); $this->pluginsinfo['mod']['bar']->check_available_updates($checker); + $this->pluginsinfo['mod']['baz']->check_available_updates($checker); + $this->pluginsinfo['bazmeg']['one']->check_available_updates($checker); + $this->pluginsinfo['quxcat']['one']->check_available_updates($checker); return $this->pluginsinfo; } @@ -547,20 +690,36 @@ class testable_plugin_manager extends plugin_manager { * Testable version of {@link plugin_manager::get_subplugins()} that works with * the simulated environment. * - * In this case, the mod_foo fake module provides subplugins of type 'foolish'. + * In this case, the mod_foo fake module provides subplugins of type 'foolish', + * mod_baz provides subplugins of type 'bazmeg' and mod_qux has 'quxcat'. * * @param bool $disablecache ignored in this class * @return array */ public function get_subplugins($disablecache=false) { - return array( + + $this->subpluginsinfo = array( 'mod_foo' => array( 'foolish' => (object)array( 'type' => 'foolish', 'typerootdir' => 'mod/foo/lish', ), ), + 'mod_baz' => array( + 'bazmeg' => (object)array( + 'type' => 'bazmeg', + 'typerootdir' => 'mod/baz/meg', + ), + ), + 'mod_qux' => array( + 'quxcat' => (object)array( + 'type' => 'quxcat', + 'typerootdir' => 'mod/qux/cat', + ), + ), ); + + return $this->subpluginsinfo; } /** @@ -569,7 +728,7 @@ class testable_plugin_manager extends plugin_manager { protected function normalize_component($component) { // List of mock plugin types used in these unit tests. - $faketypes = array('foolish'); + $faketypes = array('foolish', 'bazmeg', 'quxcat'); foreach ($faketypes as $faketype) { if (strpos($component, $faketype.'_') === 0) { @@ -594,20 +753,6 @@ class testable_plugin_manager extends plugin_manager { } return false; } - - public static function standard_plugins_list($type) { - $standard_plugins = array( - 'mod' => array( - 'bar', - ), - ); - - if (isset($standard_plugins[$type])) { - return $standard_plugins[$type]; - } else { - return false; - } - } } diff --git a/lib/upgrade.txt b/lib/upgrade.txt index eedc6903c55..edd5f9b79df 100644 --- a/lib/upgrade.txt +++ b/lib/upgrade.txt @@ -53,6 +53,13 @@ information provided here is intended especially for developers. helper functions extract_suspended_users, get_suspended_userids to extract suspended user information. * The plugin_manager class now provides two new helper methods for getting information about known plugins: get_plugins_of_type() and get_subplugins_of_plugin(). +* The get_uninstall_url() method of all subclasses of plugininfo_base class is now expected + to always return moodle_url. Subclasses can use the new method is_uninstall_allowed() + to control the availability of the 'Uninstall' link at the Plugins overview page (previously + they would do it by get_uninstall_url() returning null). By default, URL to a new general plugin + uninstall tool is returned. Unless the plugin type needs extra steps that can't be handled by + plugininfo_xxx::uninstall() method or xmldb_xxx_uninstall() function, this default URL should + satisfy all plugin types. Database (DML) layer: * $DB->sql_empty() is deprecated, you have to use sql parameters with empty values instead, From 54d758939748dafd21acc4d1e899ff90f15929f6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Mudr=C3=A1k?= Date: Fri, 12 Apr 2013 01:32:21 +0200 Subject: [PATCH 11/14] MDL-39087 Improve the Plugins overview table layout This patch returns the layout of the Uninstall | Settings links to two columns. There is no space saved on the screen by using the single column and two columns align better. The reasoning for using single column was that there would be multiple links in the 'Actions' column but that does not seem to happen anytime soon. --- admin/renderer.php | 23 +++++++++++++---------- theme/base/style/admin.css | 4 +--- theme/bootstrap/less/moodle/admin.less | 5 ++--- 3 files changed, 16 insertions(+), 16 deletions(-) diff --git a/admin/renderer.php b/admin/renderer.php index c215a898e3a..5a1d2e01d3e 100644 --- a/admin/renderer.php +++ b/admin/renderer.php @@ -1160,22 +1160,23 @@ class core_admin_renderer extends plugin_renderer_base { get_string('actions', 'core_plugin'), get_string('notes','core_plugin'), ); + $table->headspan = array(1, 1, 1, 1, 2, 1); $table->colclasses = array( - 'pluginname', 'source', 'version', 'availability', 'actions', 'notes' + 'pluginname', 'source', 'version', 'availability', 'settings', 'uninstall', 'notes' ); foreach ($plugininfo as $type => $plugins) { $header = new html_table_cell($pluginman->plugintype_name_plural($type)); $header->header = true; - $header->colspan = count($table->head); + $header->colspan = array_sum($table->headspan); $header = new html_table_row(array($header)); $header->attributes['class'] = 'plugintypeheader type-' . $type; $table->data[] = $header; if (empty($plugins)) { $msg = new html_table_cell(get_string('noneinstalled', 'core_plugin')); - $msg->colspan = count($table->head); + $msg->colspan = array_sum($table->headspan); $row = new html_table_row(array($msg)); $row->attributes['class'] .= 'msg msg-noneinstalled'; $table->data[] = $row; @@ -1222,19 +1223,21 @@ class core_admin_renderer extends plugin_renderer_base { $availability = new html_table_cell(get_string('plugindisabled', 'core_plugin')); } - $actions = array(); - $settingsurl = $plugin->get_settings_url(); if (!is_null($settingsurl)) { - $actions[] = html_writer::link($settingsurl, get_string('settings', 'core_plugin'), array('class' => 'settings')); + $settings = html_writer::link($settingsurl, get_string('settings', 'core_plugin'), array('class' => 'settings')); + } else { + $settings = ''; } + $settings = new html_table_cell($settings); if ($pluginman->can_uninstall_plugin($plugin->component)) { $uninstallurl = $plugin->get_uninstall_url(); - $actions[] = html_writer::link($uninstallurl, get_string('uninstall', 'core_plugin'), array('class' => 'uninstall')); + $uninstall = html_writer::link($uninstallurl, get_string('uninstall', 'core_plugin')); + } else { + $uninstall = ''; } - - $actions = new html_table_cell(implode(html_writer::tag('span', ' ', array('class' => 'separator')), $actions)); + $uninstall = new html_table_cell($uninstall); $requriedby = $pluginman->other_plugins_that_require($plugin->component); if ($requriedby) { @@ -1254,7 +1257,7 @@ class core_admin_renderer extends plugin_renderer_base { $notes = new html_table_cell($requiredby.$updateinfo); $row->cells = array( - $pluginname, $source, $version, $availability, $actions, $notes + $pluginname, $source, $version, $availability, $settings, $uninstall, $notes ); $table->data[] = $row; } diff --git a/theme/base/style/admin.css b/theme/base/style/admin.css index e4b3adf583f..979f14179ce 100644 --- a/theme/base/style/admin.css +++ b/theme/base/style/admin.css @@ -294,8 +294,6 @@ #page-admin-plugins #plugins-control-panel .pluginname .componentname {font-size:0.8em;color:#999;margin-left:26px;} #page-admin-plugins #plugins-control-panel .missingfromdisk .pluginname {background-color:#ffd3d9;} #page-admin-plugins #plugins-control-panel .disabled .availability {background-color:#eee;} -#page-admin-plugins #plugins-control-panel .actions a {padding:0 10px;} -#page-admin-plugins #plugins-control-panel .actions .separator {border-left:1px dotted #999;} #page-admin-plugins #plugins-control-panel .extension .source {background-color:#f3f2aa;} #page-admin-plugins #plugins-control-panel .msg td {text-align:center;} #page-admin-plugins #plugins-control-panel .requiredby {font-size:0.8em;color:#999;} @@ -308,7 +306,7 @@ #page-admin-plugins #plugins-overview-filter .filter-item.active {font-weight:bold;} #page-admin-plugins #plugins-overview-filter .separator {border-left:1px dotted #999;} #page-admin-plugins #plugins-control-panel .displayname img.icon { padding-top:0; padding-bottom: 0; } -#page-admin-plugins #plugins-control-panel .actions .uninstall {color:#900;} +#page-admin-plugins #plugins-control-panel .uninstall a {color:#900;} #page-admin-plugins #plugins-control-panel .notes .pluginupdateinfo {padding:5px 10px;margin:10px;background-color:#d2ebff;-moz-border-radius: 10px;-webkit-border-radius: 10px;border-radius: 10px;} #page-admin-plugins #plugins-control-panel .notes .pluginupdateinfo.maturity50 {background-color:#ffd3d9;} #page-admin-plugins #plugins-control-panel .notes .pluginupdateinfo.maturity100, diff --git a/theme/bootstrap/less/moodle/admin.less b/theme/bootstrap/less/moodle/admin.less index 80a4bfd4a4c..abef8f2963d 100644 --- a/theme/bootstrap/less/moodle/admin.less +++ b/theme/bootstrap/less/moodle/admin.less @@ -601,8 +601,7 @@ img.iconsmall { } #plugins-overview-filter .filter-item, -#plugins-overview-panel .info, -#plugins-control-panel .actions a { +#plugins-overview-panel .info { padding: 0 10px; } @@ -638,7 +637,7 @@ img.iconsmall { padding-bottom: 0; } -#plugins-control-panel .actions .uninstall { +#plugins-control-panel .uninstall a { color: @errorText; } From ccc6c15fd2cbc2be9cb81ade1e59138f261006c8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Mudr=C3=A1k?= Date: Fri, 12 Apr 2013 03:23:47 +0200 Subject: [PATCH 12/14] MDL-39087 Fix plugin_manager::can_uninstall_plugin() implementation There was a false positive result for subplugin required by other subplugin. See the unit test. --- lib/pluginlib.php | 71 +++++++++++++++++++++--------------- lib/tests/pluginlib_test.php | 1 + 2 files changed, 42 insertions(+), 30 deletions(-) diff --git a/lib/pluginlib.php b/lib/pluginlib.php index 354cc4370cc..6c09a9d742d 100644 --- a/lib/pluginlib.php +++ b/lib/pluginlib.php @@ -487,48 +487,36 @@ class plugin_manager { return false; } - // Backwards compatibility check. - if (is_null($pluginfo->get_uninstall_url())) { - debugging('plugininfo_base subclasses should use is_uninstall_allowed() instead of returning null in get_uninstall_url()', - DEBUG_DEVELOPER); + if (!$this->common_uninstall_check($pluginfo)) { return false; } - // In case the $component has subplugins, get their list. - $mysubplugins = $this->get_subplugins_of_plugin($pluginfo->component); - - // In case the $component is a subplugin, get all subplugins of its parent (i.e. siblings). - $myparent = $this->get_parent_of_subplugin($pluginfo->type); - if ($myparent === false) { - $mysiblings = array(); - } else { - $mysiblings = $this->get_subplugins_of_plugin($myparent); - } - - // If the plugin has subplugins, check we can uninstall them first. - foreach ($mysubplugins as $subpluginfo) { - if (!$this->can_uninstall_plugin($subpluginfo->component)) { + // If it has subplugins, check they can be uninstalled too. + $subplugins = $this->get_subplugins_of_plugin($pluginfo->component); + foreach ($subplugins as $subpluginfo) { + if (!$this->common_uninstall_check($subpluginfo)) { return false; } + // Check if there are some other plugins requiring this subplugin + // (but the parent and siblings). + foreach ($this->other_plugins_that_require($subpluginfo->component) as $requiresme) { + $ismyparent = ($pluginfo->component === $requiresme); + $ismysibling = in_array($requiresme, array_keys($subplugins)); + if (!$ismyparent and !$ismysibling) { + return false; + } + } } - // Check there are no other plugins (but eventual subplugins or siblings) that - // require us. Subplugins would be uninstalled together with the parent plugin - // without the need to uninstall each of them individually. + // Check if there are some other plugins requiring this plugin + // (but its subplugins). foreach ($this->other_plugins_that_require($pluginfo->component) as $requiresme) { - $ismyparent = ($myparent === $requiresme); - $ismysubplugin = in_array($requiresme, array_keys($mysubplugins)); - $ismysibling = in_array($requiresme, array_keys($mysiblings)); - if (!$ismyparent and !$ismysubplugin and !$ismysibling) { + $ismysubplugin = in_array($requiresme, array_keys($subplugins)); + if (!$ismysubplugin) { return false; } } - // Finally give the plugin plugininfo subclass a chance to prevent uninstallation. - if (!$pluginfo->is_uninstall_allowed()) { - return false; - } - return true; } @@ -946,6 +934,29 @@ class plugin_manager { return $result; } + + /** + * Helper method that implements common uninstall prerequisities + * + * @param plugininfo_base $pluginfo + * @return bool + */ + protected function common_uninstall_check(plugininfo_base $pluginfo) { + + if (!$pluginfo->is_uninstall_allowed()) { + // The plugin's plugininfo class declares it should not be uninstalled. + return false; + } + + if (is_null($pluginfo->get_uninstall_url())) { + // Backwards compatibility. + debugging('plugininfo_base subclasses should use is_uninstall_allowed() instead of returning null in get_uninstall_url()', + DEBUG_DEVELOPER); + return false; + } + + return true; + } } diff --git a/lib/tests/pluginlib_test.php b/lib/tests/pluginlib_test.php index 0556b2ed333..06abf584075 100644 --- a/lib/tests/pluginlib_test.php +++ b/lib/tests/pluginlib_test.php @@ -241,6 +241,7 @@ class plugin_manager_test extends advanced_testcase { // dependency on it, but its subplugin can't be uninstalled. $this->assertFalse($pluginman->can_uninstall_plugin('mod_baz')); // Because it's subplugin bazmeg_one is required by quxcat_one. $this->assertFalse($pluginman->can_uninstall_plugin('quxcat_one')); // Because of testable_pluginfo_quxcat::is_uninstall_allowed(). + $this->assertFalse($pluginman->can_uninstall_plugin('foolish_frog')); // Because foolish_hippo requires it. } public function test_get_uninstall_url() { From c2d2001a145995f437daa52fabea1ab863211341 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Mudr=C3=A1k?= Date: Fri, 12 Apr 2013 03:32:35 +0200 Subject: [PATCH 13/14] MDL-39087 Improve the Plugins overview renderer As suggested by Tim Hunt during the peer-review, rendering methods should not set properties of the page they are producing HTML code for. Additionally, the page now uses correct check that the uninstalling can happen. --- admin/plugins.php | 27 ++++++++++++++++++++++----- admin/renderer.php | 9 --------- 2 files changed, 22 insertions(+), 14 deletions(-) diff --git a/admin/plugins.php b/admin/plugins.php index 35c882df511..73e34d4f352 100644 --- a/admin/plugins.php +++ b/admin/plugins.php @@ -18,6 +18,15 @@ /** * UI for general plugins management * + * Supported HTTP parameters: + * + * ?fetchremote=1 - check for available updates + * ?updatesonly=1 - display plugins with available update only + * ?contribonly=1 - display non-standard add-ons only + * ?uninstall=foo_bar - uninstall the given plugin + * ?delete=foo_bar - delete the plugin folder (it must not be installed) + * &confirm=1 - confirm the uninstall or delete action + * * @package core * @subpackage admin * @copyright 2011 David Mudrak @@ -47,16 +56,20 @@ if ($uninstall) { require_sesskey(); $pluginfo = $pluginman->get_plugin_info($uninstall); + // Make sure we know the plugin. if (is_null($pluginfo)) { throw new moodle_exception('err_uninstalling_unknown_plugin', 'core_plugin', '', array('plugin' => $uninstall), 'plugin_manager::get_plugin_info() returned null for the plugin to be uninstalled'); } - $requiredby = $pluginman->other_plugins_that_require($pluginfo->component); - if (!empty($requiredby)) { - throw new moodle_exception('err_uninstalling_required_plugin', 'core_plugin', '', - array('plugin' => $pluginfo->component, 'requiredby' => implode(', ', $requiredby)), - 'plugin_manager::other_plugins_that_require() returned non-empty array'); + $pluginname = $pluginman->plugin_name($pluginfo->component); + $PAGE->set_title($pluginname); + $PAGE->navbar->add(get_string('uninstalling', 'core_plugin', array('name' => $pluginname))); + + if (!$pluginman->can_uninstall_plugin($pluginfo->component)) { + throw new moodle_exception('err_cannot_uninstall_plugin', 'core_plugin', '', + array('plugin' => $pluginfo->component), + 'plugin_manager::can_uninstall_plugin() returned false'); } if (!$confirmed) { @@ -90,6 +103,10 @@ if ($delete and $confirmed) { 'plugin_manager::get_plugin_info() returned null for the plugin to be deleted'); } + $pluginname = $pluginman->plugin_name($pluginfo->component); + $PAGE->set_title($pluginname); + $PAGE->navbar->add(get_string('uninstalling', 'core_plugin', array('name' => $pluginname))); + // Make sure it is not installed. if (!is_null($pluginfo->versiondb)) { throw new moodle_exception('err_removing_installed_plugin', 'core_plugin', '', diff --git a/admin/renderer.php b/admin/renderer.php index 5a1d2e01d3e..ba3b65ac594 100644 --- a/admin/renderer.php +++ b/admin/renderer.php @@ -384,9 +384,6 @@ class core_admin_renderer extends plugin_renderer_base { $pluginname = $pluginman->plugin_name($pluginfo->component); - $this->page->set_title($pluginname); - $this->page->navbar->add(get_string('uninstalling', 'core_plugin', array('name' => $pluginname))); - $output .= $this->output->header(); $output .= $this->output->heading(get_string('uninstalling', 'core_plugin', array('name' => $pluginname))); $output .= $this->output->confirm(get_string('uninstallconfirm', 'core_plugin', array('name' => $pluginname)), @@ -411,9 +408,6 @@ class core_admin_renderer extends plugin_renderer_base { $pluginname = $pluginman->plugin_name($pluginfo->component); - $this->page->set_title($pluginname); - $this->page->navbar->add(get_string('uninstalling', 'core_plugin', array('name' => $pluginname))); - $output .= $this->output->header(); $output .= $this->output->heading(get_string('uninstalling', 'core_plugin', array('name' => $pluginname))); @@ -448,9 +442,6 @@ class core_admin_renderer extends plugin_renderer_base { $pluginname = $pluginman->plugin_name($pluginfo->component); - $this->page->set_title($pluginname); - $this->page->navbar->add(get_string('uninstalling', 'core_plugin', array('name' => $pluginname))); - $output .= $this->output->header(); $output .= $this->output->heading(get_string('uninstalling', 'core_plugin', array('name' => $pluginname))); From 3ca1b54642568a42c663f77af8940a59ae5c1229 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Mudr=C3=A1k?= Date: Fri, 12 Apr 2013 04:02:28 +0200 Subject: [PATCH 14/14] MDL-39087 Use progress_trace class to display uninstallation progress This is much better API than using the array passed by reference. At the moment, it is pretty hacky as it abuses text_progress_trace to output raw HTML echoed by uninstall_plugin() but that will be improved later while moving the logic out of that function into the plugin_manager. --- admin/plugins.php | 9 +++++---- admin/renderer.php | 16 ++++++---------- lib/pluginlib.php | 14 +++++++------- 3 files changed, 18 insertions(+), 21 deletions(-) diff --git a/admin/plugins.php b/admin/plugins.php index 73e34d4f352..972bf518d5a 100644 --- a/admin/plugins.php +++ b/admin/plugins.php @@ -78,16 +78,17 @@ if ($uninstall) { exit(); } else { - $messages = array(); // Collect uninstall process messages here. - $pluginman->uninstall_plugin($pluginfo->component, $messages); + $progress = new progress_trace_buffer(new text_progress_trace(), false); + $pluginman->uninstall_plugin($pluginfo->component, $progress); + $progress->finished(); if ($pluginman->is_plugin_folder_removable($pluginfo->component)) { $continueurl = new moodle_url($PAGE->url, array('delete' => $pluginfo->component, 'sesskey' => sesskey(), 'confirm' => 1)); - echo $output->plugin_uninstall_results_removable_page($pluginman, $pluginfo, $messages, $continueurl); + echo $output->plugin_uninstall_results_removable_page($pluginman, $pluginfo, $progress, $continueurl); exit(); } else { - echo $output->plugin_uninstall_results_page($pluginman, $pluginfo, $messages); + echo $output->plugin_uninstall_results_page($pluginman, $pluginfo, $progress); exit(); } } diff --git a/admin/renderer.php b/admin/renderer.php index ba3b65ac594..5f96325f6e4 100644 --- a/admin/renderer.php +++ b/admin/renderer.php @@ -398,12 +398,12 @@ class core_admin_renderer extends plugin_renderer_base { * * @param plugin_manager $pluginman * @param plugin_info $pluginfo - * @param array $messages list of strings, the log of the process + * @param progress_trace_buffer $progress * @param moodle_url $continueurl URL to continue to remove the plugin folder * @return string */ public function plugin_uninstall_results_removable_page(plugin_manager $pluginman, plugininfo_base $pluginfo, - array $messages = array(), moodle_url $continueurl) { + progress_trace_buffer $progress, moodle_url $continueurl) { $output = ''; $pluginname = $pluginman->plugin_name($pluginfo->component); @@ -411,9 +411,7 @@ class core_admin_renderer extends plugin_renderer_base { $output .= $this->output->header(); $output .= $this->output->heading(get_string('uninstalling', 'core_plugin', array('name' => $pluginname))); - foreach ($messages as $message) { - $output .= $this->output->box($message, 'generalbox uninstallresultmessage'); - } + $output .= $this->output->box($progress->get_buffer(), 'generalbox uninstallresultmessage'); $confirm = $this->output->container(get_string('uninstalldeleteconfirm', 'core_plugin', array('name' => $pluginname, 'rootdir' => $pluginfo->rootdir)), 'uninstalldeleteconfirm'); @@ -434,10 +432,10 @@ class core_admin_renderer extends plugin_renderer_base { * * @param plugin_manager $pluginman * @param plugin_info $pluginfo - * @param array $messages list of strings, the log of the process + * @param progress_trace_buffer $progress * @return string */ - public function plugin_uninstall_results_page(plugin_manager $pluginman, plugininfo_base $pluginfo, array $messages = array()) { + public function plugin_uninstall_results_page(plugin_manager $pluginman, plugininfo_base $pluginfo, progress_trace_buffer $progress) { $output = ''; $pluginname = $pluginman->plugin_name($pluginfo->component); @@ -445,9 +443,7 @@ class core_admin_renderer extends plugin_renderer_base { $output .= $this->output->header(); $output .= $this->output->heading(get_string('uninstalling', 'core_plugin', array('name' => $pluginname))); - foreach ($messages as $message) { - $output .= $this->output->box($message, 'generalbox uninstallresultmessage'); - } + $output .= $this->output->box($progress->get_buffer(), 'generalbox uninstallresultmessage'); $output .= $this->output->box(get_string('uninstalldelete', 'core_plugin', array('name' => $pluginname, 'rootdir' => $pluginfo->rootdir)), 'generalbox uninstalldelete'); diff --git a/lib/pluginlib.php b/lib/pluginlib.php index 6c09a9d742d..c35d2eaf05c 100644 --- a/lib/pluginlib.php +++ b/lib/pluginlib.php @@ -531,10 +531,10 @@ class plugin_manager { * mimic this future behaviour by wrapping that function call. * * @param string $component - * @param array $messages log of the process is returned via this array + * @param progress_trace $progress traces the process * @return bool true on success, false on errors/problems */ - public function uninstall_plugin($component, array &$messages) { + public function uninstall_plugin($component, progress_trace $progress) { $pluginfo = $this->get_plugin_info($component); @@ -542,8 +542,8 @@ class plugin_manager { return false; } - // Give the pluginfo class a perform some steps. - $result = $pluginfo->uninstall($messages); + // Give the pluginfo class a chance to execute some steps. + $result = $pluginfo->uninstall($progress); if (!$result) { return false; } @@ -551,7 +551,7 @@ class plugin_manager { // Call the legacy core function to uninstall the plugin. ob_start(); uninstall_plugin($pluginfo->type, $pluginfo->name); - $messages[] = ob_get_clean(); + $progress->output(ob_get_clean()); return true; } @@ -2859,10 +2859,10 @@ abstract class plugininfo_base { * it is basically usable only for those plugin types that use the default * uninstall tool provided by {@link self::get_default_uninstall_url()}. * - * @param array $messages list of uninstall log messages + * @param progress_trace $progress traces the process * @return bool true on success, false on failure */ - public function uninstall(array &$messages) { + public function uninstall(progress_trace $progress) { return true; }