From afd82fb9a5d51763f255decedbd3ef09cc981ef2 Mon Sep 17 00:00:00 2001 From: Ingolf Steinhardt Date: Fri, 19 Jun 2026 12:08:51 +0200 Subject: [PATCH 1/4] Fix copy translated attributes with manual sorting --- src/CoreBundle/Resources/config/listeners.yml | 3 + .../Events/MetaModel/CopyTranslatedData.php | 91 ++++++++- .../MetaModel/CopyTranslatedDataTest.php | 184 ++++++++++++++---- 3 files changed, 235 insertions(+), 43 deletions(-) diff --git a/src/CoreBundle/Resources/config/listeners.yml b/src/CoreBundle/Resources/config/listeners.yml index 2db7e2d24..bc57c45b1 100644 --- a/src/CoreBundle/Resources/config/listeners.yml +++ b/src/CoreBundle/Resources/config/listeners.yml @@ -56,6 +56,9 @@ services: - name: kernel.event_listener event: dc-general.model.post-duplicate method: handle + - name: kernel.event_listener + event: dc-general.model.post-paste + method: handlePostPaste metamodels.reset_language_after_duplicate_listener: class: MetaModels\DcGeneral\Events\MetaModel\ResetLanguageAfterDuplicate diff --git a/src/DcGeneral/Events/MetaModel/CopyTranslatedData.php b/src/DcGeneral/Events/MetaModel/CopyTranslatedData.php index bed6dc1c6..73a134e02 100644 --- a/src/DcGeneral/Events/MetaModel/CopyTranslatedData.php +++ b/src/DcGeneral/Events/MetaModel/CopyTranslatedData.php @@ -20,16 +20,23 @@ namespace MetaModels\DcGeneral\Events\MetaModel; use ContaoCommunityAlliance\DcGeneral\Event\PostDuplicateModelEvent; -use MetaModels\Attribute\ITranslatedWithFallbackControl; +use ContaoCommunityAlliance\DcGeneral\Event\PostPasteModelEvent; +use MetaModels\Attribute\ITranslated; use MetaModels\IFactory; use MetaModels\ITranslatedMetaModel; /** * Copies translated attribute data for all languages when an item is duplicated. * - * The normal DC_General duplicate path only saves data for the currently active language. This listener runs after - * the copy has been persisted and iterates every language, copying each translated attribute's raw (non-fallback) data - * from the source item to the new item. + * The normal DC_General duplicate path only saves data for the currently active language. This listener iterates every + * language and copies each translated attribute's raw (non-fallback) data from the source item to the new item. + * + * Two paths lead here: + * - The simple copy action (Contao2BackendView CopyHandler) persists the clone first and then dispatches the + * post-duplicate event, so the new item already has an id and the copy runs right away. + * - The clipboard / manual sorting paste path (DefaultController::doCloneAction) dispatches the post-duplicate event + * *before* the clone is persisted, so the new item has no id yet. In that case we remember the source id and run + * the copy on the subsequent post-paste event, when the clone has been saved and carries a real id. */ final class CopyTranslatedData { @@ -40,6 +47,16 @@ final class CopyTranslatedData */ private IFactory $factory; + /** + * Source item ids for clones whose copy was deferred because the new item had no id yet. + * + * Keyed by the spl_object_id() of the (not yet persisted) new model. The clipboard paste path reuses the very same + * model instance for the later post-paste event, so the object id is a stable correlation key within the request. + * + * @var array + */ + private array $deferredSourceIds = []; + /** * Create a new instance. * @@ -59,14 +76,68 @@ public function __construct(IFactory $factory) */ public function handle(PostDuplicateModelEvent $event): void { + $newModel = $event->getModel(); $sourceId = (string) $event->getSourceModel()->getId(); - $newId = (string) $event->getModel()->getId(); + $newId = (string) $newModel->getId(); + + if ('' === $sourceId) { + return; + } + + // Clipboard / manual sorting path: the clone has not been persisted yet and therefore has no id. Defer the + // copy until the post-paste event fires with a real id. + if ('' === $newId) { + $this->deferredSourceIds[\spl_object_id($newModel)] = $sourceId; - if ('' === $sourceId || '' === $newId || $sourceId === $newId) { return; } - $metaModel = $this->factory->getMetaModel($event->getModel()->getProviderName()); + if ($sourceId === $newId) { + return; + } + + $this->copyAllLanguages($newModel->getProviderName(), $sourceId, $newId); + } + + /** + * Run a deferred copy after the clone has been persisted via the clipboard paste path. + * + * @param PostPasteModelEvent $event The event. + * + * @return void + */ + public function handlePostPaste(PostPasteModelEvent $event): void + { + $model = $event->getModel(); + $objectId = \spl_object_id($model); + + if (!isset($this->deferredSourceIds[$objectId])) { + return; + } + + $sourceId = $this->deferredSourceIds[$objectId]; + unset($this->deferredSourceIds[$objectId]); + + $newId = (string) $model->getId(); + if ('' === $newId || $sourceId === $newId) { + return; + } + + $this->copyAllLanguages($model->getProviderName(), $sourceId, $newId); + } + + /** + * Copy all translated attributes for every language of the MetaModel. + * + * @param string $providerName The MetaModel name. + * @param string $sourceId The source item ID. + * @param string $newId The new item ID. + * + * @return void + */ + private function copyAllLanguages(string $providerName, string $sourceId, string $newId): void + { + $metaModel = $this->factory->getMetaModel($providerName); if (!$metaModel instanceof ITranslatedMetaModel) { return; } @@ -93,16 +164,16 @@ private function copyLanguage( string $newId ): void { foreach ($metaModel->getAttributes() as $attribute) { - if (!$attribute instanceof ITranslatedWithFallbackControl) { + if (!$attribute instanceof ITranslated) { continue; } - $data = $attribute->getTranslatedDataForWithoutFallback([$sourceId], $language); + $data = $attribute->getTranslatedDataFor([$sourceId], $language); if ([] === $data || !\array_key_exists($sourceId, $data)) { continue; } - $attribute->applyTranslatedDataFor([$newId => $data[$sourceId]], $language); + $attribute->setTranslatedDataFor([$newId => $data[$sourceId]], $language); } } } diff --git a/tests/DcGeneral/Events/MetaModel/CopyTranslatedDataTest.php b/tests/DcGeneral/Events/MetaModel/CopyTranslatedDataTest.php index 1c41188c4..6748a0e0b 100644 --- a/tests/DcGeneral/Events/MetaModel/CopyTranslatedDataTest.php +++ b/tests/DcGeneral/Events/MetaModel/CopyTranslatedDataTest.php @@ -22,8 +22,9 @@ use ContaoCommunityAlliance\DcGeneral\Data\ModelInterface; use ContaoCommunityAlliance\DcGeneral\EnvironmentInterface; use ContaoCommunityAlliance\DcGeneral\Event\PostDuplicateModelEvent; +use ContaoCommunityAlliance\DcGeneral\Event\PostPasteModelEvent; +use MetaModels\Attribute\IAttribute; use MetaModels\Attribute\ITranslated; -use MetaModels\Attribute\ITranslatedWithFallbackControl; use MetaModels\DcGeneral\Events\MetaModel\CopyTranslatedData; use MetaModels\IFactory; use MetaModels\IMetaModel; @@ -33,6 +34,8 @@ /** * @covers \MetaModels\DcGeneral\Events\MetaModel\CopyTranslatedData + * + * @SuppressWarnings(PHPMD.TooManyPublicMethods) */ class CopyTranslatedDataTest extends TestCase { @@ -50,8 +53,6 @@ private function buildEvent( string $newId, string $providerName = 'mm_test' ): PostDuplicateModelEvent { - $environment = $this->getMockForAbstractClass(EnvironmentInterface::class); - $sourceModel = $this->getMockForAbstractClass(ModelInterface::class); $sourceModel->method('getId')->willReturn($sourceId); @@ -59,9 +60,40 @@ private function buildEvent( $newModel->method('getId')->willReturn($newId); $newModel->method('getProviderName')->willReturn($providerName); + return $this->buildDuplicateEvent($sourceModel, $newModel); + } + + /** + * Build a PostDuplicateModelEvent for the given source and new model. + * + * @param ModelInterface $sourceModel The source model. + * @param ModelInterface $newModel The duplicated (new) model. + * + * @return PostDuplicateModelEvent + */ + private function buildDuplicateEvent( + ModelInterface $sourceModel, + ModelInterface $newModel + ): PostDuplicateModelEvent { + $environment = $this->getMockForAbstractClass(EnvironmentInterface::class); + return new PostDuplicateModelEvent($environment, $newModel, $sourceModel); } + /** + * Build a PostPasteModelEvent for the given model. + * + * @param ModelInterface $model The pasted (persisted) model. + * + * @return PostPasteModelEvent + */ + private function buildPasteEvent(ModelInterface $model): PostPasteModelEvent + { + $environment = $this->getMockForAbstractClass(EnvironmentInterface::class); + + return new PostPasteModelEvent($environment, $model); + } + /** * Build a CopyTranslatedData listener with the given MetaModel (or null for "not found"). * @@ -106,9 +138,10 @@ public function testHandleDoesNothingWhenSourceIdIsEmpty(): void } /** - * Nothing happens when new ID is empty. + * When the new ID is empty (clipboard / manual sorting path), the copy is deferred: nothing is copied on the + * duplicate event itself. */ - public function testHandleDoesNothingWhenNewIdIsEmpty(): void + public function testHandleDefersCopyWhenNewIdIsEmpty(): void { /** @var ITranslatedMetaModel&MockObject $metaModel */ $metaModel = $this->getMockForAbstractClass(ITranslatedMetaModel::class); @@ -132,32 +165,32 @@ public function testHandleDoesNothingForNonTranslatedMetaModel(): void } /** - * Attributes that only implement ITranslated (not ITranslatedWithFallbackControl) are skipped. + * Attributes that do not implement ITranslated are skipped. */ - public function testHandleSkipsAttributesWithoutFallbackControlInterface(): void + public function testHandleSkipsNonTranslatedAttributes(): void { - /** @var ITranslated&MockObject $attribute */ - $attribute = $this->getMockForAbstractClass(ITranslated::class); - $attribute->expects(self::never())->method('setTranslatedDataFor'); + /** @var IAttribute&MockObject $attribute */ + $attribute = $this->getMockForAbstractClass(IAttribute::class); /** @var ITranslatedMetaModel&MockObject $metaModel */ $metaModel = $this->getMockForAbstractClass(ITranslatedMetaModel::class); $metaModel->method('getLanguages')->willReturn(['de', 'en']); - $metaModel->method('getAttributes')->willReturn([$attribute]); + // The attributes are inspected for both languages; a non-translated attribute is simply skipped without error. + $metaModel->expects(self::exactly(2))->method('getAttributes')->willReturn([$attribute]); $listener = $this->buildListener($metaModel); $listener->handle($this->buildEvent('1', '2')); } /** - * Languages for which the attribute holds no data are skipped (applyTranslatedDataFor not called). + * Languages for which the attribute holds no data are skipped (setTranslatedDataFor not called). */ public function testHandleSkipsLanguagesWithNoData(): void { - /** @var ITranslatedWithFallbackControl&MockObject $attribute */ - $attribute = $this->getMockForAbstractClass(ITranslatedWithFallbackControl::class); - $attribute->method('getTranslatedDataForWithoutFallback')->willReturn([]); - $attribute->expects(self::never())->method('applyTranslatedDataFor'); + /** @var ITranslated&MockObject $attribute */ + $attribute = $this->getMockForAbstractClass(ITranslated::class); + $attribute->method('getTranslatedDataFor')->willReturn([]); + $attribute->expects(self::never())->method('setTranslatedDataFor'); /** @var ITranslatedMetaModel&MockObject $metaModel */ $metaModel = $this->getMockForAbstractClass(ITranslatedMetaModel::class); @@ -169,7 +202,7 @@ public function testHandleSkipsLanguagesWithNoData(): void } /** - * Happy path: for each language and attribute with data, applyTranslatedDataFor is called with the new ID. + * Happy path: for each language and attribute with data, setTranslatedDataFor is called with the new ID. */ public function testHandleCopiesDataForAllLanguages(): void { @@ -179,14 +212,14 @@ public function testHandleCopiesDataForAllLanguages(): void $dataDE = ['item_id' => $sourceId, 'langcode' => 'de', 'value' => 'Hallo']; $dataEN = ['item_id' => $sourceId, 'langcode' => 'en', 'value' => 'Hello']; - /** @var ITranslatedWithFallbackControl&MockObject $attribute */ - $attribute = $this->getMockForAbstractClass(ITranslatedWithFallbackControl::class); - $attribute->method('getTranslatedDataForWithoutFallback')->willReturnMap([ + /** @var ITranslated&MockObject $attribute */ + $attribute = $this->getMockForAbstractClass(ITranslated::class); + $attribute->method('getTranslatedDataFor')->willReturnMap([ [[$sourceId], 'de', [$sourceId => $dataDE]], [[$sourceId], 'en', [$sourceId => $dataEN]], ]); - $attribute->expects(self::exactly(2))->method('applyTranslatedDataFor')->willReturnCallback( + $attribute->expects(self::exactly(2))->method('setTranslatedDataFor')->willReturnCallback( static function (array $arrValues, string $lang) use ($newId, $dataDE, $dataEN): void { self::assertArrayHasKey($newId, $arrValues, "New ID must be the array key for lang={$lang}"); if ('de' === $lang) { @@ -216,14 +249,14 @@ public function testHandleCopiesOnlyLanguagesWithActualData(): void $dataDE = ['item_id' => $sourceId, 'langcode' => 'de', 'value' => 'Hallo']; - /** @var ITranslatedWithFallbackControl&MockObject $attribute */ - $attribute = $this->getMockForAbstractClass(ITranslatedWithFallbackControl::class); - $attribute->method('getTranslatedDataForWithoutFallback')->willReturnMap([ + /** @var ITranslated&MockObject $attribute */ + $attribute = $this->getMockForAbstractClass(ITranslated::class); + $attribute->method('getTranslatedDataFor')->willReturnMap([ [[$sourceId], 'de', [$sourceId => $dataDE]], [[$sourceId], 'en', []], ]); - $attribute->expects(self::once())->method('applyTranslatedDataFor') + $attribute->expects(self::once())->method('setTranslatedDataFor') ->with([$newId => $dataDE], 'de'); /** @var ITranslatedMetaModel&MockObject $metaModel */ @@ -246,15 +279,15 @@ public function testHandleProcessesMultipleAttributes(): void $data1 = ['item_id' => $sourceId, 'langcode' => 'de', 'value' => 'Attr1']; $data2 = ['item_id' => $sourceId, 'langcode' => 'de', 'value' => 'Attr2']; - /** @var ITranslatedWithFallbackControl&MockObject $attribute1 */ - $attribute1 = $this->getMockForAbstractClass(ITranslatedWithFallbackControl::class); - $attribute1->method('getTranslatedDataForWithoutFallback')->willReturn([$sourceId => $data1]); - $attribute1->expects(self::once())->method('applyTranslatedDataFor')->with([$newId => $data1], 'de'); + /** @var ITranslated&MockObject $attribute1 */ + $attribute1 = $this->getMockForAbstractClass(ITranslated::class); + $attribute1->method('getTranslatedDataFor')->willReturn([$sourceId => $data1]); + $attribute1->expects(self::once())->method('setTranslatedDataFor')->with([$newId => $data1], 'de'); - /** @var ITranslatedWithFallbackControl&MockObject $attribute2 */ - $attribute2 = $this->getMockForAbstractClass(ITranslatedWithFallbackControl::class); - $attribute2->method('getTranslatedDataForWithoutFallback')->willReturn([$sourceId => $data2]); - $attribute2->expects(self::once())->method('applyTranslatedDataFor')->with([$newId => $data2], 'de'); + /** @var ITranslated&MockObject $attribute2 */ + $attribute2 = $this->getMockForAbstractClass(ITranslated::class); + $attribute2->method('getTranslatedDataFor')->willReturn([$sourceId => $data2]); + $attribute2->expects(self::once())->method('setTranslatedDataFor')->with([$newId => $data2], 'de'); /** @var ITranslatedMetaModel&MockObject $metaModel */ $metaModel = $this->getMockForAbstractClass(ITranslatedMetaModel::class); @@ -264,4 +297,89 @@ public function testHandleProcessesMultipleAttributes(): void $listener = $this->buildListener($metaModel); $listener->handle($this->buildEvent($sourceId, $newId)); } + + /** + * Clipboard / manual sorting path: the duplicate event has no new ID yet, so the copy is deferred and runs on the + * subsequent post-paste event with the persisted ID. + */ + public function testHandlePostPasteCopiesDeferredData(): void + { + $sourceId = '10'; + $newId = '99'; + + $dataDE = ['item_id' => $sourceId, 'langcode' => 'de', 'value' => 'Hallo']; + + /** @var ITranslated&MockObject $attribute */ + $attribute = $this->getMockForAbstractClass(ITranslated::class); + $attribute->method('getTranslatedDataFor')->willReturn([$sourceId => $dataDE]); + $attribute->expects(self::once())->method('setTranslatedDataFor')->with([$newId => $dataDE], 'de'); + + /** @var ITranslatedMetaModel&MockObject $metaModel */ + $metaModel = $this->getMockForAbstractClass(ITranslatedMetaModel::class); + $metaModel->method('getLanguages')->willReturn(['de']); + $metaModel->method('getAttributes')->willReturn([$attribute]); + + $sourceModel = $this->getMockForAbstractClass(ModelInterface::class); + $sourceModel->method('getId')->willReturn($sourceId); + + // The clipboard path reuses the very same model instance: it has no id on duplicate and the real id on paste. + $newModel = $this->getMockForAbstractClass(ModelInterface::class); + $newModel->method('getId')->willReturnOnConsecutiveCalls('', $newId); + $newModel->method('getProviderName')->willReturn('mm_test'); + + $listener = $this->buildListener($metaModel); + $listener->handle($this->buildDuplicateEvent($sourceModel, $newModel)); + $listener->handlePostPaste($this->buildPasteEvent($newModel)); + } + + /** + * Post-paste events for models that were not duplicated through this listener (e.g. plain moves) are ignored. + */ + public function testHandlePostPasteIgnoresUnknownModel(): void + { + /** @var ITranslatedMetaModel&MockObject $metaModel */ + $metaModel = $this->getMockForAbstractClass(ITranslatedMetaModel::class); + $metaModel->expects(self::never())->method('getLanguages'); + + $model = $this->getMockForAbstractClass(ModelInterface::class); + $model->method('getId')->willReturn('99'); + $model->method('getProviderName')->willReturn('mm_test'); + + $listener = $this->buildListener($metaModel); + $listener->handlePostPaste($this->buildPasteEvent($model)); + } + + /** + * A deferred copy is consumed only once: a second post-paste event for the same model does nothing. + */ + public function testHandlePostPasteConsumesDeferredEntryOnlyOnce(): void + { + $sourceId = '10'; + $newId = '99'; + + $dataDE = ['item_id' => $sourceId, 'langcode' => 'de', 'value' => 'Hallo']; + + /** @var ITranslated&MockObject $attribute */ + $attribute = $this->getMockForAbstractClass(ITranslated::class); + $attribute->method('getTranslatedDataFor')->willReturn([$sourceId => $dataDE]); + $attribute->expects(self::once())->method('setTranslatedDataFor')->with([$newId => $dataDE], 'de'); + + /** @var ITranslatedMetaModel&MockObject $metaModel */ + $metaModel = $this->getMockForAbstractClass(ITranslatedMetaModel::class); + $metaModel->method('getLanguages')->willReturn(['de']); + $metaModel->method('getAttributes')->willReturn([$attribute]); + + $sourceModel = $this->getMockForAbstractClass(ModelInterface::class); + $sourceModel->method('getId')->willReturn($sourceId); + + $newModel = $this->getMockForAbstractClass(ModelInterface::class); + $newModel->method('getId')->willReturnOnConsecutiveCalls('', $newId, $newId); + $newModel->method('getProviderName')->willReturn('mm_test'); + + $listener = $this->buildListener($metaModel); + $listener->handle($this->buildDuplicateEvent($sourceModel, $newModel)); + $listener->handlePostPaste($this->buildPasteEvent($newModel)); + // Second post-paste for the same model must not copy again (setTranslatedDataFor still expected exactly once). + $listener->handlePostPaste($this->buildPasteEvent($newModel)); + } } From 080a5a0c3324d38f9b5f105713060739966f64cd Mon Sep 17 00:00:00 2001 From: Ingolf Steinhardt Date: Fri, 19 Jun 2026 14:22:56 +0200 Subject: [PATCH 2/4] Fix copy un-translated attributes with manual sorting --- src/Item.php | 22 +++++++++++++++- tests/ItemTest.php | 66 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 87 insertions(+), 1 deletion(-) diff --git a/src/Item.php b/src/Item.php index d273b258b..9918857a0 100644 --- a/src/Item.php +++ b/src/Item.php @@ -28,6 +28,7 @@ use MetaModels\Attribute\IAttribute; use MetaModels\Attribute\IInternal; +use MetaModels\Attribute\ITranslated; use MetaModels\Events\ParseItemEvent; use MetaModels\Filter\IFilter; use MetaModels\Helper\EmptyTest; @@ -600,7 +601,26 @@ public function copy() unset($arrNewData['tstamp']); unset($arrNewData['vargroup']); - return new Item($this->getMetaModel(), $arrNewData, $this->dispatcher); + $metaModel = $this->getMetaModel(); + $objCopy = new Item($metaModel, $arrNewData, $this->dispatcher); + + // A copy is a brand-new item that has to be persisted from scratch. The constructor populates the data array + // directly without going through set(), so nothing would be marked dirty and saveItem() would skip every + // attribute (see MetaModel::shouldSkipAttributeUpdate()). Mark the carried-over non-translated attribute + // values as dirty so that they are actually written for the new item. + // + // Translated attributes are intentionally left untouched here: they are copied per language by the + // CopyTranslatedData listener. Marking them dirty would make saveItem() persist the active language using the + // value currently in the data array, which may be fallback-language data of the source item. + foreach (\array_keys($arrNewData) as $strColName) { + $attribute = $metaModel->getAttribute($strColName); + if (null === $attribute || $attribute instanceof ITranslated) { + continue; + } + $objCopy->dirtyAttributes[$strColName] = true; + } + + return $objCopy; } /** diff --git a/tests/ItemTest.php b/tests/ItemTest.php index b66770d18..8560dde54 100644 --- a/tests/ItemTest.php +++ b/tests/ItemTest.php @@ -21,6 +21,8 @@ namespace MetaModels\Test; +use MetaModels\Attribute\IAttribute; +use MetaModels\Attribute\ITranslated; use MetaModels\IDirtyTracking; use MetaModels\IItem; use MetaModels\IMetaModel; @@ -110,4 +112,68 @@ public function testSetPreservesValue(): void self::assertSame('World', $item->get('title')); } + + /** + * A copied item is a brand-new record: carried-over non-translated attribute values must be marked dirty so that + * saveItem() actually persists them (it skips non-dirty attributes). Otherwise non-translated attributes would not + * be copied. + */ + public function testCopyMarksNonTranslatedAttributesAsDirty(): void + { + $title = $this->createMock(IAttribute::class); + $alias = $this->createMock(IAttribute::class); + + $metaModel = $this->createMock(IMetaModel::class); + $metaModel->method('getAttribute')->willReturnMap([['title', $title], ['alias', $alias]]); + + $item = new Item($metaModel, ['id' => '5', 'tstamp' => '123', 'title' => 'Hello', 'alias' => 'hello'], $this->createMock(EventDispatcherInterface::class)); + self::assertFalse($item->isDirty('title')); + + $copy = $item->copy(); + + self::assertTrue($copy->isDirty('title')); + self::assertTrue($copy->isDirty('alias')); + self::assertSame('Hello', $copy->get('title')); + self::assertSame('hello', $copy->get('alias')); + } + + /** + * Translated attributes are copied per language by the CopyTranslatedData listener, so copy() must not mark them + * dirty — that would let saveItem() write the active language using possibly fallback data of the source item. + */ + public function testCopyDoesNotMarkTranslatedAttributesDirty(): void + { + $translated = $this->createMock(ITranslated::class); + + $metaModel = $this->createMock(IMetaModel::class); + $metaModel->method('getAttribute')->willReturnMap([['title', $translated]]); + + $item = new Item($metaModel, ['id' => '5', 'title' => 'Hallo'], $this->createMock(EventDispatcherInterface::class)); + + $copy = $item->copy(); + + self::assertFalse($copy->isDirty('title')); + self::assertSame('Hallo', $copy->get('title')); + } + + /** + * The copy must not carry over the identity columns of its source. + */ + public function testCopyDropsIdentityColumns(): void + { + $title = $this->createMock(IAttribute::class); + + $metaModel = $this->createMock(IMetaModel::class); + $metaModel->method('getAttribute')->willReturnMap([['title', $title]]); + + $item = new Item($metaModel, ['id' => '5', 'tstamp' => '123', 'vargroup' => '2', 'title' => 'Hello'], $this->createMock(EventDispatcherInterface::class)); + + $copy = $item->copy(); + + self::assertNull($copy->get('id')); + self::assertNull($copy->get('tstamp')); + self::assertNull($copy->get('vargroup')); + self::assertFalse($copy->isDirty('id')); + self::assertFalse($copy->isDirty('tstamp')); + } } From 44a039061a06deb15654c106b8728b0dc74173dd Mon Sep 17 00:00:00 2001 From: Ingolf Steinhardt Date: Fri, 19 Jun 2026 15:10:50 +0200 Subject: [PATCH 3/4] Fix copy un-translated attributes with manual sorting --- .../ShouldSkipAttributeUpdateTest.php | 165 ++++++++++++++++++ 1 file changed, 165 insertions(+) create mode 100644 tests/MetaModel/ShouldSkipAttributeUpdateTest.php diff --git a/tests/MetaModel/ShouldSkipAttributeUpdateTest.php b/tests/MetaModel/ShouldSkipAttributeUpdateTest.php new file mode 100644 index 000000000..823344c8b --- /dev/null +++ b/tests/MetaModel/ShouldSkipAttributeUpdateTest.php @@ -0,0 +1,165 @@ + + * @copyright 2012-2026 The MetaModels team. + * @license https://github.com/MetaModels/core/blob/master/LICENSE LGPL-3.0-or-later + * @filesource + */ + +namespace MetaModels\Test\MetaModel; + +use Doctrine\DBAL\Connection; +use MetaModels\Attribute\IAttribute; +use MetaModels\IItem; +use MetaModels\Item; +use MetaModels\MetaModel; +use PHPUnit\Framework\MockObject\MockObject; +use PHPUnit\Framework\TestCase; +use Symfony\Component\EventDispatcher\EventDispatcher; +use Symfony\Component\EventDispatcher\EventDispatcherInterface; + +/** + * @covers \MetaModels\MetaModel::shouldSkipAttributeUpdate + */ +class ShouldSkipAttributeUpdateTest extends TestCase +{ + /** + * Attribute is not set on the item → skip. + */ + public function testSkipsWhenAttributeNotSet(): void + { + $reflection = new \ReflectionMethod(MetaModel::class, 'shouldSkipAttributeUpdate'); + + /** @var IItem&MockObject $item */ + $item = $this->createMock(IItem::class); + $item->method('isAttributeSet')->willReturn(false); + + /** @var IAttribute&MockObject $attribute */ + $attribute = $this->createMock(IAttribute::class); + $attribute->method('getColName')->willReturn('title'); + + self::assertTrue($reflection->invoke($this->createMetaModel(), $item, $attribute, false)); + } + + /** + * IDirtyTracking item with attribute not dirty → skip. + */ + public function testSkipsWhenAttributeNotDirty(): void + { + $metaModel = $this->createMetaModel(); + + // Constructor data is never dirty — isAttributeSet=true, isDirty=false. + $item = new Item($metaModel, ['title' => 'text'], new EventDispatcher()); + + /** @var IAttribute&MockObject $attribute */ + $attribute = $this->createMock(IAttribute::class); + $attribute->method('getColName')->willReturn('title'); + + $reflection = new \ReflectionMethod(MetaModel::class, 'shouldSkipAttributeUpdate'); + self::assertTrue($reflection->invoke($metaModel, $item, $attribute, false)); + } + + /** + * IDirtyTracking item with dirty attribute, non-variant item → don't skip. + */ + public function testDoesNotSkipWhenDirtyAndNonVariant(): void + { + $metaModel = $this->createMetaModel(); + + // set() marks the attribute dirty; MetaModel without variants → isVariant()=false. + $item = new Item($metaModel, [], new EventDispatcher()); + $item->set('title', 'text'); + + /** @var IAttribute&MockObject $attribute */ + $attribute = $this->createMock(IAttribute::class); + $attribute->method('getColName')->willReturn('title'); + $attribute->method('get')->with('isvariant')->willReturn(false); + + $reflection = new \ReflectionMethod(MetaModel::class, 'shouldSkipAttributeUpdate'); + self::assertFalse($reflection->invoke($metaModel, $item, $attribute, false)); + } + + /** + * Non-IDirtyTracking item (plain IItem), attribute set → dirty tracking is not applied. + */ + public function testDoesNotSkipForPlainIItemWithAttributeSet(): void + { + $reflection = new \ReflectionMethod(MetaModel::class, 'shouldSkipAttributeUpdate'); + + /** @var IItem&MockObject $item */ + $item = $this->createMock(IItem::class); + $item->method('isAttributeSet')->willReturn(true); + $item->method('isVariant')->willReturn(false); + + /** @var IAttribute&MockObject $attribute */ + $attribute = $this->createMock(IAttribute::class); + $attribute->method('getColName')->willReturn('title'); + $attribute->method('get')->with('isvariant')->willReturn(false); + + self::assertFalse($reflection->invoke($this->createMetaModel(), $item, $attribute, false)); + } + + /** + * Variant item, non-variant attribute, baseAttributes=false → skip base attribute. + */ + public function testSkipsBaseAttributeForVariantItem(): void + { + $reflection = new \ReflectionMethod(MetaModel::class, 'shouldSkipAttributeUpdate'); + + /** @var IItem&MockObject $item */ + $item = $this->createMock(IItem::class); + $item->method('isAttributeSet')->willReturn(true); + $item->method('isVariant')->willReturn(true); + + /** @var IAttribute&MockObject $attribute */ + $attribute = $this->createMock(IAttribute::class); + $attribute->method('getColName')->willReturn('title'); + $attribute->method('get')->with('isvariant')->willReturn(false); + + self::assertTrue($reflection->invoke($this->createMetaModel(), $item, $attribute, false)); + } + + /** + * Variant item, non-variant attribute, baseAttributes=true → don't skip. + */ + public function testDoesNotSkipBaseAttributeWhenBaseAttributesEnabled(): void + { + $reflection = new \ReflectionMethod(MetaModel::class, 'shouldSkipAttributeUpdate'); + + /** @var IItem&MockObject $item */ + $item = $this->createMock(IItem::class); + $item->method('isAttributeSet')->willReturn(true); + $item->method('isVariant')->willReturn(true); + + /** @var IAttribute&MockObject $attribute */ + $attribute = $this->createMock(IAttribute::class); + $attribute->method('getColName')->willReturn('title'); + $attribute->method('get')->with('isvariant')->willReturn(false); + + self::assertFalse($reflection->invoke($this->createMetaModel(), $item, $attribute, true)); + } + + private function createMetaModel(): MetaModel + { + /** @var Connection&MockObject $connection */ + $connection = $this->getMockBuilder(Connection::class)->disableOriginalConstructor()->getMock(); + $connection->expects(self::never())->method('createQueryBuilder'); + + return new MetaModel( + [], + $this->getMockForAbstractClass(EventDispatcherInterface::class), + $connection + ); + } +} From b6753e16efc0b7faf120f051039183b445a79f82 Mon Sep 17 00:00:00 2001 From: Ingolf Steinhardt Date: Fri, 19 Jun 2026 15:18:49 +0200 Subject: [PATCH 4/4] Fix psalm --- tests/ItemTest.php | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/tests/ItemTest.php b/tests/ItemTest.php index 8560dde54..ea0632e5c 100644 --- a/tests/ItemTest.php +++ b/tests/ItemTest.php @@ -38,6 +38,8 @@ * This prevents fallback-language data from being written to the active language on save. * * @covers \MetaModels\Item + * + * @SuppressWarnings(PHPMD.TooManyPublicMethods) */ class ItemTest extends TestCase { @@ -126,7 +128,11 @@ public function testCopyMarksNonTranslatedAttributesAsDirty(): void $metaModel = $this->createMock(IMetaModel::class); $metaModel->method('getAttribute')->willReturnMap([['title', $title], ['alias', $alias]]); - $item = new Item($metaModel, ['id' => '5', 'tstamp' => '123', 'title' => 'Hello', 'alias' => 'hello'], $this->createMock(EventDispatcherInterface::class)); + $item = new Item( + $metaModel, + ['id' => '5', 'tstamp' => '123', 'title' => 'Hello', 'alias' => 'hello'], + $this->createMock(EventDispatcherInterface::class) + ); self::assertFalse($item->isDirty('title')); $copy = $item->copy(); @@ -148,7 +154,11 @@ public function testCopyDoesNotMarkTranslatedAttributesDirty(): void $metaModel = $this->createMock(IMetaModel::class); $metaModel->method('getAttribute')->willReturnMap([['title', $translated]]); - $item = new Item($metaModel, ['id' => '5', 'title' => 'Hallo'], $this->createMock(EventDispatcherInterface::class)); + $item = new Item( + $metaModel, + ['id' => '5', 'title' => 'Hallo'], + $this->createMock(EventDispatcherInterface::class) + ); $copy = $item->copy(); @@ -166,7 +176,11 @@ public function testCopyDropsIdentityColumns(): void $metaModel = $this->createMock(IMetaModel::class); $metaModel->method('getAttribute')->willReturnMap([['title', $title]]); - $item = new Item($metaModel, ['id' => '5', 'tstamp' => '123', 'vargroup' => '2', 'title' => 'Hello'], $this->createMock(EventDispatcherInterface::class)); + $item = new Item( + $metaModel, + ['id' => '5', 'tstamp' => '123', 'vargroup' => '2', 'title' => 'Hello'], + $this->createMock(EventDispatcherInterface::class) + ); $copy = $item->copy();