From 7ec1317b1bb42bbbeaff3cf623a5dc6570a468c8 Mon Sep 17 00:00:00 2001 From: Jonas Meurer Date: Wed, 15 Dec 2021 13:41:36 +0100 Subject: [PATCH 1/3] Sort app scripts by dependencies (Fixes: #30278) Signed-off-by: Jonas Meurer --- lib/public/Util.php | 32 ++++++++++++++++++++++++++++---- 1 file changed, 28 insertions(+), 4 deletions(-) diff --git a/lib/public/Util.php b/lib/public/Util.php index d0b23ddd3e0..c22ee95563c 100644 --- a/lib/public/Util.php +++ b/lib/public/Util.php @@ -78,6 +78,9 @@ class Util { /** @var array */ private static $scripts = []; + /** @var array */ + private static $scriptDeps = []; + /** * get the current installed version of Nextcloud * @return array @@ -216,13 +219,15 @@ class Util { // ] // ] if (!empty($afterAppId)) { - // init afterAppId app array if it doesn't exists + // init afterAppId app array if it doesn't exist if (!array_key_exists($afterAppId, self::$scripts)) { self::$scripts[$afterAppId] = ['first' => [], 'last' => []]; } self::$scripts[$afterAppId]['last'][] = $path; + // store dependency + self::$scriptDeps[$application] = $afterAppId; } else { - // init app array if it doesn't exists + // init app array if it doesn't exist if (!array_key_exists($application, self::$scripts)) { self::$scripts[$application] = ['first' => [], 'last' => []]; } @@ -237,12 +242,31 @@ class Util { */ public static function getScripts(): array { // merging first and last data set - $mapFunc = function (array $scriptsArray): array { + $mapFunc = static function (array $scriptsArray): array { return array_merge(...array_values($scriptsArray)); }; $appScripts = array_map($mapFunc, self::$scripts); + + // Sort by dependency if any + $sortedScripts = []; + foreach ($appScripts as $app => $scripts) { + if (array_key_exists($app, self::$scriptDeps)) { + $dep = self::$scriptDeps[$app]; + if (!array_key_exists($dep, $sortedScripts)) { + $sortedScripts[$dep] = []; + } + array_push($sortedScripts[$dep], ...$scripts); + } else { + if (!array_key_exists($app, $sortedScripts)) { + $sortedScripts[$app] = []; + } + array_unshift($sortedScripts[$app], ...$scripts); + } + } + // sort core first - $scripts = array_merge(isset($appScripts['core']) ? $appScripts['core'] : [], ...array_values($appScripts)); + $scripts = array_merge(isset($sortedScripts['core']) ? $sortedScripts['core'] : [], ...array_values($sortedScripts)); + // remove duplicates return array_unique($scripts); } From 8dd119402a19d5ce131cc68340e9db9f0ebdd382 Mon Sep 17 00:00:00 2001 From: Jonas Meurer Date: Fri, 17 Dec 2021 13:26:43 +0100 Subject: [PATCH 2/3] Improve unit testing for Util::addScript() and Util::getScript() Instead of checking for a predefined order of the scripts, test the logic: core first, dependencies before their children, no duplicates and all scripts still listed. Signed-off-by: Jonas Meurer --- tests/lib/UtilTest.php | 56 +++++++++++++++++++++++++++++++++++++++--- 1 file changed, 52 insertions(+), 4 deletions(-) diff --git a/tests/lib/UtilTest.php b/tests/lib/UtilTest.php index 1b430fd7ecd..1b244d29dd5 100644 --- a/tests/lib/UtilTest.php +++ b/tests/lib/UtilTest.php @@ -238,22 +238,70 @@ class UtilTest extends \Test\TestCase { public function testAddScript() { \OCP\Util::addScript('core', 'myFancyJSFile1'); \OCP\Util::addScript('files', 'myFancyJSFile2', 'core'); + \OCP\Util::addScript('myApp5', 'myApp5JSFile', 'myApp2'); \OCP\Util::addScript('myApp', 'myFancyJSFile3'); \OCP\Util::addScript('core', 'myFancyJSFile4'); // after itself \OCP\Util::addScript('core', 'myFancyJSFile5', 'core'); // add duplicate \OCP\Util::addScript('core', 'myFancyJSFile1'); + // dependency chain + \OCP\Util::addScript('myApp4', 'myApp4JSFile', 'myApp3'); + \OCP\Util::addScript('myApp3', 'myApp3JSFile', 'myApp2'); + \OCP\Util::addScript('myApp2', 'myApp2JSFile', 'myApp'); - $this->assertEquals([ + // Core should appear first + $this->assertEquals( + 0, + array_search('core/js/myFancyJSFile1', \OCP\Util::getScripts(), true) + ); + $this->assertEquals( + 1, + array_search('core/js/myFancyJSFile4', \OCP\Util::getScripts(), true) + ); + + // Dependencies should appear before their children + $this->assertLessThan( + array_search('files/js/myFancyJSFile2', \OCP\Util::getScripts(), true), + array_search('core/js/myFancyJSFile3', \OCP\Util::getScripts(), true) + ); + $this->assertLessThan( + array_search('myApp2/js/myApp2JSFile', \OCP\Util::getScripts(), true), + array_search('myApp/js/myFancyJSFile3', \OCP\Util::getScripts(), true) + ); + $this->assertLessThan( + array_search('myApp3/js/myApp3JSFile', \OCP\Util::getScripts(), true), + array_search('myApp2/js/myApp2JSFile', \OCP\Util::getScripts(), true) + ); + $this->assertLessThan( + array_search('myApp4/js/myApp4JSFile', \OCP\Util::getScripts(), true), + array_search('myApp3/js/myApp3JSFile', \OCP\Util::getScripts(), true) + ); + $this->assertLessThan( + array_search('myApp5/js/myApp5JSFile', \OCP\Util::getScripts(), true), + array_search('myApp2/js/myApp2JSFile', \OCP\Util::getScripts(), true) + ); + + // No duplicates + $this->assertEquals( + \OCP\Util::getScripts(), + array_unique(\OCP\Util::getScripts()) + ); + + // All scripts still there + $scripts = [ 'core/js/myFancyJSFile1', 'core/js/myFancyJSFile4', 'files/js/myFancyJSFile2', 'core/js/myFancyJSFile5', - 'files/l10n/en', - 'myApp/l10n/en', 'myApp/js/myFancyJSFile3', - ], array_values(\OCP\Util::getScripts())); + 'myApp2/js/myApp2JSFile', + 'myApp3/js/myApp3JSFile', + 'myApp4/js/myApp4JSFile', + ]; + foreach ($scripts as $script) { + $this->assertContains($script, \OCP\Util::getScripts()); + } } public function testAddVendorScript() { From 822daa3c641cbe6b2772ff0a68cefee574628d1e Mon Sep 17 00:00:00 2001 From: Jonas Meurer Date: Fri, 17 Dec 2021 13:30:09 +0100 Subject: [PATCH 3/3] Further improve addScript logic, migrate to uksort() Instead of the logic with `first` and `last`, store dependencies in an own array and sort the scripts topologically by its dependencies later. Signed-off-by: Jonas Meurer --- lib/public/Util.php | 83 ++++++++++++++------------------------------- 1 file changed, 26 insertions(+), 57 deletions(-) diff --git a/lib/public/Util.php b/lib/public/Util.php index c22ee95563c..06f99bb6029 100644 --- a/lib/public/Util.php +++ b/lib/public/Util.php @@ -198,41 +198,12 @@ class Util { self::addTranslations($application); } - // manage priorities if defined - // we store the data like this, then flatten everything - // [ - // 'core' => [ - // 'first' => [ - // '/core/js/main.js', - // ], - // 'last' => [ - // '/apps/viewer/js/viewer-main.js', - // ] - // ], - // 'viewer' => [ - // 'first' => [ - // '/apps/viewer/js/viewer-public.js', - // ], - // 'last' => [ - // '/apps/files_pdfviewer/js/files_pdfviewer-main.js', - // ] - // ] - // ] + // store dependency if defined if (!empty($afterAppId)) { - // init afterAppId app array if it doesn't exist - if (!array_key_exists($afterAppId, self::$scripts)) { - self::$scripts[$afterAppId] = ['first' => [], 'last' => []]; - } - self::$scripts[$afterAppId]['last'][] = $path; - // store dependency self::$scriptDeps[$application] = $afterAppId; - } else { - // init app array if it doesn't exist - if (!array_key_exists($application, self::$scripts)) { - self::$scripts[$application] = ['first' => [], 'last' => []]; - } - self::$scripts[$application]['first'][] = $path; } + + self::$scripts[$application][] = $path; } /** @@ -241,34 +212,32 @@ class Util { * @since 24.0.0 */ public static function getScripts(): array { - // merging first and last data set - $mapFunc = static function (array $scriptsArray): array { - return array_merge(...array_values($scriptsArray)); - }; - $appScripts = array_map($mapFunc, self::$scripts); - // Sort by dependency if any - $sortedScripts = []; - foreach ($appScripts as $app => $scripts) { - if (array_key_exists($app, self::$scriptDeps)) { - $dep = self::$scriptDeps[$app]; - if (!array_key_exists($dep, $sortedScripts)) { - $sortedScripts[$dep] = []; - } - array_push($sortedScripts[$dep], ...$scripts); - } else { - if (!array_key_exists($app, $sortedScripts)) { - $sortedScripts[$app] = []; - } - array_unshift($sortedScripts[$app], ...$scripts); + $sortByDeps = static function (string $app1, string $app2): int { + // Always sort core first + if ($app1 === 'core') { + return -1; + } + if ($app2 === 'core') { + return 1; } - } - // sort core first - $scripts = array_merge(isset($sortedScripts['core']) ? $sortedScripts['core'] : [], ...array_values($sortedScripts)); + // If app1 has a dependency + if (array_key_exists($app1, self::$scriptDeps)) { + $apps = array_keys(self::$scripts); + // Move app1 backwards if dep comes afterwards + if (array_search($app1, $apps, true) < + array_search(self::$scriptDeps[$app1], $apps, true)) { + return 1; + } + } - // remove duplicates - return array_unique($scripts); + return 0; + }; + uksort(self::$scripts, $sortByDeps); + + // Flatten array and remove duplicates + return self::$scripts ? array_unique(array_merge(...array_values(self::$scripts))) : []; } /** @@ -286,7 +255,7 @@ class Util { } else { $path = "l10n/$languageCode"; } - self::$scripts[$application]['first'][] = $path; + self::$scripts[$application][] = $path; } /**