Make sure plugins in config.ini.php are sorted depending on dependencies (#11683)

* fix #11681 make sure plugins in config.ini.php are sorted depending on dependencies

* added comment

* add missing tests

core/Application/Kernel/PluginList.php

@@ -8,6 +8,8 @@
 
 namespace Piwik\Application\Kernel;
 
+use Piwik\Plugin\MetadataLoader;
+
 /**
  * Lists the currently activated plugins. Used when setting up Piwik's environment before
  * initializing the DI container.
@@ -88,7 +90,8 @@ class PluginList
     }
 
     /**
-     * Sorts an array of plugins in the order they should be loaded.
+     * Sorts an array of plugins in the order they should be loaded. We cannot use DI here as DI is not initialized
+     * at this stage.
      *
      * @params string[] $plugins
      * @return \string[]
@@ -111,10 +114,78 @@ class PluginList
         $otherPluginsToLoadAfterDefaultPlugins = array_diff($plugins, $defaultPluginsLoadedFirst);
 
         // sort by name to have a predictable order for those extra plugins
-        sort($otherPluginsToLoadAfterDefaultPlugins);
+        natcasesort($otherPluginsToLoadAfterDefaultPlugins);
 
         $sorted = array_merge($defaultPluginsLoadedFirst, $otherPluginsToLoadAfterDefaultPlugins);
 
         return $sorted;
     }
+
+    /**
+     * Sorts an array of plugins in the order they should be saved in config.ini.php. This basically influences
+     * the order of the plugin config.php and which config will be loaded first. We want to make sure to require the
+     * config or a required plugin first before loading the plugin that requires it.
+     *
+     * We do not sort using this logic on each request since it is much slower than `sortPlugins()`. The order
+     * of plugins in config.ini.php is only important for the ContainerFactory. During a regular request it is otherwise
+     * fine to load the plugins in the order of `sortPlugins()` since we will make sure that required plugins will be
+     * loaded first in plugin manager.
+     *
+     * @param string[] $plugins
+     * @param array[] $pluginJsonCache  For internal testing only
+     * @return \string[]
+     */
+    public function sortPluginsAndRespectDependencies(array $plugins, $pluginJsonCache = array())
+    {
+        $global = $this->getPluginsBundledWithPiwik();
+
+        if (empty($global)) {
+            return $plugins;
+        }
+
+        // we need to make sure a possibly disabled plugin will be still loaded before any 3rd party plugin
+        $global = array_merge($global, $this->corePluginsDisabledByDefault);
+
+        $global = array_values($global);
+        $plugins = array_values($plugins);
+
+        $defaultPluginsLoadedFirst = array_intersect($global, $plugins);
+
+        $otherPluginsToLoadAfterDefaultPlugins = array_diff($plugins, $defaultPluginsLoadedFirst);
+
+        // we still want to sort alphabetically by default
+        natcasesort($otherPluginsToLoadAfterDefaultPlugins);
+
+        $sorted = array();
+        foreach ($otherPluginsToLoadAfterDefaultPlugins as $pluginName) {
+            $sorted = $this->sortRequiredPlugin($pluginName, $pluginJsonCache, $otherPluginsToLoadAfterDefaultPlugins, $sorted);
+        }
+
+        $sorted = array_merge($defaultPluginsLoadedFirst, $sorted);
+
+        return $sorted;
+    }
+
+    private function sortRequiredPlugin($pluginName, &$pluginJsonCache, $toBeSorted, $sorted)
+    {
+        if (!isset($pluginJsonCache[$pluginName])) {
+            $loader = new MetadataLoader($pluginName);
+            $pluginJsonCache[$pluginName] = $loader->loadPluginInfoJson();
+        }
+
+        if (!empty($pluginJsonCache[$pluginName]['require'])) {
+            $dependencies = $pluginJsonCache[$pluginName]['require'];
+            foreach ($dependencies as $possiblePluginName => $key) {
+                if (in_array($possiblePluginName, $toBeSorted, true) && !in_array($possiblePluginName, $sorted, true)) {
+                    $sorted = $this->sortRequiredPlugin($possiblePluginName, $pluginJsonCache, $toBeSorted, $sorted);
+                }
+            }
+        }
+
+        if (!in_array($pluginName, $sorted, true)) {
+            $sorted[] = $pluginName;
+        }
+
+        return $sorted;
+    }
 }

