From f8bed9b1a47c6dd2fdb286fe07ed8178fb130fda Mon Sep 17 00:00:00 2001 From: Oleh Usik Date: Wed, 13 Jan 2021 14:52:00 +0200 Subject: [PATCH 1/9] Fixed issue with Data/Schema patch applying --- .../Framework/Setup/Patch/PatchApplier.php | 28 +++++++++++++------ 1 file changed, 20 insertions(+), 8 deletions(-) diff --git a/lib/internal/Magento/Framework/Setup/Patch/PatchApplier.php b/lib/internal/Magento/Framework/Setup/Patch/PatchApplier.php index e1b0e2842628d..36a7198f83173 100644 --- a/lib/internal/Magento/Framework/Setup/Patch/PatchApplier.php +++ b/lib/internal/Magento/Framework/Setup/Patch/PatchApplier.php @@ -159,10 +159,9 @@ public function applyDataPatch($moduleName = null) } else { try { $this->moduleDataSetup->getConnection()->beginTransaction(); - $dataPatch->apply(); - $this->patchHistory->fixPatch(get_class($dataPatch)); - foreach ($dataPatch->getAliases() as $patchAlias) { - $this->patchHistory->fixPatch($patchAlias); + if (!$this->checkPatchAliases($dataPatch)) { + $dataPatch->apply(); + $this->patchHistory->fixPatch(get_class($dataPatch)); } $this->moduleDataSetup->getConnection()->commit(); } catch (\Exception $e) { @@ -238,10 +237,9 @@ 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) { - $this->patchHistory->fixPatch($patchAlias); + if (!$this->checkPatchAliases($schemaPatch)) { + $schemaPatch->apply(); + $this->patchHistory->fixPatch(get_class($schemaPatch)); } } catch (\Exception $e) { throw new SetupException( @@ -292,4 +290,18 @@ public function revertDataPatches($moduleName = null) } } } + + /** + * Checks is patch was applied with current alias name + * + * @param DataPatchInterface|SchemaPatchInterface $dataPatch + * @return bool + */ + private function checkPatchAliases($dataPatch): bool + { + foreach ($dataPatch->getAliases() as $patchAlias) { + return $this->patchHistory->isApplied($patchAlias); + } + return false; + } } From 015ac916445c98e1b0153fb116e91e80c773c9a9 Mon Sep 17 00:00:00 2001 From: Oleh Usik Date: Wed, 13 Jan 2021 15:15:09 +0200 Subject: [PATCH 2/9] minor fix --- lib/internal/Magento/Framework/Setup/Patch/PatchApplier.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/internal/Magento/Framework/Setup/Patch/PatchApplier.php b/lib/internal/Magento/Framework/Setup/Patch/PatchApplier.php index 36a7198f83173..26c6f84e07bab 100644 --- a/lib/internal/Magento/Framework/Setup/Patch/PatchApplier.php +++ b/lib/internal/Magento/Framework/Setup/Patch/PatchApplier.php @@ -300,7 +300,7 @@ public function revertDataPatches($moduleName = null) private function checkPatchAliases($dataPatch): bool { foreach ($dataPatch->getAliases() as $patchAlias) { - return $this->patchHistory->isApplied($patchAlias); + return $this->patchHistory->isApplied($patchAlias); } return false; } From d88bd43265422f11aa339a4f635c213a3d8fbe75 Mon Sep 17 00:00:00 2001 From: Oleh Usik Date: Mon, 15 Feb 2021 14:40:41 +0200 Subject: [PATCH 3/9] Fixed failed tests --- lib/internal/Magento/Framework/Setup/Patch/PatchApplier.php | 6 ++++-- .../Framework/Setup/Test/Unit/Patch/PatchApplierTest.php | 4 ---- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/lib/internal/Magento/Framework/Setup/Patch/PatchApplier.php b/lib/internal/Magento/Framework/Setup/Patch/PatchApplier.php index 26c6f84e07bab..069fac89ceea2 100644 --- a/lib/internal/Magento/Framework/Setup/Patch/PatchApplier.php +++ b/lib/internal/Magento/Framework/Setup/Patch/PatchApplier.php @@ -299,8 +299,10 @@ public function revertDataPatches($moduleName = null) */ private function checkPatchAliases($dataPatch): bool { - foreach ($dataPatch->getAliases() as $patchAlias) { - return $this->patchHistory->isApplied($patchAlias); + if ($dataPatchAliases = $dataPatch->getAliases()) { + foreach ($dataPatchAliases as $patchAlias) { + return $this->patchHistory->isApplied($patchAlias); + } } return false; } diff --git a/lib/internal/Magento/Framework/Setup/Test/Unit/Patch/PatchApplierTest.php b/lib/internal/Magento/Framework/Setup/Test/Unit/Patch/PatchApplierTest.php index 82c267dc7d51f..7674a1c667268 100644 --- a/lib/internal/Magento/Framework/Setup/Test/Unit/Patch/PatchApplierTest.php +++ b/lib/internal/Magento/Framework/Setup/Test/Unit/Patch/PatchApplierTest.php @@ -203,8 +203,6 @@ public function testApplyDataPatchForNewlyInstalledModule($moduleName, $dataPatc */ public function testApplyDataPatchForAlias($moduleName, $dataPatches, $moduleVersionInDb) { - $this->expectException('Exception'); - $this->expectExceptionMessageMatches('"Unable to apply data patch .+ cannot be applied twice"'); $this->dataPatchReaderMock->expects($this->once()) ->method('read') ->with($moduleName) @@ -516,8 +514,6 @@ public function testSchemaPatchAplly($moduleName, $schemaPatches, $moduleVersion */ public function testSchemaPatchApplyForPatchAlias($moduleName, $schemaPatches, $moduleVersionInDb) { - $this->expectException('Exception'); - $this->expectExceptionMessageMatches('"Unable to apply patch .+ cannot be applied twice"'); $this->schemaPatchReaderMock->expects($this->once()) ->method('read') ->with($moduleName) From 13e4a2d362f4313ee0468080ebfc952a9152469e Mon Sep 17 00:00:00 2001 From: Oleh Usik Date: Tue, 16 Feb 2021 13:15:50 +0200 Subject: [PATCH 4/9] Fixed failed tests --- .../Magento/Framework/Setup/Patch/PatchApplier.php | 2 +- .../Setup/Test/Unit/Patch/PatchApplierTest.php | 11 ++--------- 2 files changed, 3 insertions(+), 10 deletions(-) diff --git a/lib/internal/Magento/Framework/Setup/Patch/PatchApplier.php b/lib/internal/Magento/Framework/Setup/Patch/PatchApplier.php index 069fac89ceea2..cac6783b3bbbc 100644 --- a/lib/internal/Magento/Framework/Setup/Patch/PatchApplier.php +++ b/lib/internal/Magento/Framework/Setup/Patch/PatchApplier.php @@ -301,7 +301,7 @@ private function checkPatchAliases($dataPatch): bool { if ($dataPatchAliases = $dataPatch->getAliases()) { foreach ($dataPatchAliases as $patchAlias) { - return $this->patchHistory->isApplied($patchAlias); + return $this->patchHistory->isApplied($patchAlias) ?? true; } } return false; diff --git a/lib/internal/Magento/Framework/Setup/Test/Unit/Patch/PatchApplierTest.php b/lib/internal/Magento/Framework/Setup/Test/Unit/Patch/PatchApplierTest.php index 7674a1c667268..98fb52ceabf76 100644 --- a/lib/internal/Magento/Framework/Setup/Test/Unit/Patch/PatchApplierTest.php +++ b/lib/internal/Magento/Framework/Setup/Test/Unit/Patch/PatchApplierTest.php @@ -24,6 +24,7 @@ use Magento\Framework\Setup\SchemaSetupInterface; use Magento\Framework\Setup\SetupInterface; use Magento\Framework\TestFramework\Unit\Helper\ObjectManager; +use phpDocumentor\Reflection\Types\True_; use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; @@ -216,6 +217,7 @@ public function testApplyDataPatchForAlias($moduleName, $dataPatches, $moduleVer $patch1 = $this->getMockForAbstractClass(DataPatchInterface::class); $patch1->expects($this->once())->method('getAliases')->willReturn(['PatchAlias']); + $patch1->expects($this->never())->method('apply'); $patchClass = get_class($patch1); $patchRegistryMock = $this->createAggregateIteratorMock(PatchRegistry::class, [$patchClass], ['registerPatch']); @@ -231,15 +233,6 @@ public function testApplyDataPatchForAlias($moduleName, $dataPatches, $moduleVer ['\\' . $patchClass, ['moduleDataSetup' => $this->moduleDataSetupMock], $patch1], ] ); - $this->connectionMock->expects($this->exactly(1))->method('beginTransaction'); - $this->connectionMock->expects($this->never())->method('commit'); - $this->patchHistoryMock->expects($this->any())->method('fixPatch')->willReturnCallback( - function ($param1) { - if ($param1 == 'PatchAlias') { - throw new \LogicException(sprintf("Patch %s cannot be applied twice", $param1)); - } - } - ); $this->patchApllier->applyDataPatch($moduleName); } From 31f3498a0427427d9c008ca738f5801eb4277e02 Mon Sep 17 00:00:00 2001 From: Matthijs Breed Date: Wed, 28 Jun 2023 12:39:39 +0200 Subject: [PATCH 5/9] Update to Usik2203's PR branch, account for applied patch aliases, update patch aliases in db regardless of patch application --- .../Framework/Setup/Patch/PatchApplier.php | 38 ++++++++++++++----- 1 file changed, 29 insertions(+), 9 deletions(-) diff --git a/lib/internal/Magento/Framework/Setup/Patch/PatchApplier.php b/lib/internal/Magento/Framework/Setup/Patch/PatchApplier.php index b90dd9c3b1a6f..0590b4dc54cb7 100644 --- a/lib/internal/Magento/Framework/Setup/Patch/PatchApplier.php +++ b/lib/internal/Magento/Framework/Setup/Patch/PatchApplier.php @@ -159,10 +159,11 @@ public function applyDataPatch($moduleName = null) } else { try { $this->moduleDataSetup->getConnection()->beginTransaction(); - if (!$this->checkPatchAliases($dataPatch)) { + if ($this->shouldApply($dataPatch)) { $dataPatch->apply(); $this->patchHistory->fixPatch(get_class($dataPatch)); } + $this->fixAliases($dataPatch); $this->moduleDataSetup->getConnection()->commit(); } catch (\Exception $e) { $this->moduleDataSetup->getConnection()->rollBack(); @@ -237,10 +238,11 @@ public function applySchemaPatch($moduleName = null) * @var SchemaPatchInterface $schemaPatch */ $schemaPatch = $this->patchFactory->create($schemaPatch, ['schemaSetup' => $this->schemaSetup]); - if (!$this->checkPatchAliases($schemaPatch)) { + if ($this->shouldApply($schemaPatch)) { $schemaPatch->apply(); $this->patchHistory->fixPatch(get_class($schemaPatch)); } + $this->fixAliases($schemaPatch); } catch (\Exception $e) { throw new SetupException( new Phrase( @@ -292,18 +294,36 @@ public function revertDataPatches($moduleName = null) } /** - * Checks is patch was applied with current alias name + * Checks if patch should be applied by already applied patch alias names * - * @param DataPatchInterface|SchemaPatchInterface $dataPatch + * @param DataPatchInterface|SchemaPatchInterface $patch * @return bool */ - private function checkPatchAliases($dataPatch): bool + private function shouldApply($patch): bool { - if ($dataPatchAliases = $dataPatch->getAliases()) { - foreach ($dataPatchAliases as $patchAlias) { - return $this->patchHistory->isApplied($patchAlias) ?? true; + $shouldApply = true; + if ($patch = $patch->getAliases()) { + foreach ($patch as $patchAlias) { + if ($this->patchHistory->isApplied($patchAlias)) { + $shouldApply = false; + } + } + } + return $shouldApply; + } + + /** + * Save all patch aliases + * + * @param $patch + * @return void + */ + private function fixAliases($patch) + { + foreach ($patch->getAliases() as $patchAlias) { + if (!$this->patchHistory->isApplied($patchAlias)) { + $this->patchHistory->fixPatch($patchAlias); } } - return false; } } From 6881afe102a28efeb6941e7fa6b4c7f558d4eba2 Mon Sep 17 00:00:00 2001 From: Matthijs Breed Date: Wed, 28 Jun 2023 17:35:18 +0200 Subject: [PATCH 6/9] Fix testcases --- .../Magento/Framework/Setup/Patch/PatchApplier.php | 10 +++++----- .../Setup/Test/Unit/Patch/PatchApplierTest.php | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/lib/internal/Magento/Framework/Setup/Patch/PatchApplier.php b/lib/internal/Magento/Framework/Setup/Patch/PatchApplier.php index 0590b4dc54cb7..34da2bfbbacbf 100644 --- a/lib/internal/Magento/Framework/Setup/Patch/PatchApplier.php +++ b/lib/internal/Magento/Framework/Setup/Patch/PatchApplier.php @@ -299,7 +299,7 @@ public function revertDataPatches($moduleName = null) * @param DataPatchInterface|SchemaPatchInterface $patch * @return bool */ - private function shouldApply($patch): bool + private function shouldApply(DataPatchInterface|SchemaPatchInterface$patch): bool { $shouldApply = true; if ($patch = $patch->getAliases()) { @@ -313,12 +313,12 @@ private function shouldApply($patch): bool } /** - * Save all patch aliases - * - * @param $patch + * Save all patch aliases + * + * @param DataPatchInterface|SchemaPatchInterface $patch * @return void */ - private function fixAliases($patch) + private function fixAliases(DataPatchInterface|SchemaPatchInterface $patch): void { foreach ($patch->getAliases() as $patchAlias) { if (!$this->patchHistory->isApplied($patchAlias)) { diff --git a/lib/internal/Magento/Framework/Setup/Test/Unit/Patch/PatchApplierTest.php b/lib/internal/Magento/Framework/Setup/Test/Unit/Patch/PatchApplierTest.php index 11f317dc9ee7c..8a48da03e7ad7 100644 --- a/lib/internal/Magento/Framework/Setup/Test/Unit/Patch/PatchApplierTest.php +++ b/lib/internal/Magento/Framework/Setup/Test/Unit/Patch/PatchApplierTest.php @@ -180,7 +180,7 @@ public function testApplyDataPatchForNewlyInstalledModule($moduleName, $dataPatc // 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->exactly(2))->method('getAliases')->willReturn([]); $this->objectManagerMock->expects($this->any())->method('create')->willReturnMap( [ From 8ddedc8d34d4e19f904c75417f3b64116111d301 Mon Sep 17 00:00:00 2001 From: Matthijs Breed Date: Thu, 20 Jul 2023 18:22:13 +0200 Subject: [PATCH 7/9] Applied feedback, fixed unit tests --- .../Framework/Setup/Patch/PatchApplier.php | 18 ++++++++---------- .../Setup/Test/Unit/Patch/PatchApplierTest.php | 14 +++++++++----- 2 files changed, 17 insertions(+), 15 deletions(-) diff --git a/lib/internal/Magento/Framework/Setup/Patch/PatchApplier.php b/lib/internal/Magento/Framework/Setup/Patch/PatchApplier.php index 34da2bfbbacbf..72c9eef726173 100644 --- a/lib/internal/Magento/Framework/Setup/Patch/PatchApplier.php +++ b/lib/internal/Magento/Framework/Setup/Patch/PatchApplier.php @@ -294,19 +294,17 @@ public function revertDataPatches($moduleName = null) } /** - * Checks if patch should be applied by already applied patch alias names + * Check if patch should be applied by already applied patch alias names * - * @param DataPatchInterface|SchemaPatchInterface $patch + * @param PatchInterface $patch * @return bool */ - private function shouldApply(DataPatchInterface|SchemaPatchInterface$patch): bool + private function shouldApply(PatchInterface $patch): bool { $shouldApply = true; - if ($patch = $patch->getAliases()) { - foreach ($patch as $patchAlias) { - if ($this->patchHistory->isApplied($patchAlias)) { - $shouldApply = false; - } + foreach ($patch->getAliases() as $patchAlias) { + if ($this->patchHistory->isApplied($patchAlias)) { + $shouldApply = false; } } return $shouldApply; @@ -315,10 +313,10 @@ private function shouldApply(DataPatchInterface|SchemaPatchInterface$patch): boo /** * Save all patch aliases * - * @param DataPatchInterface|SchemaPatchInterface $patch + * @param PatchInterface $patch * @return void */ - private function fixAliases(DataPatchInterface|SchemaPatchInterface $patch): void + private function fixAliases(PatchInterface $patch): void { foreach ($patch->getAliases() as $patchAlias) { if (!$this->patchHistory->isApplied($patchAlias)) { diff --git a/lib/internal/Magento/Framework/Setup/Test/Unit/Patch/PatchApplierTest.php b/lib/internal/Magento/Framework/Setup/Test/Unit/Patch/PatchApplierTest.php index 8a48da03e7ad7..0d57921e751be 100644 --- a/lib/internal/Magento/Framework/Setup/Test/Unit/Patch/PatchApplierTest.php +++ b/lib/internal/Magento/Framework/Setup/Test/Unit/Patch/PatchApplierTest.php @@ -24,7 +24,6 @@ use Magento\Framework\Setup\SchemaSetupInterface; use Magento\Framework\Setup\SetupInterface; use Magento\Framework\TestFramework\Unit\Helper\ObjectManager; -use phpDocumentor\Reflection\Types\True_; use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; @@ -176,7 +175,7 @@ public function testApplyDataPatchForNewlyInstalledModule($moduleName, $dataPatc // 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->exactly(2))->method('getAliases')->willReturn([]); // phpstan:ignore "Class OtherDataPatch not found." $patch2 = $this->createMock(\OtherDataPatch::class); $patch2->expects($this->once())->method('apply'); @@ -220,9 +219,13 @@ public function testApplyDataPatchForAlias($moduleName, $dataPatches, $moduleVer [$moduleName, $moduleVersionInDb] ] ); - + $this->patchHistoryMock->method('isApplied')->willReturnCallback( + function ($patchAlias) { + return in_array($patchAlias, ['PatchAlias']); + } + ); $patch1 = $this->getMockForAbstractClass(DataPatchInterface::class); - $patch1->expects($this->once())->method('getAliases')->willReturn(['PatchAlias']); + $patch1->expects($this->exactly(2))->method('getAliases')->willReturn(['PatchAlias']); $patch1->expects($this->never())->method('apply'); $patchClass = get_class($patch1); @@ -239,6 +242,7 @@ public function testApplyDataPatchForAlias($moduleName, $dataPatches, $moduleVer ['\\' . $patchClass, ['moduleDataSetup' => $this->moduleDataSetupMock], $patch1], ] ); + $this->patchApllier->applyDataPatch($moduleName); } @@ -550,7 +554,7 @@ public function testSchemaPatchApplyForPatchAlias($moduleName, $schemaPatches, $ ); $patch1 = $this->getMockForAbstractClass(PatchInterface::class); - $patch1->expects($this->once())->method('getAliases')->willReturn(['PatchAlias']); + $patch1->expects($this->exactly(2))->method('getAliases')->willReturn(['PatchAlias']); $patchClass = get_class($patch1); $patchRegistryMock = $this->createAggregateIteratorMock(PatchRegistry::class, [$patchClass], ['registerPatch']); From 76e06f1144f511626734c5b86ea3c045a79b571d Mon Sep 17 00:00:00 2001 From: Matthijs Breed Date: Thu, 20 Jul 2023 18:34:59 +0200 Subject: [PATCH 8/9] Fix 1 more test --- .../Magento/Framework/Setup/Test/Unit/Patch/PatchApplierTest.php | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/internal/Magento/Framework/Setup/Test/Unit/Patch/PatchApplierTest.php b/lib/internal/Magento/Framework/Setup/Test/Unit/Patch/PatchApplierTest.php index 0d57921e751be..6c251d78a1483 100644 --- a/lib/internal/Magento/Framework/Setup/Test/Unit/Patch/PatchApplierTest.php +++ b/lib/internal/Magento/Framework/Setup/Test/Unit/Patch/PatchApplierTest.php @@ -387,6 +387,7 @@ public function testApplyDataPatchRollback($moduleName, $dataPatches, $moduleVer $patch1->expects($this->never())->method('apply'); // phpstan:ignore "Class OtherDataPatch not found." $patch2 = $this->createMock(\OtherDataPatch::class); + $patch2->expects($this->exactly(1))->method('getAliases')->willReturn([]); $exception = new \Exception('Patch Apply Error'); $patch2->expects($this->once())->method('apply')->willThrowException($exception); From 022190babdefbcf40da608455809ac26a05d3592 Mon Sep 17 00:00:00 2001 From: Matthijs Breed Date: Fri, 15 Sep 2023 11:05:04 +0200 Subject: [PATCH 9/9] [FEEDBACK] Early return to prevent unneeded iterations --- lib/internal/Magento/Framework/Setup/Patch/PatchApplier.php | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/lib/internal/Magento/Framework/Setup/Patch/PatchApplier.php b/lib/internal/Magento/Framework/Setup/Patch/PatchApplier.php index 72c9eef726173..2fd2740001d67 100644 --- a/lib/internal/Magento/Framework/Setup/Patch/PatchApplier.php +++ b/lib/internal/Magento/Framework/Setup/Patch/PatchApplier.php @@ -301,13 +301,12 @@ public function revertDataPatches($moduleName = null) */ private function shouldApply(PatchInterface $patch): bool { - $shouldApply = true; foreach ($patch->getAliases() as $patchAlias) { if ($this->patchHistory->isApplied($patchAlias)) { - $shouldApply = false; + return false; } } - return $shouldApply; + return true; } /**