resourceloader: Skip modules with circular deps in tree optimiser

Either the server needs to omit these from the registry with
state=error output to the client (and server-side error logging),
or it needs to detect them, and transport them unchanged, so that
the existing client-side logic can handle it.

This patch does the latter.

Without the source code change in this patch, the added test case
fails due to "top" and "middle1" then being registered with
an empty array as dependencies.

Bug: T223402
Change-Id: I57502d7c4e434de4737759aed325dd4200ca89bf

diff --git a/autoload.php b/autoload.php
index ae044f4..2d1e175 100644 (file)
--- a/autoload.php
+++ b/autoload.php
@@ -1235,6 +1235,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}"
+    ]
 ] );',
                        ] ],
                        [ [