MDL-49329 admin: Improve the plugin package validator
authorDavid Mudrák <david@moodle.com>
Wed, 7 Oct 2015 09:50:49 +0000 (11:50 +0200)
committerDavid Mudrák <david@moodle.com>
Thu, 8 Oct 2015 21:32:05 +0000 (23:32 +0200)
Previously, the validator was used for installation of the new plugins
only. We now validate every incoming plugin package. As a result, we
must no longer insists on the target location is empty. Instead, warning
is raised.

lang/en/plugin.php
lib/classes/plugin_manager.php
lib/classes/update/validator.php
lib/tests/fixtures/testable_update_validator.php
lib/tests/update_validator_test.php

index 19fe706..189d817 100644 (file)
@@ -201,8 +201,9 @@ $string['validationmsg_rootdir'] = 'Name of the plugin to be installed';
 $string['validationmsg_rootdir_help'] = 'The name of the root directory in the ZIP package forms the name of the plugin to be installed. If the name is not correct, you may wish to rename the root directory in the ZIP prior to installing the plugin.';
 $string['validationmsg_rootdirinvalid'] = 'Invalid plugin name';
 $string['validationmsg_rootdirinvalid_help'] = 'The name of the root directory in the ZIP package violates formal syntax requirements. Some ZIP packages, such as those generated by Github, may contain an incorrect root directory name. You need to fix the name of the root directory to match the plugin name.';
-$string['validationmsg_targetexists'] = 'Target location already exists';
-$string['validationmsg_targetexists_help'] = 'The directory that the plugin is to be installed to must not yet exist.';
+$string['validationmsg_targetexists'] = 'Target location already exists and will be removed';
+$string['validationmsg_targetexists_help'] = 'The plugin directory already exists and will be replaced by the plugin package contents.';
+$string['validationmsg_targetnotdir'] = 'Target location occupied by a file';
 $string['validationmsg_unknowntype'] = 'Unknown plugin type';
 $string['validationmsg_versionphpsyntax'] = 'Unsupported syntax detected in version.php file';
 $string['validationmsglevel_debug'] = 'Debug';
index 860af8a..7c4e091 100644 (file)
@@ -1807,7 +1807,7 @@ class core_plugin_manager {
      * @param string $fullpath
      * @return boolean
      */
-    protected function is_directory_removable($fullpath) {
+    public function is_directory_removable($fullpath) {
 
         if (!is_writable($fullpath)) {
             return false;
index 64c13cb..e4e2a94 100644 (file)
@@ -29,6 +29,7 @@
 namespace core\update;
 
 use core_component;
+use core_plugin_manager;
 use help_icon;
 use coding_exception;
 
@@ -474,18 +475,31 @@ class validator {
             throw new coding_exception('Plugin type location does not exist!');
         }
 
-        $target = $plugintypepath.'/'.$this->rootdir;
-
-        if (file_exists($target)) {
-            $this->add_message(self::ERROR, 'targetexists', $target);
+        // Always check that the plugintype root is writable.
+        if (!is_writable($plugintypepath)) {
+            $this->add_message(self::ERROR, 'pathwritable', $plugintypepath);
             return false;
+        } else {
+            $this->add_message(self::INFO, 'pathwritable', $plugintypepath);
         }
 
-        if (is_writable($plugintypepath)) {
-            $this->add_message(self::INFO, 'pathwritable', $plugintypepath);
-        } else {
-            $this->add_message(self::ERROR, 'pathwritable', $plugintypepath);
-            return false;
+        // The target location itself may or may not exist. Even if installing an
+        // available update, the code could have been removed by accident (and
+        // be reported as missing) etc. So we just make sure that the code
+        // can be replaced if it already exists.
+        $target = $plugintypepath.'/'.$this->rootdir;
+        if (file_exists($target)) {
+            if (!is_dir($target)) {
+                $this->add_message(self::ERROR, 'targetnotdir', $target);
+                return false;
+            }
+            $this->add_message(self::WARNING, 'targetexists', $target);
+            if ($this->get_plugin_manager()->is_directory_removable($target)) {
+                $this->add_message(self::INFO, 'pathwritable', $target);
+            } else {
+                $this->add_message(self::ERROR, 'pathwritable', $target);
+                return false;
+            }
         }
 
         return true;
@@ -599,16 +613,13 @@ class validator {
      * @return string|null
      */
     public function get_plugintype_location($plugintype) {
+        return $this->get_plugin_manager()->get_plugintype_root($plugintype);
+    }
 
-        $plugintypepath = null;
-
-        foreach (core_component::get_plugin_types() as $type => $fullpath) {
-            if ($type === $plugintype) {
-                $plugintypepath = $fullpath;
-                break;
-            }
-        }
-
-        return $plugintypepath;
+    /**
+     * @return core_plugin_manager
+     */
+    protected function get_plugin_manager() {
+        return core_plugin_manager::instance();
     }
 }
index 8d75a77..5c8edcd 100644 (file)
@@ -40,15 +40,10 @@ class testable_core_update_validator extends \core\update\validator {
     public function get_plugintype_location($plugintype) {
 
         $testableroot = make_temp_directory('testable_core_update_validator/plugintypes');
-        if (!is_dir($testableroot.'/'.$plugintype)) {
+        if (!file_exists($testableroot.'/'.$plugintype)) {
             make_temp_directory('testable_core_update_validator/plugintypes/'.$plugintype);
         }
 
-        if ($plugintype === 'local') {
-            // We need the following for the test_validate_target_location() method
-            make_temp_directory('testable_core_update_validator/plugintypes/local/greenbar');
-        }
-
         return $testableroot.'/'.$plugintype;
     }
 }
index dfb46d3..f8342a9 100644 (file)
@@ -277,10 +277,26 @@ class core_update_validator_testcase extends advanced_testcase {
             'greenbar/lang/en/local_greenbar.php' => true));
         $validator->assert_plugin_type('local');
         $validator->assert_moodle_version('2013031400.00');
+
+        $this->assertTrue($validator->execute());
+        $this->assertFalse($this->has_message($validator->get_messages(), $validator::WARNING, 'targetexists',
+            $validator->get_plugintype_location('local').'/greenbar'));
+
+        $typeroot = $validator->get_plugintype_location('local');
+        make_writable_directory($typeroot.'/greenbar');
+        $this->assertTrue($validator->execute());
+        $this->assertTrue($this->has_message($validator->get_messages(), $validator::WARNING, 'targetexists',
+            $validator->get_plugintype_location('local').'/greenbar'));
+
+        remove_dir($typeroot.'/greenbar');
+        file_put_contents($typeroot.'/greenbar', 'This file occupies a place where a plugin should land.');
         $this->assertFalse($validator->execute());
-        $this->assertTrue($this->has_message($validator->get_messages(), $validator::ERROR, 'targetexists',
+        $this->assertTrue($this->has_message($validator->get_messages(), $validator::ERROR, 'targetnotdir',
             $validator->get_plugintype_location('local').'/greenbar'));
 
+        unlink($typeroot.'/greenbar');
+        $this->assertTrue($validator->execute());
+
         $validator = testable_core_update_validator::instance($fixtures.'/plugindir', array(
             'foobar/' => true,
             'foobar/version.php' => true,