fix changeset calculation with translated documents #555
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -144,6 +144,11 @@ class UnitOfWork | |
| */ | ||
| private $originalData = array(); | ||
|
|
||
| /** | ||
| * @var array | ||
| */ | ||
| private $originalTranslatedData = array(); | ||
|
|
||
| /** | ||
| * @var array | ||
| */ | ||
|
|
@@ -712,7 +717,12 @@ public function bindTranslation($document, $locale) | |
| )); | ||
| } | ||
|
|
||
| $this->doBindTranslation($document, $locale, $class); | ||
| } | ||
|
|
||
| private function doBindTranslation($document, $locale, ClassMetadata $class, $supressEvents = false) | ||
| { | ||
| if (!$supressEvents && $invoke = $this->eventListenersInvoker->getSubscribedSystems($class, Event::preBindTranslation)) { | ||
| $this->eventListenersInvoker->invoke( | ||
| $class, | ||
| Event::preBindTranslation, | ||
|
|
@@ -726,15 +736,11 @@ public function bindTranslation($document, $locale) | |
|
|
||
| $oid = spl_object_hash($document); | ||
|
|
||
| foreach ($class->translatableFields as $field) { | ||
| $this->documentTranslations[$oid][$locale][$field] = $class->reflFields[$field]->getValue($document); | ||
| } | ||
|
|
||
| if (!$supressEvents && $invoke = $this->eventListenersInvoker->getSubscribedSystems($class, Event::postBindTranslation)) { | ||
| $this->eventListenersInvoker->invoke( | ||
| $class, | ||
| Event::postBindTranslation, | ||
|
|
@@ -1539,24 +1545,64 @@ public function computeChangeSet(ClassMetadata $class, $document) | |
|
|
||
| $fields = array_intersect_key($changeSet, $class->mappings); | ||
|
|
||
| if ($this->isDocumentTranslatable($class)) { | ||
| $locale = $this->getCurrentLocale($document, $class); | ||
| $oid = spl_object_hash($document); | ||
|
|
||
| // check if the document has not been bound or if the locale has not yet been bound | ||
| // (note array_key_exists is necessary to handle removed translations) | ||
| if (empty($this->documentTranslations[$oid]) | ||
| || !array_key_exists($locale, $this->documentTranslations[$oid]) | ||
| ) { | ||
| $this->doBindTranslation($document, $locale, $class); | ||
| } | ||
| } | ||
|
|
||
| if ($isNew) { | ||
| $this->documentChangesets[$oid]['fields'] = $fields; | ||
| $this->scheduledInserts[$oid] = $document; | ||
| return; | ||
| } | ||
|
|
||
| $translationChanges = false; | ||
| if ($this->isDocumentTranslatable($class)) { | ||
| $oid = spl_object_hash($document); | ||
| if (isset($this->documentTranslations[$oid])) { | ||
| foreach ($this->documentTranslations[$oid] as $localeToCheck => $data) { | ||
| // a translation was removed | ||
| if (empty($data)) { | ||
| $translationChanges = true; | ||
| break; | ||
| } | ||
| // a translation was added | ||
| if (empty($this->originalTranslatedData[$oid][$localeToCheck])) { | ||
| $translationChanges = true; | ||
| break; | ||
| } | ||
| // a translation was changed | ||
| foreach ($data as $fieldName => $fieldValue) { | ||
| if ($this->originalTranslatedData[$oid][$localeToCheck][$fieldName] !== $fieldValue) { | ||
| $translationChanges = true; | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // ensure that locale changes are not considered a change in the document | ||
| if ($class->localeMapping && array_key_exists($class->localeMapping, $fields)) { | ||
| unset($fields[$class->localeMapping]); | ||
| } | ||
| } | ||
|
|
||
| foreach ($fields as $fieldName => $fieldValue) { | ||
| $keepChange = false; | ||
| if ($fieldValue instanceof ReferenceManyCollection || $fieldValue instanceof ReferrersCollection) { | ||
| if ($fieldValue->changed()) { | ||
| $keepChange = true; | ||
| } | ||
| } elseif ($this->originalData[$oid][$fieldName] !== $fieldValue) { | ||
| $keepChange = true; | ||
| } | ||
|
|
||
| if ($keepChange) { | ||
|
|
@@ -1566,7 +1612,7 @@ public function computeChangeSet(ClassMetadata $class, $document) | |
| } | ||
| } | ||
|
|
||
| if (!empty($fields) || $translationChanges) { | ||
| $this->documentChangesets[$oid]['fields'] = $fields; | ||
| $this->originalData[$oid] = $actualData; | ||
| $this->scheduledUpdates[$oid] = $document; | ||
|
|
@@ -2967,6 +3013,7 @@ private function unregisterDocument($document) | |
| $this->scheduledReorders[$oid], | ||
| $this->scheduledInserts[$oid], | ||
| $this->originalData[$oid], | ||
| $this->originalTranslatedData[$oid], | ||
| $this->documentIds[$oid], | ||
| $this->documentState[$oid], | ||
| $this->documentTranslations[$oid], | ||
|
|
@@ -3103,6 +3150,7 @@ public function clear() | |
| $this->documentLocales = | ||
| $this->nonMappedData = | ||
| $this->originalData = | ||
| $this->originalTranslatedData = | ||
| $this->documentChangesets = | ||
| $this->changesetComputed = | ||
| $this->scheduledUpdates = | ||
|
|
@@ -3171,23 +3219,15 @@ private function doSaveTranslation($document, NodeInterface $node, $metadata) | |
| return; | ||
| } | ||
|
|
||
| $oid = spl_object_hash($document); | ||
| if (!empty($this->documentTranslations[$oid])) { | ||
| $strategy = $this->dm->getTranslationStrategy($metadata->translator); | ||
| foreach ($this->documentTranslations[$oid] as $locale => $data) { | ||
| if ($data) { | ||
| foreach ($data as $fieldName => $fieldValue) { | ||
| $this->originalTranslatedData[$oid][$locale][$fieldName] = $fieldValue; | ||
| } | ||
|
|
||
| $strategy->saveTranslation($data, $node, $metadata, $locale); | ||
| } else { | ||
| $strategy->removeTranslation($document, $node, $metadata, $locale); | ||
|
|
@@ -3330,6 +3370,7 @@ public function doLoadTranslation($document, ClassMetadata $metadata, $locale = | |
| } | ||
|
|
||
| $currentLocale = $this->getCurrentLocale($document, $metadata); | ||
|
|
||
| // if no locale is specified, we reset the current translation | ||
|
There was a problem hiding this comment. i think this is the wrong behaviour. we should simply do nothing if there is nothing to do. There was a problem hiding this comment. for resetting, there is DocumentManager::refresh There was a problem hiding this comment. fine for me too .. but imho this should be done in a new PR There was a problem hiding this comment. the scenario from #556 is fixed and a test is included for this even .. |
||
| $locale = $locale ?: $currentLocale; | ||
|
|
||
|
|
@@ -3339,7 +3380,13 @@ public function doLoadTranslation($document, ClassMetadata $metadata, $locale = | |
| $localeUsed = $this->doLoadDatabaseTranslation($document, $metadata, $locale, $fallback, $refresh); | ||
| } | ||
|
|
||
| $this->doBindTranslation($document, $localeUsed, $metadata, true); | ||
|
There was a problem hiding this comment. this There was a problem hiding this comment. so you are calling doBindTranslation here to store the original data of the translation that was loaded? or what for? |
||
|
|
||
| $oid = spl_object_hash($document); | ||
| foreach ($metadata->translatableFields as $fieldName) { | ||
| $this->originalData[$oid][$fieldName] = $this->originalTranslatedData[$oid][$localeUsed][$fieldName] | ||
| = $metadata->getFieldValue($document, $fieldName); | ||
| } | ||
|
|
||
| if ($metadata->parentMapping) { | ||
| $parent = $metadata->reflFields[$metadata->parentMapping]->getValue($document); | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,161 @@ | ||
| <?php | ||
|
|
||
| namespace Doctrine\Tests\ODM\PHPCR\Functional; | ||
|
|
||
| use Doctrine\Common\EventArgs; | ||
| use Doctrine\Common\Persistence\Event\LifecycleEventArgs; | ||
| use Doctrine\ODM\PHPCR\Event; | ||
| use Doctrine\ODM\PHPCR\Translation\LocaleChooser\LocaleChooser; | ||
|
|
||
| class ChangesetCalculationTest extends \Doctrine\Tests\ODM\PHPCR\PHPCRFunctionalTestCase | ||
| { | ||
| /** | ||
| * @var ChangesetListener | ||
| */ | ||
| private $listener; | ||
|
|
||
| /** | ||
| * @var \Doctrine\ODM\PHPCR\DocumentManager | ||
| */ | ||
| private $dm; | ||
|
|
||
| public function setUp() | ||
| { | ||
| $this->listener = new ChangesetListener(); | ||
| $this->dm = $this->createDocumentManager(); | ||
| $this->dm->setLocaleChooserStrategy(new LocaleChooser(array('en' => array('fr'), 'fr' => array('en')), 'en')); | ||
| $this->node = $this->resetFunctionalNode($this->dm); | ||
| } | ||
|
|
||
| public function testComputeChangeset() | ||
| { | ||
| $this->dm | ||
| ->getEventManager() | ||
| ->addEventListener( | ||
| array( | ||
| Event::postUpdate, | ||
| ), | ||
| $this->listener | ||
| ); | ||
|
|
||
| // Create initial user | ||
| $user1 = new \Doctrine\Tests\Models\CMS\CmsUser(); | ||
| $user1->name = 'david'; | ||
| $user1->username = 'dbu'; | ||
| $user1->status = 'active'; | ||
| $this->dm->persist($user1); | ||
|
|
||
| // Create additional user | ||
| $user2 = new \Doctrine\Tests\Models\CMS\CmsUser(); | ||
| $user2->name = 'lukas'; | ||
| $user2->username = 'lsmith'; | ||
| $user2->status = 'active'; | ||
| $this->dm->persist($user2); | ||
|
|
||
| $this->dm->flush(); | ||
| $this->assertEquals(0, $this->listener->count); | ||
| $this->dm->clear(); | ||
|
|
||
| $user1 = $this->dm->find(null, $user1->id); | ||
| $this->dm->find(null, $user2->id); | ||
|
|
||
| $user1->status = 'changed'; | ||
| $this->dm->flush(); | ||
|
|
||
| $this->assertEquals(1, $this->listener->count); | ||
| } | ||
|
|
||
| public function testComputeChangesetTranslatable() | ||
| { | ||
| $this->dm | ||
| ->getEventManager() | ||
| ->addEventListener( | ||
| array( | ||
| Event::postUpdate, | ||
| ), | ||
| $this->listener | ||
| ); | ||
|
|
||
| // Create initial user | ||
| $user1 = new \Doctrine\Tests\Models\CMS\CmsUserTranslatable(); | ||
| $user1->name = 'david'; | ||
| $user1->username = 'dbu'; | ||
| $user1->status = 'active'; | ||
| $this->dm->persist($user1); | ||
|
|
||
| // Create additional user | ||
| $user2 = new \Doctrine\Tests\Models\CMS\CmsUserTranslatable(); | ||
| $user2->name = 'lukas'; | ||
| $user2->username = 'lsmith'; | ||
| $user2->status = 'active'; | ||
| $this->dm->persist($user2); | ||
|
|
||
| $this->dm->flush(); | ||
| $this->assertEquals(0, $this->listener->count); | ||
| $this->dm->clear(); | ||
|
|
||
| $user1 = $this->dm->find(null, $user1->id); | ||
| $this->dm->find(null, $user2->id); | ||
|
|
||
| $user1->status = 'changed'; | ||
| $this->dm->flush(); | ||
|
|
||
| $this->assertEquals(1, $this->listener->count); | ||
| } | ||
|
|
||
| public function testComputeChangesetTranslatableFind() | ||
| { | ||
| $this->dm | ||
| ->getEventManager() | ||
| ->addEventListener( | ||
| array( | ||
| Event::postUpdate, | ||
| ), | ||
| $this->listener | ||
| ); | ||
|
|
||
| // Create initial user | ||
| $user1 = new \Doctrine\Tests\Models\CMS\CmsUserTranslatable(); | ||
| $user1->name = 'david'; | ||
| $user1->username = 'dbu'; | ||
| $user1->status = 'activ'; | ||
| $this->dm->persist($user1); | ||
| $this->dm->bindTranslation($user1, 'en'); | ||
| $user1->status = 'actif'; | ||
| $this->dm->bindTranslation($user1, 'fr'); | ||
|
|
||
| $this->dm->flush(); | ||
| $this->assertEquals(0, $this->listener->count); | ||
| $this->dm->clear(); | ||
|
|
||
| $user1 = $this->dm->findTranslation(null, $user1->id, 'en'); | ||
| $this->dm->findTranslation(null, $user1->id, 'fr'); | ||
|
|
||
| $this->dm->flush(); | ||
|
|
||
| $this->assertEquals(0, $this->listener->count); | ||
|
|
||
| $user1 = $this->dm->findTranslation(null, $user1->id, 'en'); | ||
| $user1->status = 'active'; | ||
| $this->dm->findTranslation(null, $user1->id, 'fr'); | ||
|
|
||
| $this->dm->flush(); | ||
|
|
||
| $this->assertEquals(1, $this->listener->count); | ||
| $this->dm->clear(); | ||
|
|
||
| $user1 = $this->dm->findTranslation(null, $user1->id, 'en'); | ||
| $this->assertEquals('active', $user1->status); | ||
|
|
||
| } | ||
| } | ||
|
|
||
| class ChangesetListener | ||
| { | ||
| public $count = 0; | ||
|
|
||
| public function postUpdate(LifecycleEventArgs $e) | ||
| { | ||
| $this->count++; | ||
| } | ||
| } |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so you opt for comparing translation fields with the original value only at this point. what of the idea to check in bindTranslation and not create an entry in documentTranslations if there is no change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
with this approach we also ensure that if event listeners mess with the data that we detect changes. i also do not see the advantage of doing it in
bindTranslation()aside from the fact that thecomputeChangeSet()method is rather large alreadyThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think we would have less copies of the translated content lying around when nothing changed. i don't see how event listeners could mess with the actual translated values in the array.
the only thing we really do need to compare at this time here are the fields of the currently loaded locale, which we do above when iterating over the fields as per metadata. i think right now we even compare the current locale twice, once as part of all fields of the document and once here. as the current locale is also in documentTranslations.
but anyway, i am fine to merge with this state. i can try to refactor afterwards unless you can convince me that this would somehow break event listeners from changing data.