Fix Patch aliases usage (issue #31396)

lib/internal/Magento/Framework/Setup/Patch/PatchApplier.php

@@ -3,6 +3,7 @@
  * Copyright © Magento, Inc. All rights reserved.
  * See COPYING.txt for license details.
  */
+declare(strict_types=1);
 
 namespace Magento\Framework\Setup\Patch;
 
@@ -153,19 +154,15 @@ public function applyDataPatch($moduleName = null)
                     new Phrase("Patch %1 should implement DataPatchInterface", [get_class($dataPatch)])
                 );
             }
+            if ($this->isApplied($dataPatch)) {
+                continue;
+            }
             if ($dataPatch instanceof NonTransactionableInterface) {
-                $dataPatch->apply();
-                $this->patchHistory->fixPatch(get_class($dataPatch));
+                $this->applyPatch($dataPatch);
             } else {
                 try {
                     $this->moduleDataSetup->getConnection()->beginTransaction();
-                    $dataPatch->apply();
-                    $this->patchHistory->fixPatch(get_class($dataPatch));
-                    foreach ($dataPatch->getAliases() as $patchAlias) {
-                        if (!$this->patchHistory->isApplied($patchAlias)) {
-                            $this->patchHistory->fixPatch($patchAlias);
-                        }
-                    }
+                    $this->applyPatch($dataPatch);
                     $this->moduleDataSetup->getConnection()->commit();
                 } catch (\Exception $e) {
                     $this->moduleDataSetup->getConnection()->rollBack();
@@ -187,35 +184,6 @@ public function applyDataPatch($moduleName = null)
         }
     }
 
-    /**
-     * Register all patches in registry in order to manipulate chains and dependencies of patches of patches
-     *
-     * @param string $moduleName
-     * @param string $patchType
-     * @return PatchRegistry
-     */
-    private function prepareRegistry($moduleName, $patchType)
-    {
-        $reader = $patchType === self::DATA_PATCH ? $this->dataPatchReader : $this->schemaPatchReader;
-        $registry = $this->patchRegistryFactory->create();
-
-        //Prepare modules to read
-        if ($moduleName === null) {
-            $patchNames = [];
-            foreach ($this->moduleList->getNames() as $moduleName) {
-                $patchNames += $reader->read($moduleName);
-            }
-        } else {
-            $patchNames = $reader->read($moduleName);
-        }
-
-        foreach ($patchNames as $patchName) {
-            $registry->registerPatch($patchName);
-        }
-
-        return $registry;
-    }
-
     /**
      * Apply all patches for one module
      *
@@ -240,12 +208,8 @@ public function applySchemaPatch($moduleName = null)
                  * @var SchemaPatchInterface $schemaPatch
                  */
                 $schemaPatch = $this->patchFactory->create($schemaPatch, ['schemaSetup' => $this->schemaSetup]);
-                $schemaPatch->apply();
-                $this->patchHistory->fixPatch(get_class($schemaPatch));
-                foreach ($schemaPatch->getAliases() as $patchAlias) {
-                    if (!$this->patchHistory->isApplied($patchAlias)) {
-                        $this->patchHistory->fixPatch($patchAlias);
-                    }
+                if (!$this->isApplied($schemaPatch)) {
+                    $this->applyPatch($schemaPatch);
                 }
             } catch (\Exception $e) {
                 $schemaPatchClass = is_object($schemaPatch) ? get_class($schemaPatch) : $schemaPatch;
@@ -297,4 +261,69 @@ public function revertDataPatches($moduleName = null)
             }
         }
     }
+
+    /**
+     * Register all patches in registry in order to manipulate chains and dependencies of patches of patches
+     *
+     * @param string $moduleName
+     * @param string $patchType
+     * @return PatchRegistry
+     */
+    private function prepareRegistry(string $moduleName, string $patchType): PatchRegistry
+    {
+        $reader = $patchType === self::DATA_PATCH ? $this->dataPatchReader : $this->schemaPatchReader;
+        $registry = $this->patchRegistryFactory->create();
+
+        //Prepare modules to read
+        if ($moduleName === null) {
+            $patchNames = [];
+            foreach ($this->moduleList->getNames() as $moduleName) {
+                $patchNames += $reader->read($moduleName);
+            }
+        } else {
+            $patchNames = $reader->read($moduleName);
+        }
+
+        foreach ($patchNames as $patchName) {
+            $registry->registerPatch($patchName);
+        }
+
+        return $registry;
+    }
+
+    /**
+     * Apply the given patch. The patch is and its aliases are added to the history.
+     *
+     * @param PatchInterface $patch
+     */
+    private function applyPatch(PatchInterface $patch): void
+    {
+        $patch->apply();
+        $this->patchHistory->fixPatch(get_class($patch));
+        foreach ($patch->getAliases() ?? [] as $patchAlias) {
+            if (!$this->patchHistory->isApplied($patchAlias)) {
+                $this->patchHistory->fixPatch($patchAlias);
+            }
+        }
+    }
+
+    /**
+     * Check wether the given patch or any of its aliases are already applied or not.
+     *
+     * @param PatchInterface $patch
+     */
+    private function isApplied(PatchInterface $patch): bool
+    {
+        $patchIdentity = get_class($patch);
+        if (!$this->patchHistory->isApplied($patchIdentity)) {
+            foreach ($patch->getAliases() ?? [] as $alias) {
+                if ($this->patchHistory->isApplied($alias)) {
+                    $this->patchHistory->fixPatch($patchIdentity);
+                    return true;
+                }
+            }
+        }
+
+        return false;
+    }
 }

