MC-5465: Enforcing 'require' and 'require-dev' ordering

Author: pdohogne-magento

docs/class_descriptions.md

@@ -150,13 +150,17 @@ This is accomplished by comparing `composer.json` fields between the original Ma
  - **`findResolution()`**
    - For an individual field value, compare the original Magento value to the target Magento value, and if a delta is found, check if the user's installation has a customized value for the field. If the user has changed the value, resolve the conflict according to the CLI command options: use the user's custom value, override with the target Magento value, or interactively ask the user which of the two values should be used
  - **`resolveLinkSection()`**
-   - For a given `composer.json` section that consists of links to package versions/constraints (such as the `require` and `conflict` sections), call `findResolution()` for each package constraint found in either the original Magento root or the target Magento root
+   - For a given `composer.json` section that consists of links to package versions/constraints (such as the `require` and `conflict` sections), call `findLinkResolution()` for each package constraint found in either the original Magento root or the target Magento root
  - **`resolveArraySection()`**
    - For a given `composer.json` section that consists of data that is not package links (such as the `"autoload"` or `"extra"` sections), call `resolveNestedArray()` and accept the new values if changes were made
  - **`resolveNestedArray()`**
    - Recursively processes changes to a `composer.json` value that could be a nested array, calling `findResolution()` for each "leaf" value found in either the original Magento root or the target Magento root
- - **`linksToMap()`**
-   - Helper function to convert a set of package links to an associative array for use by `resolveLinkSection()`
+ - **`findLinkResolution()`**
+   - Helper function to call `findResolution()` for a particular package for use by `resolveLinkSection()`
+ - **`getLinkOrderOverride()`**
+   - Determine the order to use for a link section when the user's order disagrees with the target Magento section order
+ - **`buildLinkOrderComparator()`**
+   - Construct the comparator function to use for sorting a set of links according to `getLinkOverride()` results followed by the order in the target Magento version followed by the order of custom values in the user's installation
 
 #### [**MagentoRootUpdater**](../src/Magento/ComposerRootUpdatePlugin/Updater/MagentoRootUpdater.php)
 

src/Magento/ComposerRootUpdatePlugin/Updater/DeltaResolver.php

@@ -89,21 +89,41 @@ public function resolveRootDeltas()
         $target = $this->targetMageRootPackage;
         $user = $this->userRootPackage;
 
-        $this->resolveLinkSection('require', $original->getRequires(), $target->getRequires(), $user->getRequires());
+        $this->resolveLinkSection(
+            'require',
+            $original->getRequires(),
+            $target->getRequires(),
+            $user->getRequires(),
+            true
+        );
         $this->resolveLinkSection(
             'require-dev',
             $original->getDevRequires(),
             $target->getDevRequires(),
-            $user->getDevRequires()
+            $user->getDevRequires(),
+            true
         );
         $this->resolveLinkSection(
             'conflict',
             $original->getConflicts(),
             $target->getConflicts(),
-            $user->getConflicts()
+            $user->getConflicts(),
+            false
+        );
+        $this->resolveLinkSection(
+            'provide',
+            $original->getProvides(),
+            $target->getProvides(),
+            $user->getProvides(),
+            false
+        );
+        $this->resolveLinkSection(
+            'replace',
+            $original->getReplaces(),
+            $target->getReplaces(),
+            $user->getReplaces(),
+            false
         );
