fix various bugs around translation fallback #304

Merged
merged 1 commit into from Aug 4, 2013

Conversation

Projects
None yet
5 participants
Member

dbu commented Aug 2, 2013

Q A
Bug fix? yes
New feature? no
BC breaks? in edge cases
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR only bugfix

@dbu dbu commented on the diff Aug 2, 2013

lib/Doctrine/ODM/PHPCR/Mapping/ClassMetadata.php
@@ -1233,7 +1233,7 @@ public function mapField(array $mapping, ClassMetadata $inherited = null)
}
if (!isset($mapping['nullable'])) {
- $mapping['nullable'] = isset($parentMapping['nullable']) ? : false;
+ $mapping['nullable'] = isset($parentMapping['nullable']) ? $parentMapping['nullable'] : false;
@dbu

dbu Aug 2, 2013

Member

this was the reason why symfony-cmf/MenuBundle#115 helped in the sandbox. the return value of isset is true, regardless of what the field in $parentMapping is...

@dbu dbu commented on the diff Aug 2, 2013

...tion/TranslationStrategy/ChildTranslationStrategy.php
@@ -47,7 +47,10 @@ public function saveTranslation(array $data, NodeInterface $node, ClassMetadata
*/
public function loadTranslation($document, NodeInterface $node, ClassMetadata $metadata, $locale)
{
- $translationNode = $this->getTranslationNode($node, $locale);
+ $translationNode = $this->getTranslationNode($node, $locale, false);
+ if (!$translationNode) {
+ return false;
+ }
@dbu

dbu Aug 2, 2013

Member

we where creating an empty node. if we flush later, that node would get persisted.
rather not creat it on reading and return early.

@dbu dbu commented on the diff Aug 2, 2013

lib/Doctrine/ODM/PHPCR/UnitOfWork.php
@@ -2733,11 +2733,25 @@ public function doLoadTranslation($document, ClassMetadata $metadata, $locale =
}
if (empty($localeUsed)) {
- $msg = "No translation for locale '$locale' at '{$node->getPath()}' found with strategy '{$metadata->translator}.";
- if (!empty($localesToTry)) {
- $msg.= " Tried the following additional locales: ".var_export($localesToTry, true);
+ // we found no translation. if all translated fields can be
+ // null, we want to just null them.
@dbu

dbu Aug 2, 2013

Member

this is the job of unit of work, not of the translation strategy.

@dbu dbu commented on the diff Aug 2, 2013

lib/Doctrine/ODM/PHPCR/UnitOfWork.php
@@ -2733,11 +2733,25 @@ public function doLoadTranslation($document, ClassMetadata $metadata, $locale =
}
if (empty($localeUsed)) {
- $msg = "No translation for locale '$locale' at '{$node->getPath()}' found with strategy '{$metadata->translator}.";
- if (!empty($localesToTry)) {
- $msg.= " Tried the following additional locales: ".var_export($localesToTry, true);
+ // we found no translation. if all translated fields can be
+ // null, we want to just null them.
+ $allNullable = true;
+ foreach ($metadata->translatableFields as $fieldName) {
+ if (!$metadata->translatableFields[$fieldName]['nullable']) {
+ $allNullable = false;
+ break;
@dbu

dbu Aug 2, 2013

Member

or should we continue setting fields to null anyways? after the exception, the document is in a messed up state either way.

@wjzijderveld

wjzijderveld Aug 2, 2013

Contributor

Doesn't the exception causes the document to be invalid anyhow?
If not, we can refactor this so we only set the values when no exception is thrown.

@dbu

dbu Aug 3, 2013

Member

after an exception in the odm, documents and document manager state tend to be seriously messed up. i guess its not worth it to do something special here.

@wouterj wouterj commented on an outdated diff Aug 2, 2013

lib/Doctrine/ODM/PHPCR/UnitOfWork.php
- if (!empty($localesToTry)) {
- $msg.= " Tried the following additional locales: ".var_export($localesToTry, true);
+ // we found no translation. if all translated fields can be
+ // null, we want to just null them.
+ $allNullable = true;
+ foreach ($metadata->translatableFields as $fieldName) {
+ if (!$metadata->translatableFields[$fieldName]['nullable']) {
+ $allNullable = false;
+ break;
+ }
+ $value = ($metadata->mappings[$fieldName]['multivalue']) ? array() : null;
+ $metadata->reflFields[$fieldName]->setValue($document, $value);
+ }
+ $localeUsed = $locale;
+ if (!$allNullable) {
+ $msg = "No translation for locale '$locale' at '{$node->getPath()}' found with strategy '{$metadata->translator}'.";
@wouterj

wouterj Aug 2, 2013

Contributor

including variables in strings is not really the symfony-way, use sprintf for exception messages (it's also a standard)

@wouterj wouterj commented on an outdated diff Aug 2, 2013

lib/Doctrine/ODM/PHPCR/UnitOfWork.php
+ // we found no translation. if all translated fields can be
+ // null, we want to just null them.
+ $allNullable = true;
+ foreach ($metadata->translatableFields as $fieldName) {
+ if (!$metadata->translatableFields[$fieldName]['nullable']) {
+ $allNullable = false;
+ break;
+ }
+ $value = ($metadata->mappings[$fieldName]['multivalue']) ? array() : null;
+ $metadata->reflFields[$fieldName]->setValue($document, $value);
+ }
+ $localeUsed = $locale;
+ if (!$allNullable) {
+ $msg = "No translation for locale '$locale' at '{$node->getPath()}' found with strategy '{$metadata->translator}'.";
+ if (!empty($localesToTry)) {
+ $msg.= " Tried the following additional locales: ".var_export($localesToTry, true);
@wouterj

wouterj Aug 2, 2013

Contributor

missing space before .=

@wouterj wouterj commented on an outdated diff Aug 2, 2013

lib/Doctrine/ODM/PHPCR/UnitOfWork.php
+ $allNullable = true;
+ foreach ($metadata->translatableFields as $fieldName) {
+ if (!$metadata->translatableFields[$fieldName]['nullable']) {
+ $allNullable = false;
+ break;
+ }
+ $value = ($metadata->mappings[$fieldName]['multivalue']) ? array() : null;
+ $metadata->reflFields[$fieldName]->setValue($document, $value);
+ }
+ $localeUsed = $locale;
+ if (!$allNullable) {
+ $msg = "No translation for locale '$locale' at '{$node->getPath()}' found with strategy '{$metadata->translator}'.";
+ if (!empty($localesToTry)) {
+ $msg.= " Tried the following additional locales: ".var_export($localesToTry, true);
+ }
+ throw new MissingTranslationException($msg);
@wouterj

wouterj Aug 2, 2013

Contributor

an empty line before this one will make it more readable

@wouterj wouterj commented on an outdated diff Aug 2, 2013

.../PHPCR/Functional/Translation/DocumentManagerTest.php
+ $this->dm->flush();
+ $this->dm->getLocaleChooserStrategy()->setLocale('it');
+ $this->doc = $this->dm->find($this->class, '/functional/' . $this->testNodeName);
+
+ $this->assertNotNull($this->doc);
+ $this->assertEquals('fr', $this->doc->locale);
+ $this->assertEquals('Un autre sujet', $this->doc->topic);
+ $this->assertEquals('Text', $this->doc->text);
+ }
+
+ /**
+ * Same as findWithLanguageFallback, but all properties are nullable.
+ */
+ public function testFindWithLanguageFallbackNullable()
+ {
+ $doc = new Comment;
@wouterj

wouterj Aug 2, 2013

Contributor

new Comment()

@wouterj wouterj commented on an outdated diff Aug 2, 2013

.../TranslationStrategy/AttributeTranslationStrategy.php
}
+ // first check if we have any field translated in this language. if
+ // there is not a single one, this locale has no translation stored.
+ // if we encounter a non-nullable missing property we return
+ // immediately. do not update the document yet at all, to not mix
+ // languages.
+ foreach ($metadata->translatableFields as $field) {
+ $propName = $this->getTranslatedPropertyName($locale, $field);
+ if ($node->hasProperty($propName)) {
+ $anyTranslatedField = true;
+ } elseif (!$metadata->mappings[$field]['nullable']) {
+ // this field is required but missing
+ return false;
+ }
+ }
+ if (!$anyTranslatedField) {
@wouterj

wouterj Aug 2, 2013

Contributor

I prefer an empty line before this one

Contributor

wjzijderveld commented Aug 2, 2013

Other then some coding standards that WouterJ already commented on, this seems fine to me.

Contributor

wjzijderveld commented Aug 2, 2013

And the failing test caused by accessing private property should be fixed.

@dbu dbu added a commit that referenced this pull request Aug 4, 2013

@dbu dbu Merge pull request #304 from doctrine/fix-translations
fix various bugs around translation fallback
0d1999a

@dbu dbu merged commit 0d1999a into master Aug 4, 2013

1 check passed

default The Travis CI build passed
Details

dbu deleted the fix-translations branch Aug 4, 2013

@uwej711 uwej711 commented on the diff Aug 5, 2013

lib/Doctrine/ODM/PHPCR/UnitOfWork.php
@@ -2733,11 +2733,31 @@ public function doLoadTranslation($document, ClassMetadata $metadata, $locale =
}
if (empty($localeUsed)) {
- $msg = "No translation for locale '$locale' at '{$node->getPath()}' found with strategy '{$metadata->translator}.";
- if (!empty($localesToTry)) {
- $msg.= " Tried the following additional locales: ".var_export($localesToTry, true);
+ // we found no translation. if all translated fields can be
+ // null, we want to just null them.
+ $allNullable = true;
+ foreach ($metadata->translatableFields as $fieldName) {
+ if (!$metadata->translatableFields[$fieldName]['nullable']) {
@uwej711

uwej711 Aug 5, 2013

Contributor

This breaks my code, $metadata->translatableFields is just an array of strings,
see https://github.com/doctrine/phpcr-odm/blob/master/lib/Doctrine/ODM/PHPCR/Mapping/ClassMetadata.php#L1249, will try to provide a failing test

@dbu

dbu Aug 5, 2013

Member

argh. this should read !$metadata->mappings[$field]['nullable'] as in the attribute strategy above. no idea why i did not see that. maybe the tests in the end still do not cover all situations. would be great if you can add that test and then try if that fix is the correct one.

@dbu

dbu Aug 5, 2013

Member

ftr: uwe fixed this in #313

Contributor

dantleech commented Aug 11, 2013

@dbu: This seems to be creating a regression at least when using the attribute translation strategy. I get an error message saying that it can't find any translations, reverting to the commit before this merge fixes the pb...

Contributor

dantleech commented Aug 11, 2013

Created #317

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment