From 5e0be498ef8151895d121b7b49d050b547659b99 Mon Sep 17 00:00:00 2001 From: Tom Richards Date: Thu, 24 Aug 2017 10:01:40 -0400 Subject: [PATCH 1/5] Implement Kahn's topological sort algorithm in the Graph data structure Use toposort for properly determining the order in which modules should be loaded --- lib/internal/Magento/Framework/Data/Graph.php | 82 +++++++++++++++++++ .../Framework/Module/ModuleList/Loader.php | 60 ++++---------- .../Test/Unit/ModuleList/LoaderTest.php | 2 +- 3 files changed, 97 insertions(+), 47 deletions(-) diff --git a/lib/internal/Magento/Framework/Data/Graph.php b/lib/internal/Magento/Framework/Data/Graph.php index fb680bb6870d0..18223e0de7c02 100644 --- a/lib/internal/Magento/Framework/Data/Graph.php +++ b/lib/internal/Magento/Framework/Data/Graph.php @@ -76,6 +76,26 @@ public function addRelation($fromNode, $toNode) return $this; } + /** + * Unset a relation between nodes + * + * @param string|int $fromNode + * @param string|int $toNode + * @return $this + * @throws \InvalidArgumentException + */ + public function removeRelation($fromNode, $toNode) + { + if ($fromNode == $toNode) { + throw new \InvalidArgumentException("Graph node '{$fromNode}' is linked to itself."); + } + $this->_assertNode($fromNode, true); + $this->_assertNode($toNode, true); + unset($this->_from[$fromNode][$toNode]); + unset($this->_to[$toNode][$fromNode]); + return $this; + } + /** * Export relations between nodes. Can return inverse relations * @@ -182,6 +202,68 @@ public function dfs($fromNode, $toNode, $mode = self::DIRECTIONAL) } /** + * Perform a topological sort on the graph. This algorithm is not possible if the graph contains a cycle. + * + * @return array + * @throws \Exception + */ + public function topoSort() + { + $l = []; + $s = $this->_findStartNodes(); + + while (!empty($s)) { + $n = array_shift($s); + array_unshift($l, $n); + if (!isset($this->_from[$n])) { + continue; + } + foreach ($this->_from[$n] as $m) { + $this->removeRelation($n, $m); + if (empty($this->_to[$m])) { + array_push($s, $m); + } + } + } + + if ($this->hasEdges()) { + $cycle = $this->findCycle(); + $message = "Dependency cycle detected: " . implode(" -> ", $cycle); + throw new \Exception($message); + } + + return $l; + } + + public function hasEdges() + { + foreach ($this->_from as $from) { + if (count($from) > 0) { + return true; + } + } + + foreach ($this->_to as $to) { + if (count($to) > 0) { + return true; + } + } + + return false; + } + + /** + * Finds all graph nodes with no incoming edge + * + * @return array + */ + protected function _findStartNodes() + { + $nodesWithIncomingEdges = array_keys($this->_to); + return array_diff($this->_nodes, $nodesWithIncomingEdges); + } + + /** * Recursive sub-routine of dfs() * * @param string|int $fromNode diff --git a/lib/internal/Magento/Framework/Module/ModuleList/Loader.php b/lib/internal/Magento/Framework/Module/ModuleList/Loader.php index bdfb77762b41c..fc661aa85c488 100644 --- a/lib/internal/Magento/Framework/Module/ModuleList/Loader.php +++ b/lib/internal/Magento/Framework/Module/ModuleList/Loader.php @@ -126,62 +126,30 @@ private function getModuleConfigs() * * @param array $origList * @return array - * @SuppressWarnings(PHPMD.UnusedLocalVariable) */ private function sortBySequence($origList) { - ksort($origList); - $expanded = []; - foreach ($origList as $moduleName => $value) { - $expanded[] = [ - 'name' => $moduleName, - 'sequence' => $this->expandSequence($origList, $moduleName), - ]; - } + $nodes = []; + $relations = []; - // Use "bubble sorting" because usort does not check each pair of elements and in this case it is important - $total = count($expanded); - for ($i = 0; $i < $total - 1; $i++) { - for ($j = $i; $j < $total; $j++) { - if (in_array($expanded[$j]['name'], $expanded[$i]['sequence'])) { - $temp = $expanded[$i]; - $expanded[$i] = $expanded[$j]; - $expanded[$j] = $temp; - } + // collect graph nodes and edges + foreach($origList as $module) { + $nodes[] = $module['name']; + foreach ($module['sequence'] as $dependency) { + $relations[] = [$module['name'], $dependency]; } } + // construct a graph and perform topological sort on it + $moduleGraph = new \Magento\Framework\Data\Graph($nodes, $relations); + $sortedResult = $moduleGraph->topoSort(); + + // re-attach version information to the linear ordering $result = []; - foreach ($expanded as $pair) { - $result[$pair['name']] = $origList[$pair['name']]; + foreach ($sortedResult as $item) { + $result[$item] = $origList[$item]; } return $result; } - - /** - * Accumulate information about all transitive "sequence" references - * - * @param array $list - * @param string $name - * @param array $accumulated - * @return array - * @throws \Exception - */ - private function expandSequence($list, $name, $accumulated = []) - { - $accumulated[] = $name; - $result = $list[$name]['sequence']; - foreach ($result as $relatedName) { - if (in_array($relatedName, $accumulated)) { - throw new \Exception("Circular sequence reference from '{$name}' to '{$relatedName}'."); - } - if (!isset($list[$relatedName])) { - continue; - } - $relatedResult = $this->expandSequence($list, $relatedName, $accumulated); - $result = array_unique(array_merge($result, $relatedResult)); - } - return $result; - } } diff --git a/lib/internal/Magento/Framework/Module/Test/Unit/ModuleList/LoaderTest.php b/lib/internal/Magento/Framework/Module/Test/Unit/ModuleList/LoaderTest.php index fe613450fd485..34bb23dccdb32 100644 --- a/lib/internal/Magento/Framework/Module/Test/Unit/ModuleList/LoaderTest.php +++ b/lib/internal/Magento/Framework/Module/Test/Unit/ModuleList/LoaderTest.php @@ -143,7 +143,7 @@ public function testLoadExclude() /** * @expectedException \Exception - * @expectedExceptionMessage Circular sequence reference from 'b' to 'a' + * @expectedExceptionMessage Dependency cycle detected: a -> b -> a */ public function testLoadCircular() { From 91ca91495ba494724d0850ca379937041eaee792 Mon Sep 17 00:00:00 2001 From: Tom Richards Date: Thu, 24 Aug 2017 11:03:10 -0400 Subject: [PATCH 2/5] Lint --- lib/internal/Magento/Framework/Data/Graph.php | 2 +- lib/internal/Magento/Framework/Module/ModuleList/Loader.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/internal/Magento/Framework/Data/Graph.php b/lib/internal/Magento/Framework/Data/Graph.php index 18223e0de7c02..7e196f01de2f8 100644 --- a/lib/internal/Magento/Framework/Data/Graph.php +++ b/lib/internal/Magento/Framework/Data/Graph.php @@ -263,7 +263,7 @@ protected function _findStartNodes() return array_diff($this->_nodes, $nodesWithIncomingEdges); } - /** + /** * Recursive sub-routine of dfs() * * @param string|int $fromNode diff --git a/lib/internal/Magento/Framework/Module/ModuleList/Loader.php b/lib/internal/Magento/Framework/Module/ModuleList/Loader.php index fc661aa85c488..c05351e659346 100644 --- a/lib/internal/Magento/Framework/Module/ModuleList/Loader.php +++ b/lib/internal/Magento/Framework/Module/ModuleList/Loader.php @@ -133,7 +133,7 @@ private function sortBySequence($origList) $relations = []; // collect graph nodes and edges - foreach($origList as $module) { + foreach ($origList as $module) { $nodes[] = $module['name']; foreach ($module['sequence'] as $dependency) { $relations[] = [$module['name'], $dependency]; From deb34a8b6bdc54cd9df73c313b59949acb8a1315 Mon Sep 17 00:00:00 2001 From: Tom Richards Date: Thu, 24 Aug 2017 14:25:00 -0400 Subject: [PATCH 3/5] Fix variable name length --- lib/internal/Magento/Framework/Data/Graph.php | 33 +++++++++++-------- 1 file changed, 20 insertions(+), 13 deletions(-) diff --git a/lib/internal/Magento/Framework/Data/Graph.php b/lib/internal/Magento/Framework/Data/Graph.php index 7e196f01de2f8..defedb438808c 100644 --- a/lib/internal/Magento/Framework/Data/Graph.php +++ b/lib/internal/Magento/Framework/Data/Graph.php @@ -209,19 +209,19 @@ public function dfs($fromNode, $toNode, $mode = self::DIRECTIONAL) */ public function topoSort() { - $l = []; - $s = $this->_findStartNodes(); + $sortedElements = []; + $nodeSet = $this->_findStartNodes(); - while (!empty($s)) { - $n = array_shift($s); - array_unshift($l, $n); - if (!isset($this->_from[$n])) { + while (!empty($nodeSet)) { + $cur = array_shift($nodeSet); + array_unshift($sortedElements, $cur); + if (!isset($this->_from[$cur])) { continue; } - foreach ($this->_from[$n] as $m) { - $this->removeRelation($n, $m); - if (empty($this->_to[$m])) { - array_push($s, $m); + foreach ($this->_from[$cur] as $parent) { + $this->removeRelation($cur, $parent); + if (empty($this->_to[$parent])) { + array_push($nodeSet, $parent); } } } @@ -232,9 +232,16 @@ public function topoSort() throw new \Exception($message); } - return $l; + return $sortedElements; } + /** + * Determines whether the graph has edges. + * + * Returns true if the graph has edges. Returns false otherwise. + * + * @return bool + */ public function hasEdges() { foreach ($this->_from as $from) { @@ -259,8 +266,8 @@ public function hasEdges() */ protected function _findStartNodes() { - $nodesWithIncomingEdges = array_keys($this->_to); - return array_diff($this->_nodes, $nodesWithIncomingEdges); + $incomingEdgeNodes = array_keys($this->_to); + return array_diff($this->_nodes, $incomingEdgeNodes); } /** From 392d7b84220f32ed97fa41fe9684a79b34a4af8f Mon Sep 17 00:00:00 2001 From: Tom Richards Date: Thu, 24 Aug 2017 23:13:01 -0400 Subject: [PATCH 4/5] Fix codacy camelCase issue --- lib/internal/Magento/Framework/Data/Graph.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/internal/Magento/Framework/Data/Graph.php b/lib/internal/Magento/Framework/Data/Graph.php index defedb438808c..1f1aaec2d54c4 100644 --- a/lib/internal/Magento/Framework/Data/Graph.php +++ b/lib/internal/Magento/Framework/Data/Graph.php @@ -210,7 +210,7 @@ public function dfs($fromNode, $toNode, $mode = self::DIRECTIONAL) public function topoSort() { $sortedElements = []; - $nodeSet = $this->_findStartNodes(); + $nodeSet = $this->findStartNodes(); while (!empty($nodeSet)) { $cur = array_shift($nodeSet); @@ -264,7 +264,7 @@ public function hasEdges() * * @return array */ - protected function _findStartNodes() + public function findStartNodes() { $incomingEdgeNodes = array_keys($this->_to); return array_diff($this->_nodes, $incomingEdgeNodes); From d4c33dd07c29e63209e0347bc2dec98db3188fa1 Mon Sep 17 00:00:00 2001 From: Tom Richards Date: Mon, 28 Aug 2017 09:59:10 -0400 Subject: [PATCH 5/5] Replace use of array_shift/array_unshift in loop --- lib/internal/Magento/Framework/Data/Graph.php | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/internal/Magento/Framework/Data/Graph.php b/lib/internal/Magento/Framework/Data/Graph.php index 1f1aaec2d54c4..850ffa1a93fb3 100644 --- a/lib/internal/Magento/Framework/Data/Graph.php +++ b/lib/internal/Magento/Framework/Data/Graph.php @@ -213,15 +213,15 @@ public function topoSort() $nodeSet = $this->findStartNodes(); while (!empty($nodeSet)) { - $cur = array_shift($nodeSet); - array_unshift($sortedElements, $cur); + $cur = array_pop($nodeSet); + $sortedElements[] = $cur; if (!isset($this->_from[$cur])) { continue; } foreach ($this->_from[$cur] as $parent) { $this->removeRelation($cur, $parent); if (empty($this->_to[$parent])) { - array_push($nodeSet, $parent); + $nodeSet[] = $parent; } } } @@ -232,7 +232,7 @@ public function topoSort() throw new \Exception($message); } - return $sortedElements; + return array_reverse($sortedElements); } /**