-        $this->resolveLinkSection('provide', $original->getProvides(), $target->getProvides(), $user->getProvides());
-        $this->resolveLinkSection('replace', $original->getReplaces(), $target->getReplaces(), $user->getReplaces());
 
         $this->resolveArraySection('autoload', $original->getAutoload(), $target->getAutoload(), $user->getAutoload());
         $this->resolveArraySection(
@@ -215,53 +235,26 @@ public function findResolution(
      * @param Link[] $originalMageLinks
      * @param Link[] $targetMageLinks
      * @param Link[] $userLinks
+     * @param bool $verifyOrder
      * @return array
      */
-    public function resolveLinkSection($section, $originalMageLinks, $targetMageLinks, $userLinks)
+    public function resolveLinkSection($section, $originalMageLinks, $targetMageLinks, $userLinks, $verifyOrder)
     {
-        /** @var Link[] $originalLinkMap */
-        $originalLinkMap = static::linksToMap($originalMageLinks);
-
-        /** @var Link[] $targetLinkMap */
-        $targetLinkMap = static::linksToMap($targetMageLinks);
-
-        /** @var Link[] $userLinkMap */
-        $userLinkMap = static::linksToMap($userLinks);
-
         $adds = [];
         $removes = [];
         $changes = [];
-        $magePackages = array_unique(array_merge(array_keys($originalLinkMap), array_keys($targetLinkMap)));
+        $magePackages = array_unique(array_merge(array_keys($originalMageLinks), array_keys($targetMageLinks)));
         foreach ($magePackages as $pkg) {
             if ($section === 'require' && PackageUtils::getMagentoProductEdition($pkg)) {
                 continue;
             }
-            $field = "$section:$pkg";
-            $originalConstraint = key_exists($pkg, $originalLinkMap) ? $originalLinkMap[$pkg]->getConstraint() : null;
-            $originalMageVal = ($originalConstraint === null) ? null : $originalConstraint->__toString();
-            $prettyOriginalMageVal = ($originalConstraint === null) ? null : $originalConstraint->getPrettyString();
-            $targetConstraint = key_exists($pkg, $targetLinkMap) ? $targetLinkMap[$pkg]->getConstraint() : null;
-            $targetMageVal = ($targetConstraint === null) ? null : $targetConstraint->__toString();
-            $prettyTargetMageVal = ($targetConstraint === null) ? null : $targetConstraint->getPrettyString();
-            $userConstraint = key_exists($pkg, $userLinkMap) ? $userLinkMap[$pkg]->getConstraint() : null;
-            $userVal = ($userConstraint === null) ? null : $userConstraint->__toString();
-            $prettyUserVal = ($userConstraint === null) ? null : $userConstraint->getPrettyString();
-
-            $action = $this->findResolution(
-                $field,
-                $originalMageVal,
-                $targetMageVal,
-                $userVal,
-                $prettyOriginalMageVal,
-                $prettyTargetMageVal,
-                $prettyUserVal
-            );
+            $action = $this->findLinkResolution($section, $pkg, $originalMageLinks, $targetMageLinks, $userLinks);
             if ($action == static::ADD_VAL) {
-                $adds[$pkg] = $targetLinkMap[$pkg];
+                $adds[$pkg] = $targetMageLinks[$pkg];
             } elseif ($action == static::REMOVE_VAL) {
                 $removes[] = $pkg;
             } elseif ($action == static::CHANGE_VAL) {
-                $changes[$pkg] = $targetLinkMap[$pkg];
+                $changes[$pkg] = $targetMageLinks[$pkg];
             }
         }
 
@@ -287,11 +280,26 @@ public function resolveLinkSection($section, $originalMageLinks, $targetMageLink
             $this->console->labeledVerbose("Updating $section constraints: " . implode(', ', $prettyChanges));
         }
 
+        $enforcedOrder = [];
+        if ($verifyOrder) {
+            $enforcedOrder = $this->getLinkOrderOverride(
+                $section,
+                array_keys($originalMageLinks),
+                array_keys($targetMageLinks),
+                array_keys($userLinks)
+            );
+            if ($enforcedOrder !== []) {
+                $changed = true;
+                $prettyOrder = "   [\n      " . implode(",\n      ",  $enforcedOrder) . "\n   ]";
+                $this->console->labeledVerbose("Updating $section order:\n$prettyOrder");
+            }
+        }
+
         if ($changed) {
             $replacements = array_values($adds);
 
             /** @var Link $userLink */
-            foreach ($userLinkMap as $pkg => $userLink) {
+            foreach ($userLinks as $pkg => $userLink) {
                 if (in_array($pkg, $removes)) {
                     continue;
                 } elseif (key_exists($pkg, $changes)) {
@@ -301,6 +309,12 @@ public function resolveLinkSection($section, $originalMageLinks, $targetMageLink
                 }
             }
 
+            usort($replacements, $this->buildLinkOrderComparator(
+                $enforcedOrder,
+                array_keys($targetMageLinks),
+                array_keys($userLinks)
+            ));
+
             $newJson = [];
             /** @var Link $link */
             foreach ($replacements as $link) {
@@ -349,37 +363,15 @@ public function resolveNestedArray($field, $originalMageVal, $targetMageVal, $us
         $result = $userVal === null ? [] : $userVal;
 
         if (is_array($originalMageVal) && is_array($targetMageVal) && is_array($userVal)) {
-            $originalMageAssociativePart = [];
-            $originalMageFlatPart = [];
-            foreach ($originalMageVal as $key => $value) {
-                if (is_string($key)) {
-                    $originalMageAssociativePart[$key] = $value;
-                } else {
-                    $originalMageFlatPart[] = $value;
-                }
-            }
+            $originalMageAssociativePart = array_filter($originalMageVal, 'is_string', ARRAY_FILTER_USE_KEY);
+            $originalMageFlatPart = array_filter($originalMageVal, 'is_int', ARRAY_FILTER_USE_KEY);
 
-            $targetMageAssociativePart = [];
-            $targetMageFlatPart = [];
-            foreach ($targetMageVal as $key => $value) {
-                if (is_string($key)) {
-                    $targetMageAssociativePart[$key] = $value;
-                } else {
-                    $targetMageFlatPart[] = $value;
-                }
-            }
+            $targetMageAssociativePart = array_filter($targetMageVal, 'is_string', ARRAY_FILTER_USE_KEY);
+            $targetMageFlatPart = array_filter($targetMageVal, 'is_int', ARRAY_FILTER_USE_KEY);
 
-            $userAssociativePart = [];
-            $userFlatPart = [];
-            foreach ($userVal as $key => $value) {
-                if (is_string($key)) {
-                    $userAssociativePart[$key] = $value;
-                } else {
-                    $userFlatPart[] = $value;
-                }
-            }
+            $userAssociativePart = array_filter($userVal, 'is_string', ARRAY_FILTER_USE_KEY);
 
-            $associativeResult = array_filter($result, 'is_string', ARRAY_FILTER_USE_KEY);
+            $associativeResult = $userAssociativePart;
             $mageKeys = array_unique(
                 array_merge(array_keys($originalMageAssociativePart), array_keys($targetMageAssociativePart))
             );
@@ -417,7 +409,7 @@ public function resolveNestedArray($field, $originalMageVal, $targetMageVal, $us
                 }
             }
 
-            $flatResult = array_filter($result, 'is_int', ARRAY_FILTER_USE_KEY);
+            $flatResult = array_filter($userVal, 'is_int', ARRAY_FILTER_USE_KEY);
             $flatAdds = array_diff(array_diff($targetMageFlatPart, $originalMageFlatPart), $flatResult);
             if ($flatAdds !== []) {
                 $valChanged = true;
@@ -456,17 +448,132 @@ public function resolveNestedArray($field, $originalMageVal, $targetMageVal, $us
     }
 
     /**
-     * Helper function to convert a set of links to an associative array with target package names as keys
+     * Helper function to find the resolution action for a package constraint in the Link sections
      *
-     * @param Link[] $links
-     * @return array
+     * @param string $section
+     * @param string $pkg
+     * @param Link[] $originalLinkMap
+     * @param Link[] $targetLinkMap
+     * @param Link[] $userLinkMap
+     * @return string|null ADD_VAL|REMOVE_VAL|CHANGE_VAL to adjust the link constraint, null for no change
      */
-    protected function linksToMap($links)
+    protected function findLinkResolution($section, $pkg, $originalLinkMap, $targetLinkMap, $userLinkMap)
     {
-        $targets = array_map(function ($link) {
-            /** @var Link $link */
-            return $link->getTarget();
-        }, $links);
-        return array_combine($targets, $links);
+        $field = "$section:$pkg";
+        $originalConstraint = key_exists($pkg, $originalLinkMap) ? $originalLinkMap[$pkg]->getConstraint() : null;
+        $originalMageVal = ($originalConstraint === null) ? null : $originalConstraint->__toString();
+        $prettyOriginalMageVal = ($originalConstraint === null) ? null : $originalConstraint->getPrettyString();
+        $targetConstraint = key_exists($pkg, $targetLinkMap) ? $targetLinkMap[$pkg]->getConstraint() : null;
+        $targetMageVal = ($targetConstraint === null) ? null : $targetConstraint->__toString();
+        $prettyTargetMageVal = ($targetConstraint === null) ? null : $targetConstraint->getPrettyString();
+        $userConstraint = key_exists($pkg, $userLinkMap) ? $userLinkMap[$pkg]->getConstraint() : null;
+        $userVal = ($userConstraint === null) ? null : $userConstraint->__toString();
+        $prettyUserVal = ($userConstraint === null) ? null : $userConstraint->getPrettyString();
+
+        return $this->findResolution(
+            $field,
+            $originalMageVal,
+            $targetMageVal,
+            $userVal,
+            $prettyOriginalMageVal,
+            $prettyTargetMageVal,
+            $prettyUserVal
+        );
+    }
+
+    /**
+     * Get the order to use for a link section if local and target versions disagree
+     *
+     * @param string $section
+     * @param string[] $originalMageOrder
+     * @param string[] $targetMageOrder
+     * @param string[] $userOrder
+     * @return string[]
+     */
+    protected function getLinkOrderOverride($section, $originalMageOrder, $targetMageOrder, $userOrder)
+    {
+        $overrideOrder = [];
+
+        $conflictTargetOrder = array_values(array_intersect($targetMageOrder, $userOrder));
+        $conflictUserOrder = array_values(array_intersect($userOrder, $targetMageOrder));
+
+        // Check if the user's link order does not match the target section for links that appear in both
+        if ($conflictTargetOrder != $conflictUserOrder) {
+            $conflictOriginalOrder = array_values(array_intersect($originalMageOrder, $targetMageOrder));
+
+            // Check if the user's order is different than the target order because the order has changed between
+            // the original and target Magento versions
+            if ($conflictOriginalOrder !== $conflictUserOrder) {
+                $targetLabel = $this->retriever->getTargetLabel();
+                $userOrderDesc = "   [\n      " . implode(",\n      ", $conflictUserOrder) . "\n   ]";
+                $targetOrderDesc = "   [\n      " . implode(",\n      ",  $conflictTargetOrder) . "\n   ]";
+                $conflictDesc = "$targetLabel is trying to change the existing order of the $section section.\n" .
+                    "Local order:\n$userOrderDesc\n$targetLabel order:\n$targetOrderDesc";
+                $shouldOverride = $this->overrideUserValues;
+                if ($this->overrideUserValues) {
+                    $this->console->log($conflictDesc);
+                    $this->console->log(
+                        'Overriding local order due to --' . MageRootRequireCommand::OVERRIDE_OPT . '.'
+                    );
+                } else {
+                    $shouldOverride = $this->console->ask(
+                        "$conflictDesc\nWould you like to override the local order?"
+                    );
+                }
+
+                if (!$shouldOverride) {
+                    $this->console->comment("$conflictDesc but it will not be changed. Re-run using " .
+                        '--' . MageRootRequireCommand::OVERRIDE_OPT . ' or ' .
+                        '--' . MageRootRequireCommand::INTERACTIVE_OPT . ' to override with the Magento order.');
+                    $overrideOrder = $conflictUserOrder;
+                } else {
+                    $overrideOrder = $conflictTargetOrder;
+                }
+            } else {
+                $overrideOrder = $conflictTargetOrder;
+            }
+        }
+
+        return $overrideOrder;
+    }
+
+    /**
+     * Construct a comparison function to use in sorting an array of links by prioritized order lists
+     *
+     * @param string[] $overrideOrder
+     * @param string[] $targetMageOrder
+     * @param string[] $userOrder
+     * @return \Closure
+     */
+    protected function buildLinkOrderComparator($overrideOrder, $targetMageOrder, $userOrder)
+    {
+        $prioritizedOrderings = [$overrideOrder, $targetMageOrder, $userOrder];
+
+        return function ($link1, $link2) use ($prioritizedOrderings) {
+            /**
+             * @var Link $link1
+             * @var Link $link2
+             */
+            $package1 = $link1->getTarget();
+            $package2 = $link2->getTarget();
+
+            // Check each ordering array to see if it contains both links and if so use their positions to sort
+            // If the ordering array does not contain both links, try the next one
+            foreach ($prioritizedOrderings as $sortOrder) {
+                $index1 = array_search($package1, $sortOrder);
+                $index2 = array_search($package2, $sortOrder);
+                if ($index1 !== false && $index2 !== false) {
+                    if ($index1 == $index2) {
+                        return 0;
+                    } else {
+                        return $index1 < $index2 ? -1 : 1;
+                    }
+                }
+            }
+
+            // None of the ordering arrays contain both elements, so their relative positions in the sorted array
+            // do not matter
+            return 0;
+        };
     }
 }