lib/internal/Magento/Framework/Setup/Test/Unit/Patch/PatchApplierTest.php

@@ -166,20 +166,17 @@ public function testApplyDataPatchForNewlyInstalledModule($moduleName, $dataPatc
             \OtherDataPatch::class
         ];
         $patchRegistryMock = $this->createAggregateIteratorMock(PatchRegistry::class, $patches, ['registerPatch']);
-        $patchRegistryMock->expects($this->exactly(2))
-            ->method('registerPatch');
+        $patchRegistryMock->expects($this->exactly(2))->method('registerPatch');
 
-        $this->patchRegistryFactoryMock->expects($this->any())
-            ->method('create')
-            ->willReturn($patchRegistryMock);
+        $this->patchRegistryFactoryMock->expects($this->any())->method('create')->willReturn($patchRegistryMock);
         // phpstan:ignore "Class SomeDataPatch not found."
         $patch1 = $this->createMock(\SomeDataPatch::class);
         $patch1->expects($this->once())->method('apply');
-        $patch1->expects($this->once())->method('getAliases')->willReturn([]);
+        $patch1->expects($this->any())->method('getAliases')->willReturn([]);
         // phpstan:ignore "Class OtherDataPatch not found."
         $patch2 = $this->createMock(\OtherDataPatch::class);
         $patch2->expects($this->once())->method('apply');
-        $patch2->expects($this->once())->method('getAliases')->willReturn([]);
+        $patch2->expects($this->any())->method('getAliases')->willReturn([]);
 
         $this->objectManagerMock->expects($this->any())->method('create')->willReturnMap(
             [
@@ -221,7 +218,7 @@ public function testApplyDataPatchForAlias($moduleName, $dataPatches, $moduleVer
         );
 
         $patch1 = $this->getMockForAbstractClass(DataPatchInterface::class);
-        $patch1->expects($this->once())->method('getAliases')->willReturn(['PatchAlias']);
+        $patch1->expects($this->any())->method('getAliases')->willReturn(['PatchAlias']);
         $patchClass = get_class($patch1);
 
         $patchRegistryMock = $this->createAggregateIteratorMock(PatchRegistry::class, [$patchClass], ['registerPatch']);
@@ -326,7 +323,7 @@ public function testApplyDataPatchForInstalledModule($moduleName, $dataPatches,
     public static function applyDataPatchDataInstalledModuleProvider()
     {
         return [
-            'upgrade module iwth only OtherDataPatch' => [
+            'upgrade module with only OtherDataPatch' => [
                 'moduleName' => 'Module1',
                 'dataPatches' => [
                     // phpstan:ignore
@@ -548,7 +545,7 @@ public function testSchemaPatchApplyForPatchAlias($moduleName, $schemaPatches, $
         );
 
         $patch1 = $this->getMockForAbstractClass(PatchInterface::class);
-        $patch1->expects($this->once())->method('getAliases')->willReturn(['PatchAlias']);
+        $patch1->expects($this->any())->method('getAliases')->willReturn(['PatchAlias']);
         $patchClass = get_class($patch1);
 
         $patchRegistryMock = $this->createAggregateIteratorMock(PatchRegistry::class, [$patchClass], ['registerPatch']);
@@ -611,7 +608,7 @@ public function testRevertDataPatches()
     public static function schemaPatchDataProvider()
     {
         return [
-            'upgrade module iwth only OtherSchemaPatch' => [
+            'upgrade module with only OtherSchemaPatch' => [
                 'moduleName' => 'Module1',
                 'schemaPatches' => [
                     // phpstan:ignore