Skip to content

[Fixed: #31396] Data/Schema Patches getAliases() not working as expected #31635

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 7 commits into
base: 2.4-develop
Choose a base branch
from
Open

[Fixed: #31396] Data/Schema Patches getAliases() not working as expected #31635

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f8bed9b
Fixed issue with Data/Schema patch applying
Usik2203 Jan 13, 2021
015ac91
minor fix
Usik2203 Jan 13, 2021
05aba68
Merge branch '2.4-develop' into data-patch-fix
Usik2203 Feb 3, 2021
fbca506
Merge branch '2.4-develop' into data-patch-fix
Usik2203 Feb 15, 2021
d88bd43
Fixed failed tests
Usik2203 Feb 15, 2021
13e4a2d
Fixed failed tests
Usik2203 Feb 16, 2021
391d60e
Merge branch '2.4-develop' into data-patch-fix
Usik2203 Feb 16, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 22 additions & 8 deletions lib/internal/Magento/Framework/Setup/Patch/PatchApplier.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Usik2203 We should still also loop over every alias and mark them as applied. In the case where a patch file is renamed (e.g. changing the namespace of a module), if we install the new version and only mark the new name as installed, rolling back to before the change will result in the same patch applying a second time. If we mark all aliases as installed, then it will prevent that from happening.

}
} catch (\Exception $e) {
throw new SetupException(
Expand Down Expand Up @@ -292,4 +290,20 @@ 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
{
if ($dataPatchAliases = $dataPatch->getAliases()) {
foreach ($dataPatchAliases as $patchAlias) {
return $this->patchHistory->isApplied($patchAlias) ?? true;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Usik2203 I don't see how this will work if you have more than one alias. This line will cause the function to return on the first test, no matter what, when it should only return if it finds an alias that is applied (and then default to false if none have been installed).
cc: @rogyar

}
}
return false;
}
}
15 changes: 2 additions & 13 deletions lib/internal/Magento/Framework/Setup/Test/Unit/Patch/PatchApplierTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -203,8 +204,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)
Expand All @@ -218,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']);
Expand All @@ -233,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);
}

Expand Down Expand Up @@ -516,8 +507,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)
Expand Down