Merge "resourceloader: Skip modules with circular deps in tree optimiser"

diff --git a/autoload.php b/autoload.php
index b3900a8..43440f0 100644 (file)
--- a/autoload.php
+++ b/autoload.php
@@ -1231,6 +1231,7 @@ $wgAutoloadLocalClasses = [
        'ResetUserTokens' => __DIR__ . '/maintenance/resetUserTokens.php',
        'ResourceFileCache' => __DIR__ . '/includes/cache/ResourceFileCache.php',
        'ResourceLoader' => __DIR__ . '/includes/resourceloader/ResourceLoader.php',
+       'ResourceLoaderCircularDependencyError' => __DIR__ . '/includes/resourceloader/ResourceLoaderCircularDependencyError.php',
        'ResourceLoaderClientHtml' => __DIR__ . '/includes/resourceloader/ResourceLoaderClientHtml.php',
        'ResourceLoaderContext' => __DIR__ . '/includes/resourceloader/ResourceLoaderContext.php',
        'ResourceLoaderFileModule' => __DIR__ . '/includes/resourceloader/ResourceLoaderFileModule.php',

diff --git a/includes/resourceloader/ResourceLoaderCircularDependencyError.php b/includes/resourceloader/ResourceLoaderCircularDependencyError.php
new file mode 100644 (file)
index 0000000..7cd53fe
--- /dev/null
+++ b/includes/resourceloader/ResourceLoaderCircularDependencyError.php
@@ -0,0 +1,26 @@
+<?php
+/**
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
+ * http://www.gnu.org/copyleft/gpl.html
+ *
+ * @file
+ */
+
+/**
+ * @internal For use by ResourceLoaderStartUpModule only
+ */
+class ResourceLoaderCircularDependencyError extends Exception {
+}

diff --git a/includes/resourceloader/ResourceLoaderStartUpModule.php b/includes/resourceloader/ResourceLoaderStartUpModule.php
index efed418..4ce4498 100644 (file)
--- a/includes/resourceloader/ResourceLoaderStartUpModule.php
+++ b/includes/resourceloader/ResourceLoaderStartUpModule.php
@@ -124,29 +124,50 @@ class ResourceLoaderStartUpModule extends ResourceLoaderModule {
         *
         * @param array $registryData
         * @param string $moduleName
+        * @param string[] $handled Internal parameter for recursion. (Optional)
         * @return array
+        * @throws ResourceLoaderCircularDependencyError
         */
-       protected static function getImplicitDependencies( array $registryData, $moduleName ) {
+       protected static function getImplicitDependencies(
+               array $registryData,
+               $moduleName,
+               array $handled = []
+       ) {
                static $dependencyCache = [];
 
-               // The list of implicit dependencies won't be altered, so we can
-               // cache them without having to worry.
+               // No modules will be added or changed server-side after this point,
+               // so we can safely cache parts of the tree for re-use.
                if ( !isset( $dependencyCache[$moduleName] ) ) {
                        if ( !isset( $registryData[$moduleName] ) ) {
-                               // Dependencies may not exist
-                               $dependencyCache[$moduleName] = [];
+                               // Unknown module names are allowed here, this is only an optimisation.
+                               // Checks for illegal and unknown dependencies happen as PHPUnit structure tests,
+                               // and also client-side at run-time.
+                               $flat = [];
                        } else {
                                $data = $registryData[$moduleName];
-                               $dependencyCache[$moduleName] = $data['dependencies'];
+                               $flat = $data['dependencies'];
 
+                               // Prevent recursion
+                               $handled[] = $moduleName;
                                foreach ( $data['dependencies'] as $dependency ) {
-                                       // Recursively get the dependencies of the dependencies
-                                       $dependencyCache[$moduleName] = array_merge(
-                                               $dependencyCache[$moduleName],
-                                               self::getImplicitDependencies( $registryData, $dependency )
-                                       );
+                                       if ( in_array( $dependency, $handled, true ) ) {
+                                               // If we encounter a circular dependency, then stop the optimiser and leave the
+                                               // original dependencies array unmodified. Circular dependencies are not
+                                               // supported in ResourceLoader. Awareness of them exists here so that we can
+                                               // optimise the registry when it isn't broken, and otherwise transport the
+                                               // registry unchanged. The client will handle this further.
+                                               throw new ResourceLoaderCircularDependencyError();
+                                       } else {
+                                               // Recursively add the dependencies of the dependencies
+                                               $flat = array_merge(
+                                                       $flat,
+                                                       self::getImplicitDependencies( $registryData, $dependency, $handled )
+                                               );
+                                       }
                                }
                        }
+
+                       $dependencyCache[$moduleName] = $flat;
                }
 
                return $dependencyCache[$moduleName];
@@ -173,10 +194,16 @@ class ResourceLoaderStartUpModule extends ResourceLoaderModule {
        public static function compileUnresolvedDependencies( array &$registryData ) {
                foreach ( $registryData as $name => &$data ) {
                        $dependencies = $data['dependencies'];
-                       foreach ( $data['dependencies'] as $dependency ) {
-                               $implicitDependencies = self::getImplicitDependencies( $registryData, $dependency );
-                               $dependencies = array_diff( $dependencies, $implicitDependencies );
+                       try {
+                               foreach ( $data['dependencies'] as $dependency ) {
+                                       $implicitDependencies = self::getImplicitDependencies( $registryData, $dependency );
+                                       $dependencies = array_diff( $dependencies, $implicitDependencies );
+                               }
+                       } catch ( ResourceLoaderCircularDependencyError $err ) {
+                               // Leave unchanged
+                               $dependencies = $data['dependencies'];
                        }
+
                        // Rebuild keys
                        $data['dependencies'] = array_values( $dependencies );
                }

diff --git a/tests/phpunit/includes/resourceloader/ResourceLoaderStartUpModuleTest.php b/tests/phpunit/includes/resourceloader/ResourceLoaderStartUpModuleTest.php
index b5dd008..99f5e1b 100644 (file)
--- a/tests/phpunit/includes/resourceloader/ResourceLoaderStartUpModuleTest.php
+++ b/tests/phpunit/includes/resourceloader/ResourceLoaderStartUpModuleTest.php
@@ -105,6 +105,83 @@ mw.loader.register( [
         "c",
         "{blankVer}"
     ]
+] );',
+                       ] ],
+                       [ [
+                               // Regression test for T223402.
+                               'msg' => 'Optimise the dependency tree (indirect circular dependency)',
+                               'modules' => [
+                                       'top' => new ResourceLoaderTestModule( [ 'dependencies' => [ 'middle1', 'util' ] ] ),
+                                       'middle1' => new ResourceLoaderTestModule( [ 'dependencies' => [ 'middle2', 'util' ] ] ),
+                                       'middle2' => new ResourceLoaderTestModule( [ 'dependencies' => [ 'bottom' ] ] ),
+                                       'bottom' => new ResourceLoaderTestModule( [ 'dependencies' => [ 'top' ] ] ),
+                                       'util' => new ResourceLoaderTestModule( [ 'dependencies' => [] ] ),
+                               ],
+                               'out' => '
+mw.loader.addSource( {
+    "local": "/w/load.php"
+} );
+mw.loader.register( [
+    [
+        "top",
+        "{blankVer}",
+        [
+            1,
+            4
+        ]
+    ],
+    [
+        "middle1",
+        "{blankVer}",
+        [
+            2,
+            4
+        ]
+    ],
+    [
+        "middle2",
+        "{blankVer}",
+        [
+            3
+        ]
+    ],
+    [
+        "bottom",
+        "{blankVer}",
+        [
+            0
+        ]
+    ],
+    [
+        "util",
+        "{blankVer}"
+    ]
+] );',
+                       ] ],
+                       [ [
+                               // Regression test for T223402.
+                               'msg' => 'Optimise the dependency tree (direct circular dependency)',
+                               'modules' => [
+                                       'top' => new ResourceLoaderTestModule( [ 'dependencies' => [ 'util', 'top' ] ] ),
+                                       'util' => new ResourceLoaderTestModule( [ 'dependencies' => [] ] ),
+                               ],
+                               'out' => '
+mw.loader.addSource( {
+    "local": "/w/load.php"
+} );
+mw.loader.register( [
+    [
+        "top",
+        "{blankVer}",
+        [
+            1,
+            0
+        ]
+    ],
+    [
+        "util",
+        "{blankVer}"
+    ]
 ] );',
                        ] ],
                        [ [