From 0c8f68e566129ed0c3deffb311fcc3873e3cf363 Mon Sep 17 00:00:00 2001 From: Alexander Skvortsov Date: Thu, 28 Jan 2021 00:37:13 -0500 Subject: [PATCH 1/7] Add and calculate optional dependencies --- src/Extension/Extension.php | 44 ++++++++++++++++++++++++++---- src/Extension/ExtensionManager.php | 5 +++- 2 files changed, 43 insertions(+), 6 deletions(-) diff --git a/src/Extension/Extension.php b/src/Extension/Extension.php index 0844164c49..ec12b0f040 100644 --- a/src/Extension/Extension.php +++ b/src/Extension/Extension.php @@ -64,7 +64,7 @@ protected static function nameToId($name) * Unique Id of the extension. * * @info Identical to the directory in the extensions directory. - * @example flarum_suspend + * @example flarum-suspend * * @var string */ @@ -91,6 +91,14 @@ protected static function nameToId($name) */ protected $extensionDependencyIds; + /** + * The IDs of all Flarum extensions that this extension should be booted after + * if enabled. + * + * @var string[] + */ + protected $optionalDependencyIds; + /** * Whether the extension is installed. * @@ -203,16 +211,30 @@ public function setVersion($version) * @param array $extensionSet: An associative array where keys are the composer package names * of installed extensions. Used to figure out which dependencies * are flarum extensions. + * @param array $enabledIds: An associative array where keys are the composer package names + * of enabled extensions. Used to figure out optional dependencies. */ - public function calculateDependencies($extensionSet) + public function calculateDependencies($extensionSet, $enabledIds) { $this->extensionDependencyIds = (new Collection(Arr::get($this->composerJson, 'require', []))) ->keys() + ->merge(Arr::get($this->composerJson, 'extra.flarum-extension.optional-dependencies', [])) ->filter(function ($key) use ($extensionSet) { return array_key_exists($key, $extensionSet); - })->map(function ($key) { + }) + ->map(function ($key) { + return static::nameToId($key); + }) + ->toArray(); + + $this->optionalDependencyIds = (new Collection(Arr::get($this->composerJson, 'extra.flarum-extension.optional-dependencies', []))) + ->map(function ($key) { return static::nameToId($key); - })->toArray(); + }) + ->filter(function ($key) use ($enabledIds) { + return array_key_exists($key, $enabledIds); + }) + ->toArray(); } /** @@ -299,11 +321,22 @@ public function getPath() * * @return array */ - public function getExtensionDependencyIds() + public function getExtensionDependencyIds(): array { return $this->extensionDependencyIds; } + /** + * The IDs of all Flarum extensions that this extension should be booted after + * if enabled. + * + * @return array + */ + public function getOptionalDependencyIds(): array + { + return $this->optionalDependencyIds; + } + private function getExtenders(): array { $extenderFile = $this->getExtenderFile(); @@ -455,6 +488,7 @@ public function toArray() 'hasAssets' => $this->hasAssets(), 'hasMigrations' => $this->hasMigrations(), 'extensionDependencyIds' => $this->getExtensionDependencyIds(), + 'optionalDependencyIds' => $this->getOptionalDependencyIds(), 'links' => $this->getLinks(), ], $this->composerJson); } diff --git a/src/Extension/ExtensionManager.php b/src/Extension/ExtensionManager.php index 6f5ee9ba85..d64a6e6f7b 100644 --- a/src/Extension/ExtensionManager.php +++ b/src/Extension/ExtensionManager.php @@ -86,7 +86,9 @@ public function getExtensions() // We calculate and store a set of composer package names for all installed Flarum extensions, // so we know what is and isn't a flarum extension in `calculateDependencies`. // Using keys of an associative array allows us to do these checks in constant time. + // We do the same for enabled extensions, for optional dependencies. $installedSet = []; + $enabledIds = array_flip($this->getEnabled()); foreach ($installed as $package) { if (Arr::get($package, 'type') != 'flarum-extension' || empty(Arr::get($package, 'name'))) { @@ -109,8 +111,9 @@ public function getExtensions() $extensions->put($extension->getId(), $extension); } + foreach ($extensions as $extension) { - $extension->calculateDependencies($installedSet); + $extension->calculateDependencies($installedSet, $enabledIds); } $this->extensions = $extensions->sortBy(function ($extension, $name) { From 56066f92e7e999e9fcf86a7af1669880d7cc1a6e Mon Sep 17 00:00:00 2001 From: Alexander Skvortsov Date: Thu, 28 Jan 2021 00:51:38 -0500 Subject: [PATCH 2/7] Add extension dependency resolver (Kahn's algorithm), plus unit tests --- src/Extension/ExtensionManager.php | 83 +++++++++++ .../ExtensionDependencyResolutionTest.php | 129 ++++++++++++++++++ 2 files changed, 212 insertions(+) create mode 100644 tests/unit/Foundation/ExtensionDependencyResolutionTest.php diff --git a/src/Extension/ExtensionManager.php b/src/Extension/ExtensionManager.php index d64a6e6f7b..182a6bbba9 100644 --- a/src/Extension/ExtensionManager.php +++ b/src/Extension/ExtensionManager.php @@ -385,4 +385,87 @@ public static function pluckTitles(array $exts) return $extension->getTitle(); }, $exts); } + + /** + * Sort a list of extensions so that they are properly resolved in respect to order. + * Effectively just topological sorting. + * + * @param Extension[] $extensionList: an array of \Flarum\Extension\Extension objects + * + * @return array with 2 keys: 'valid' points to an ordered array of \Flarum\Extension\Extension + * 'missingDependencies' points to an associative array of extensions that could not be resolved due + * to missing dependencies, in the format extension id => array of missing dependency IDs. + * 'circularDependencies' points to an array of extensions ids of extensions + * that cannot be processed due to circular dependencies + */ + public static function resolveExtensionOrder($extensionList) + { + $extensionIdMapping = []; // Used for caching so we don't rerun ->getExtensions every time. + + // This is an implementation of Kahn's Algorithm (https://dl.acm.org/doi/10.1145/368996.369025) + $extensionGraph = []; + $output = []; + $missingDependencies = []; // Extensions are invalid if they are missing dependencies, or have circular dependencies. + $circularDependencies = []; + $pendingQueue = []; + $inDegreeCount = []; // How many extensions are dependent on a given extension? + + + foreach ($extensionList as $extension) { + $extensionIdMapping[$extension->getId()] = $extension; + $extensionGraph[$extension->getId()] = array_merge($extension->getExtensionDependencyIds(), $extension->getOptionalDependencyIds()); + + foreach ($extensionGraph[$extension->getId()] as $dependency) { + $inDegreeCount[$dependency] = array_key_exists($dependency, $inDegreeCount) ? $inDegreeCount[$dependency] + 1 : 1; + } + } + + foreach ($extensionList as $extension) { + if (!array_key_exists($extension->getId(), $inDegreeCount)) { + $inDegreeCount[$extension->getId()] = 0; + $pendingQueue[] = $extension->getId(); + } + } + + while (! empty($pendingQueue)) { + $activeNode = array_shift($pendingQueue); + $output[] = $activeNode; + + foreach ($extensionGraph[$activeNode] as $dependency) { + $inDegreeCount[$dependency] -= 1; + + if ($inDegreeCount[$dependency] === 0) { + if (!array_key_exists($dependency, $extensionGraph)) { + // Missing Dependency + $missingDependencies[$activeNode] = array_merge( + Arr::get($missingDependencies, $activeNode, []), + [$dependency] + ); + } else { + $pendingQueue[] = $dependency; + } + } + } + } + + $validOutput = array_filter($output, function ($extension) use ($missingDependencies) { + return !array_key_exists($extension, $missingDependencies); + }); + + $validExtensions = array_reverse(array_map(function($extensionId) use ($extensionIdMapping) { + return $extensionIdMapping[$extensionId]; + }, $validOutput)); // Reversed as required by Kahn's algorithm. + + foreach ($inDegreeCount as $id => $count) { + if ($count != 0) { + $circularDependencies[] = $id; + } + } + + return [ + 'valid' => $validExtensions, + 'missingDependencies' => $missingDependencies, + 'circularDependencies' => $circularDependencies + ]; + } } diff --git a/tests/unit/Foundation/ExtensionDependencyResolutionTest.php b/tests/unit/Foundation/ExtensionDependencyResolutionTest.php new file mode 100644 index 0000000000..290f46582f --- /dev/null +++ b/tests/unit/Foundation/ExtensionDependencyResolutionTest.php @@ -0,0 +1,129 @@ +tags = new FakeExtension('flarum-tags', []); + $this->categories = new FakeExtension('flarum-categories', ['flarum-tags', 'flarum-tag-backgrounds']); + $this->tagBackgrounds = new FakeExtension('flarum-tag-backgrounds', ['flarum-tags']); + $this->something = new FakeExtension('flarum-something', ['flarum-categories', 'flarum-help']); + $this->help = new FakeExtension('flarum-help', []); + $this->missing = new FakeExtension('flarum-missing', ['this-does-not-exist', 'flarum-tags', 'also-not-exists']); + $this->circular1 = new FakeExtension('circular1', ['circular2']); + $this->circular2 = new FakeExtension('circular2', ['circular1']); + $this->optionalDependencyCategories = new FakeExtension('flarum-categories', ['flarum-tags'], ['flarum-tag-backgrounds']); + } + + + /** @test */ + public function works_with_empty_set() + { + $expected = [ + 'valid' => [], + 'missingDependencies' => [], + 'circularDependencies' => [], + ]; + + $this->assertEquals($expected, ExtensionManager::resolveExtensionOrder([])); + } + + /** @test */ + public function works_with_proper_data() + { + $exts = [$this->tags, $this->categories, $this->tagBackgrounds, $this->something, $this->help]; + + $expected = [ + 'valid' => [$this->tags, $this->tagBackgrounds, $this->help, $this->categories, $this->something], + 'missingDependencies' => [], + 'circularDependencies' => [], + ]; + + $this->assertEquals($expected, ExtensionManager::resolveExtensionOrder($exts)); + } + + /** @test */ + public function works_with_missing_dependencies() + { + $exts = [$this->tags, $this->categories, $this->tagBackgrounds, $this->something, $this->help, $this->missing]; + + $expected = [ + 'valid' => [$this->tags, $this->tagBackgrounds, $this->help, $this->categories, $this->something], + 'missingDependencies' => ['flarum-missing' => ['this-does-not-exist', 'also-not-exists']], + 'circularDependencies' => [], + ]; + + $this->assertEquals($expected, ExtensionManager::resolveExtensionOrder($exts)); + } + + /** @test */ + public function works_with_circular_dependencies() + { + $exts = [$this->tags, $this->categories, $this->tagBackgrounds, $this->something, $this->help, $this->circular1, $this->circular2]; + + $expected = [ + 'valid' => [$this->tags, $this->tagBackgrounds, $this->help, $this->categories, $this->something], + 'missingDependencies' => [], + 'circularDependencies' => ['circular2', 'circular1'], + ]; + + $this->assertEquals($expected, ExtensionManager::resolveExtensionOrder($exts)); + } + + /** @test */ + public function works_with_optional_dependencies() + { + $exts = [$this->tags, $this->optionalDependencyCategories, $this->tagBackgrounds, $this->something, $this->help]; + + $expected = [ + 'valid' => [$this->tags, $this->tagBackgrounds, $this->help, $this->optionalDependencyCategories, $this->something], + 'missingDependencies' => [], + 'circularDependencies' => [], + ]; + + $this->assertEquals($expected, ExtensionManager::resolveExtensionOrder($exts)); + } +} + +class FakeExtension +{ + protected $id; + protected $extensionDependencies; + protected $optionalDependencies; + + public function __construct($id, $extensionDependencies, $optionalDependencies = []) + { + $this->id = $id; + $this->extensionDependencies = $extensionDependencies; + $this->optionalDependencies = $optionalDependencies; + } + + public function getId() + { + return $this->id; + } + + public function getExtensionDependencyIds() + { + return $this->extensionDependencies; + } + + public function getOptionalDependencyIds() + { + return $this->optionalDependencies; + } +} From 7973141b3cda2d1884aa69deec5bbd832f7ea5aa Mon Sep 17 00:00:00 2001 From: Alexander Skvortsov Date: Thu, 28 Jan 2021 01:06:56 -0500 Subject: [PATCH 3/7] Resolve extension dependency on enable/disable --- src/Extension/ExtensionManager.php | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/src/Extension/ExtensionManager.php b/src/Extension/ExtensionManager.php index 182a6bbba9..213620a8c3 100644 --- a/src/Extension/ExtensionManager.php +++ b/src/Extension/ExtensionManager.php @@ -351,13 +351,21 @@ public function getEnabled() /** * Persist the currently enabled extensions. * - * @param array $enabled + * @param array $enabledIds */ - protected function setEnabled(array $enabled) + protected function setEnabled(array $enabledIds) { - $enabled = array_values(array_unique($enabled)); + $enabled = array_map(function ($id) { + return $this->getExtension($id); + }, array_unique($enabledIds)); - $this->config->set('extensions_enabled', json_encode($enabled)); + $sortedEnabled = static::resolveExtensionOrder($enabled)['valid']; + + $sortedEnabledIds = array_map(function (Extension $extension) { + return $extension->getId(); + }, $sortedEnabled); + + $this->config->set('extensions_enabled', json_encode($sortedEnabledIds)); } /** From d37a8768d0c8b1d1f6acc45301bdb8ea413de6af Mon Sep 17 00:00:00 2001 From: Alexander Skvortsov Date: Thu, 28 Jan 2021 06:07:19 +0000 Subject: [PATCH 4/7] Apply fixes from StyleCI [ci skip] [skip ci] --- src/Extension/ExtensionManager.php | 10 ++++------ .../Foundation/ExtensionDependencyResolutionTest.php | 1 - 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/src/Extension/ExtensionManager.php b/src/Extension/ExtensionManager.php index 213620a8c3..40433aa376 100644 --- a/src/Extension/ExtensionManager.php +++ b/src/Extension/ExtensionManager.php @@ -111,7 +111,6 @@ public function getExtensions() $extensions->put($extension->getId(), $extension); } - foreach ($extensions as $extension) { $extension->calculateDependencies($installedSet, $enabledIds); } @@ -418,7 +417,6 @@ public static function resolveExtensionOrder($extensionList) $pendingQueue = []; $inDegreeCount = []; // How many extensions are dependent on a given extension? - foreach ($extensionList as $extension) { $extensionIdMapping[$extension->getId()] = $extension; $extensionGraph[$extension->getId()] = array_merge($extension->getExtensionDependencyIds(), $extension->getOptionalDependencyIds()); @@ -429,7 +427,7 @@ public static function resolveExtensionOrder($extensionList) } foreach ($extensionList as $extension) { - if (!array_key_exists($extension->getId(), $inDegreeCount)) { + if (! array_key_exists($extension->getId(), $inDegreeCount)) { $inDegreeCount[$extension->getId()] = 0; $pendingQueue[] = $extension->getId(); } @@ -443,7 +441,7 @@ public static function resolveExtensionOrder($extensionList) $inDegreeCount[$dependency] -= 1; if ($inDegreeCount[$dependency] === 0) { - if (!array_key_exists($dependency, $extensionGraph)) { + if (! array_key_exists($dependency, $extensionGraph)) { // Missing Dependency $missingDependencies[$activeNode] = array_merge( Arr::get($missingDependencies, $activeNode, []), @@ -457,10 +455,10 @@ public static function resolveExtensionOrder($extensionList) } $validOutput = array_filter($output, function ($extension) use ($missingDependencies) { - return !array_key_exists($extension, $missingDependencies); + return ! array_key_exists($extension, $missingDependencies); }); - $validExtensions = array_reverse(array_map(function($extensionId) use ($extensionIdMapping) { + $validExtensions = array_reverse(array_map(function ($extensionId) use ($extensionIdMapping) { return $extensionIdMapping[$extensionId]; }, $validOutput)); // Reversed as required by Kahn's algorithm. diff --git a/tests/unit/Foundation/ExtensionDependencyResolutionTest.php b/tests/unit/Foundation/ExtensionDependencyResolutionTest.php index 290f46582f..7383d645ce 100644 --- a/tests/unit/Foundation/ExtensionDependencyResolutionTest.php +++ b/tests/unit/Foundation/ExtensionDependencyResolutionTest.php @@ -29,7 +29,6 @@ public function setUp() $this->optionalDependencyCategories = new FakeExtension('flarum-categories', ['flarum-tags'], ['flarum-tag-backgrounds']); } - /** @test */ public function works_with_empty_set() { From 299ed625f382bdf32ff287402f0c4ddf3da174b0 Mon Sep 17 00:00:00 2001 From: Alexander Skvortsov Date: Tue, 16 Feb 2021 18:53:07 -0500 Subject: [PATCH 5/7] Fix forgotten merge --- src/Extension/Extension.php | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Extension/Extension.php b/src/Extension/Extension.php index ec12b0f040..de5e5f30dd 100644 --- a/src/Extension/Extension.php +++ b/src/Extension/Extension.php @@ -218,7 +218,6 @@ public function calculateDependencies($extensionSet, $enabledIds) { $this->extensionDependencyIds = (new Collection(Arr::get($this->composerJson, 'require', []))) ->keys() - ->merge(Arr::get($this->composerJson, 'extra.flarum-extension.optional-dependencies', [])) ->filter(function ($key) use ($extensionSet) { return array_key_exists($key, $extensionSet); }) From 0745c3e7f3496be11c6066dcdc11f2f5c6436047 Mon Sep 17 00:00:00 2001 From: Alexander Skvortsov Date: Tue, 16 Feb 2021 19:33:48 -0500 Subject: [PATCH 6/7] Fix interface compatibility --- tests/unit/Foundation/ExtensionDependencyResolutionTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/Foundation/ExtensionDependencyResolutionTest.php b/tests/unit/Foundation/ExtensionDependencyResolutionTest.php index 7383d645ce..fca08ba629 100644 --- a/tests/unit/Foundation/ExtensionDependencyResolutionTest.php +++ b/tests/unit/Foundation/ExtensionDependencyResolutionTest.php @@ -14,7 +14,7 @@ class ExtensionDependencyResolutionTest extends TestCase { - public function setUp() + public function setUp(): void { parent::setUp(); From b13c7e2e667ee40b829f283e1e7ddb627f3273a4 Mon Sep 17 00:00:00 2001 From: Alexander Skvortsov Date: Sat, 20 Feb 2021 19:44:08 -0500 Subject: [PATCH 7/7] Don't consider a missing optional dependency as a missing dependency --- src/Extension/ExtensionManager.php | 8 +++++++- .../ExtensionDependencyResolutionTest.php | 14 ++++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/src/Extension/ExtensionManager.php b/src/Extension/ExtensionManager.php index 40433aa376..58a77e7f0a 100644 --- a/src/Extension/ExtensionManager.php +++ b/src/Extension/ExtensionManager.php @@ -419,7 +419,13 @@ public static function resolveExtensionOrder($extensionList) foreach ($extensionList as $extension) { $extensionIdMapping[$extension->getId()] = $extension; - $extensionGraph[$extension->getId()] = array_merge($extension->getExtensionDependencyIds(), $extension->getOptionalDependencyIds()); + } + + foreach ($extensionList as $extension) { + $optionalDependencies = array_filter($extension->getOptionalDependencyIds(), function ($id) use ($extensionIdMapping) { + return array_key_exists($id, $extensionIdMapping); + }); + $extensionGraph[$extension->getId()] = array_merge($extension->getExtensionDependencyIds(), $optionalDependencies); foreach ($extensionGraph[$extension->getId()] as $dependency) { $inDegreeCount[$dependency] = array_key_exists($dependency, $inDegreeCount) ? $inDegreeCount[$dependency] + 1 : 1; diff --git a/tests/unit/Foundation/ExtensionDependencyResolutionTest.php b/tests/unit/Foundation/ExtensionDependencyResolutionTest.php index fca08ba629..39fc66ab7d 100644 --- a/tests/unit/Foundation/ExtensionDependencyResolutionTest.php +++ b/tests/unit/Foundation/ExtensionDependencyResolutionTest.php @@ -96,6 +96,20 @@ public function works_with_optional_dependencies() $this->assertEquals($expected, ExtensionManager::resolveExtensionOrder($exts)); } + + /** @test */ + public function works_with_optional_dependencies_if_optional_dependency_missing() + { + $exts = [$this->tags, $this->optionalDependencyCategories, $this->something, $this->help]; + + $expected = [ + 'valid' => [$this->tags, $this->help, $this->optionalDependencyCategories, $this->something], + 'missingDependencies' => [], + 'circularDependencies' => [], + ]; + + $this->assertEquals($expected, ExtensionManager::resolveExtensionOrder($exts)); + } } class FakeExtension