core/Plugin/Manager.php

@@ -203,7 +203,7 @@ class Manager
      */
     private function updatePluginsConfig($pluginsToLoad)
     {
-        $pluginsToLoad = $this->pluginList->sortPlugins($pluginsToLoad);
+        $pluginsToLoad = $this->pluginList->sortPluginsAndRespectDependencies($pluginsToLoad);
         $section = PiwikConfig::getInstance()->Plugins;
         $section['Plugins'] = $pluginsToLoad;
         PiwikConfig::getInstance()->Plugins = $section;

core/Plugin/MetadataLoader.php

@@ -11,6 +11,7 @@ namespace Piwik\Plugin;
 use Exception;
 use Piwik\Piwik;
 use Piwik\Version;
+use Piwik\Plugin;
 
 /**
  * @see core/Version.php
@@ -90,12 +91,22 @@ class MetadataLoader
         );
     }
 
-    private function loadPluginInfoJson()
+    /**
+     * It is important that this method works without using anything from DI
+     * @return array|mixed
+     */
+    public function loadPluginInfoJson()
     {
-        $path = $this->getPathToPluginFolder() . '/' . self::PLUGIN_JSON_FILENAME;
+        $path = $this->getPathToPluginJson();
         return $this->loadJsonMetadata($path);
     }
 
+    public function getPathToPluginJson()
+    {
+        $path = $this->getPathToPluginFolder() . '/' . self::PLUGIN_JSON_FILENAME;
+        return $path;
+    }
+
     private function loadJsonMetadata($path)
     {
         if (!file_exists($path)) {

tests/PHPUnit/Framework/TestingEnvironmentManipulator.php

@@ -227,16 +227,15 @@ class TestingEnvironmentManipulator implements EnvironmentManipulator
 
     private function getPluginAndRequiredPlugins($pluginName, $plugins)
     {
-        $pluginJsonPath = $this->makePathToPluginJson($pluginName);
+        $pluginLoader = new Plugin\MetadataLoader($pluginName);
+        $pluginJson = $pluginLoader->loadPluginInfoJson();
 
-        if (file_exists($pluginJsonPath)) {
-            $pluginJson = json_decode(trim(file_get_contents($pluginJsonPath)), true);
+        if (!empty($pluginJson['require'])) {
+            foreach ($pluginJson['require'] as $possiblePluginName => $requiredVersion) {
 
-            if (!empty($pluginJson['require'])) {
-                foreach ($pluginJson['require'] as $possiblePluginName => $requiredVersion) {
-                    if (file_exists($this->makePathToPluginJson($possiblePluginName))) {
-                        $plugins = $this->getPluginAndRequiredPlugins($possiblePluginName, $plugins);
-                    }
+                $pluginLoader2 = new Plugin\MetadataLoader($possiblePluginName);
+                if (file_exists($pluginLoader2->getPathToPluginJson())) {
+                    $plugins = $this->getPluginAndRequiredPlugins($possiblePluginName, $plugins);
                 }
             }
         }
@@ -248,11 +247,6 @@ class TestingEnvironmentManipulator implements EnvironmentManipulator
         return $plugins;
     }
 
-    private function makePathToPluginJson($pluginName)
-    {
-        return Plugin\Manager::getPluginsDirectory() . $pluginName . '/' . Plugin\MetadataLoader::PLUGIN_JSON_FILENAME;
-    }
-
     private function classExists($klass)
     {
         if (class_exists($klass)) {

tests/PHPUnit/Integration/Application/Kernel/PluginListTest.php

+<?php
+/**
+ * Piwik - free/libre analytics platform
+ *
+ * @link    http://piwik.org
+ * @license http://www.gnu.org/licenses/gpl-3.0.html GPL v3 or later
+ */
+namespace Piwik\Tests\Integration\Application\Kernel;
+
+use Piwik\Application\Kernel\PluginList;
+use Piwik\Container\StaticContainer;
+use Piwik\Tests\Framework\TestCase\IntegrationTestCase;
+
+/**
+ * @group PluginListTest
+ * @group Core
+ */
+class PluginListTest extends IntegrationTestCase
+{
+
+    /**
+     * @var PluginList
+     */
+    private $pluginList = array();
+
+    public function setUp()
+    {
+        parent::setUp();
+        $this->pluginList = $this->makePluginList();
+    }
+
+    public function test_sortPlugins()
+    {
+        $pluginList = $this->makePluginList();
+        $sorted = $pluginList->sortPlugins(array('UsersManager', 'CoreHome', 'MyCustomPlugin', 'ExampleCommand', 'MyCustomPlugin2', 'Abcdef'));
+        $this->assertSame(array(
+            'CoreHome', // core plugins loaded first
+            'UsersManager',
+            'ExampleCommand', // a "by default disabled plugin" is loaded before custom plugins
+            'Abcdef', // then we load custom plugins
+            'MyCustomPlugin',
+            'MyCustomPlugin2',
+        ), $sorted);
+    }
+
+    public function test_sortPlugins_onlyCorePlugins()
+    {
+        $pluginList = $this->makePluginList();
+        $sorted = $pluginList->sortPlugins(array('UsersManager', 'CoreHome'));
+        $this->assertSame(array('CoreHome','UsersManager'), $sorted);
+    }
+
+    public function test_sortPluginsAndRespectDependencies_sortsPluginsAlphabetically()
+    {
+        $pluginList = $this->makePluginList();
+        $sorted = $pluginList->sortPluginsAndRespectDependencies(array(
+            'UsersManager', 'MyCustomPlugin', 'ExampleCommand', 'MyCustomPlugin2', 'CoreHome', 'Abcdef'
+        ));
+        $this->assertSame(array(
+            'CoreHome', // core plugins loaded first
+            'UsersManager',
+            'ExampleCommand', // a "by default disabled plugin" is loaded before custom plugins
+            'Abcdef', // then we load custom plugins
+            'MyCustomPlugin',
+            'MyCustomPlugin2',
+        ), $sorted);
+    }
+
+    public function test_sortPluginsAndRespectDependencies_makesSureToListRequiredDependencyFirst()
+    {
+        $pluginJsonInfo = array(
+            'Abcdef' => array('require' => array('MyCustomPlugin2' => '2.2.1')),
+            'MyCustomPlugin2' => array('require' => array('CoreHome' => '4.2.1', 'MyCustomPlugin3' => '3.0.3')),
+            'fooBar' => array('require' => array('Ast' => '1.2.1', 'MyCustomPlugin3' => '3.0.3'))
+        );
+
+        $pluginList = $this->makePluginList();
+        $sorted = $pluginList->sortPluginsAndRespectDependencies(array(
+            'UsersManager', 'MyCustomPlugin',
+            'ExampleCommand', 'MyCustomPlugin2', 'Ast',
+            'Acc', 'MyCustomPlugin3', 'CoreHome', 'Abcdef', 'fooBar',
+        ), $pluginJsonInfo);
+        $this->assertSame(array(
+            'CoreHome', // core plugins loaded first
+            'UsersManager',
+            'ExampleCommand', // a "by default disabled plugin" is loaded before custom plugins
+            'MyCustomPlugin3',
+            'MyCustomPlugin2',
+            'Abcdef',
+            'Acc',
+            'Ast',
+            'fooBar',
+            'MyCustomPlugin',
+        ), $sorted);
+    }
+
+    public function test_sortPluginsAndRespectDependencies_onlyCorePlugins()
+    {
+        $pluginList = $this->makePluginList();
+        $sorted = $pluginList->sortPluginsAndRespectDependencies(array('UsersManager', 'CoreHome'));
+        $this->assertSame(array('CoreHome','UsersManager'), $sorted);
+    }
+
+    private function makePluginList()
+    {
+        $globalSettingsProvider = StaticContainer::get('Piwik\Application\Kernel\GlobalSettingsProvider');
+        $section = $globalSettingsProvider->getSection('Plugins');
+       // $section['Plugins'] = $pluginsToLoad;
+        $globalSettingsProvider->setSection('Plugins', $section);
+        return new PluginList($globalSettingsProvider);
+    }
+
+}