Skip to content

Commit

Permalink
Merge pull request #304 from doctrine/fix-translations
Browse files Browse the repository at this point in the history 
fix various bugs around translation fallback
  • Loading branch information
@dbu
dbu committed Aug 4, 2013
2 parents 8fc6074 + dcd021f commit 0d1999a
Show file tree
Hide file tree
Showing 11 changed files with 187 additions and 34 deletions.
2 changes: 1 addition & 1 deletion lib/Doctrine/ODM/PHPCR/Mapping/ClassMetadata.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}

Expand Down
32 changes: 28 additions & 4 deletions lib/Doctrine/ODM/PHPCR/Translation/TranslationStrategy/AttributeTranslationStrategy.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,9 +73,35 @@ public function loadTranslation($document, NodeInterface $node, ClassMetadata $m
if ($node->hasProperty($this->prefix . ':' . $locale . self::NULLFIELDS)) {
$nullFields = $node->getPropertyValue($this->prefix . ':' . $locale . self::NULLFIELDS);
$nullFields = array_flip($nullFields);
// we have stored the document in this language at some point, even
// if all fields might be null.
$anyTranslatedField = true;
} else {
$nullFields = array();
$anyTranslatedField = false;
}
// 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) {
// we found neither the null list nor any field defined, this node
// was never stored in this locale. lets try the next locale.
return false;
}

// we have a translation, now update the document fields
foreach ($metadata->translatableFields as $field) {
$propName = $this->getTranslatedPropertyName($locale, $field);
if (isset($nullFields[$field])) {
Expand All @@ -91,10 +117,8 @@ public function loadTranslation($document, NodeInterface $node, ClassMetadata $m
}
} else {
// Could not find the translation in the given language
if (!$metadata->mappings[$field]['nullable']) {
return false;
}
$value = (true === $metadata->mappings[$field]['multivalue']) ? array() : null;
// we already returned above if this was a required field
$value = ($metadata->mappings[$field]['multivalue']) ? array() : null;
}
$metadata->reflFields[$field]->setValue($document, $value);
}
Expand Down
22 changes: 20 additions & 2 deletions lib/Doctrine/ODM/PHPCR/Translation/TranslationStrategy/ChildTranslationStrategy.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

return parent::loadTranslation($document, $translationNode, $metadata, $locale);
}
Expand Down Expand Up @@ -88,10 +91,25 @@ public function getLocalesFor($document, NodeInterface $node, ClassMetadata $met
return $locales;
}

protected function getTranslationNode(NodeInterface $parentNode, $locale)
/**
* Get the child node with the translation. If create is true, the child
* node is created if not existing.
*
* @param NodeInterface $parentNode
* @param string $locale
* @param boolean $create whether to create the node if it is
* not yet existing
*
* @return boolean|NodeInterface the node or false if $create is false and
* the node is not existing.
*/
protected function getTranslationNode(NodeInterface $parentNode, $locale, $create = true)
{
$name = Translation::LOCALE_NAMESPACE . ":$locale";
if (!$parentNode->hasNode($name)) {
if (!$create) {
return false;
}
$node = $parentNode->addNode($name);
} else {
$node = $parentNode->getNode($name);
Expand Down
28 changes: 24 additions & 4 deletions lib/Doctrine/ODM/PHPCR/UnitOfWork.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2735,11 +2735,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']) {
$allNullable = false;
break;
}
$value = ($metadata->mappings[$fieldName]['multivalue']) ? array() : null;
$metadata->reflFields[$fieldName]->setValue($document, $value);
}
// there was no translation, treat this as the requested locale was found.
$localeUsed = $locale;
if (!$allNullable) {
$msg = sprintf("No translation for locale '%s' at '%s' found with strategy '%s'.",
$locale,
$node->getPath(),
$metadata->translator
);
if (!empty($localesToTry)) {
$msg .= " Tried the following additional locales: ".var_export($localesToTry, true);
}

throw new MissingTranslationException($msg);
}
throw new MissingTranslationException($msg);
}
}

Expand Down
2 changes: 1 addition & 1 deletion tests/Doctrine/Tests/Models/Translation/Article.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ class Article
public $nullable;

/**
* @PHPCRODM\String(assoc="", translated=true)
* @PHPCRODM\String(assoc="", translated=true, nullable=true)
*/
protected $settings;

Expand Down
2 changes: 1 addition & 1 deletion tests/Doctrine/Tests/Models/Translation/Comment.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ class Comment
/** @PHPCRODM\ParentDocument */
public $parent;

/** @PHPCRODM\String(translated=true) */
/** @PHPCRODM\String(translated=true,nullable=true) */
private $text;

/**
Expand Down
38 changes: 31 additions & 7 deletions tests/Doctrine/Tests/ODM/PHPCR/Functional/Translation/AttributeTranslationStrategyTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,9 @@

namespace Doctrine\Tests\ODM\PHPCR\Functional\Translation;

use Doctrine\ODM\PHPCR\Mapping\ClassMetadata,
Doctrine\ODM\PHPCR\Translation\Translation,
Doctrine\ODM\PHPCR\Translation\TranslationStrategy\AttributeTranslationStrategy;
use Doctrine\ODM\PHPCR\Mapping\ClassMetadata;
use Doctrine\ODM\PHPCR\Translation\Translation;
use Doctrine\ODM\PHPCR\Translation\TranslationStrategy\AttributeTranslationStrategy;

use Doctrine\Tests\Models\Translation\Article;

Expand Down Expand Up @@ -100,17 +100,34 @@ public function testLoadTranslation()
$doc = new Article();

$strategy = new AttributeTranslationStrategy();
$strategy->loadTranslation($doc, $node, $this->metadata, 'en');
$this->assertTrue($strategy->loadTranslation($doc, $node, $this->metadata, 'en'));

// And check the translatable properties have the correct value
$this->assertEquals('English topic', $doc->topic);
$this->assertEquals('English text', $doc->getText());
$this->assertEquals(array(), $doc->getSettings()); // nullable

// Load another language and test the document has been updated
$strategy->loadTranslation($doc, $node, $this->metadata, 'fr');

$this->assertEquals('Sujet français', $doc->topic);
$this->assertEquals('Texte français', $doc->getText());
$this->assertEquals(array(), $doc->getSettings());
}

public function testLoadTranslationNotNullable()
{
// Create the node in the content repository
$node = $this->getTestNode();
$node->setProperty('author', 'John Doe');

$this->session->save();

// Then try to read it's translation
$doc = new Article();

$strategy = new AttributeTranslationStrategy();
$this->assertFalse($strategy->loadTranslation($doc, $node, $this->metadata, 'en'));
}

/**
Expand All @@ -125,16 +142,19 @@ public function testTranslationNullProperties()
$data['author'] = 'John Doe';
$data['topic'] = 'Some interesting subject';
$data['text'] = 'Lorem ipsum...';
$data['nullable'] = 'not null';
$data['settings'] = array('key' => 'value');

$node = $this->getTestNode();

$strategy = new AttributeTranslationStrategy();
$strategy->saveTranslation($data, $node, $this->metadata, 'en');

// Save translation in another language

$data = array();
$data['author'] = 'John Doe';
$data['topic'] = 'Un sujet intéressant';
$data['text'] = null;
$data['text'] = 'Lorem français';

$strategy->saveTranslation($data, $node, $this->metadata, 'fr');
$this->dm->flush();
Expand All @@ -146,10 +166,14 @@ public function testTranslationNullProperties()
$strategy->loadTranslation($doc, $node, $this->metadata, 'en');
$this->assertEquals('Some interesting subject', $doc->topic);
$this->assertEquals('Lorem ipsum...', $doc->getText());
$this->assertEquals('not null', $doc->nullable);
$this->assertEquals(array('key' => 'value'), $doc->getSettings());

$strategy->loadTranslation($doc, $node, $this->metadata, 'fr');
$this->assertEquals('Un sujet intéressant', $doc->topic);
$this->assertNull($doc->getText());
$this->assertEquals('Lorem français', $doc->getText());
$this->assertNull($doc->nullable);
$this->assertEquals(array(), $doc->getSettings());
}

public function testRemoveTranslation()
Expand Down
13 changes: 10 additions & 3 deletions tests/Doctrine/Tests/ODM/PHPCR/Functional/Translation/ChildTranslationStrategyTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -89,16 +89,19 @@ public function testLoadTranslation()
$data['author'] = 'John Doe';
$data['topic'] = 'English topic';
$data['text'] = 'English text';
$data['nullable'] = 'not null';
$data['settings'] = array('key' => 'value');

$node = $this->getTestNode();

$strategy = new ChildTranslationStrategy();
$strategy->saveTranslation($data, $node, $this->metadata, 'en');

// Save translation in another language

$data = array();
$data['author'] = 'John Doe';
$data['topic'] = 'Sujet français';
$data['text'] = null;
$data['text'] = 'Texte français';

$strategy->saveTranslation($data, $node, $this->metadata, 'fr');
$this->dm->flush();
Expand All @@ -112,12 +115,16 @@ public function testLoadTranslation()
// And check the translatable properties have the correct value
$this->assertEquals('English topic', $doc->topic);
$this->assertEquals('English text', $doc->getText());
$this->assertEquals('not null', $doc->nullable);
$this->assertEquals(array('key' => 'value'), $doc->getSettings());

// Load another language and test the document has been updated
$strategy->loadTranslation($doc, $node, $this->metadata, 'fr');

$this->assertEquals('Sujet français', $doc->topic);
$this->assertNull($doc->getText());
$this->assertEquals('Texte français', $doc->text);
$this->assertNull($doc->nullable);
$this->assertEquals(array(), $doc->getSettings());
}

public function testTranslationNullProperties()
Expand Down
74 changes: 66 additions & 8 deletions tests/Doctrine/Tests/ODM/PHPCR/Functional/Translation/DocumentManagerTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,16 @@

namespace Doctrine\Tests\ODM\PHPCR\Functional\Translation;

use Doctrine\Tests\Models\Translation\Article,
Doctrine\Tests\Models\Translation\Comment,
Doctrine\Tests\Models\Translation\InvalidMapping,
Doctrine\Tests\Models\Translation\DerivedArticle,
Doctrine\Tests\Models\CMS\CmsArticle,
Doctrine\Tests\ODM\PHPCR\PHPCRFunctionalTestCase;
use Doctrine\Tests\Models\Translation\Article;
use Doctrine\Tests\Models\Translation\Comment;
use Doctrine\Tests\Models\Translation\InvalidMapping;
use Doctrine\Tests\Models\Translation\DerivedArticle;
use Doctrine\Tests\Models\CMS\CmsArticle;
use Doctrine\Tests\ODM\PHPCR\PHPCRFunctionalTestCase;
use Doctrine\ODM\PHPCR\Mapping\ClassMetadata;

use Doctrine\ODM\PHPCR\Translation\TranslationStrategy\AttributeTranslationStrategy,
Doctrine\ODM\PHPCR\Translation\LocaleChooser\LocaleChooser;
use Doctrine\ODM\PHPCR\Translation\TranslationStrategy\AttributeTranslationStrategy;
use Doctrine\ODM\PHPCR\Translation\LocaleChooser\LocaleChooser;


class DocumentManagerTest extends PHPCRFunctionalTestCase
Expand Down Expand Up @@ -378,6 +378,47 @@ public function testFindByUUID()
$this->assertInstanceOf($this->class, $this->document);
}

/**
* Italian translation does not exist so as defined in $this->localePrefs we
* will get french as it has higher priority than english
*/
public function testFindWithLanguageFallback()
{
$this->dm->persist($this->doc);
$this->doc->topic = 'Un autre sujet';
$this->doc->text = 'Text';
$this->doc->locale = 'fr';
$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();
$doc->id = '/functional/fallback-nullable';
$doc->setText('Un commentaire');
$doc->locale = 'fr';
$this->dm->persist($doc);
$this->dm->flush();
$this->dm->clear();

$this->dm->getLocaleChooserStrategy()->setLocale('it');
$doc = $this->dm->find(null, '/functional/fallback-nullable');

$this->assertNotNull($doc);
$this->assertEquals('fr', $doc->locale);
$this->assertEquals('Un commentaire', $doc->getText());
}

/**
* Italian translation does not exist so as defined in $this->localePrefs we
* will get french as it has higher priority than english
Expand All @@ -396,6 +437,23 @@ public function testFindTranslationWithLanguageFallback()
$this->assertEquals('Un autre sujet', $this->doc->topic);
}

/**
* Test what happens if all document fields are nullable and actually null.
*/
public function testTranslationOnlyNullProperties()
{
$path = $this->node->getPath() . '/only-null';
$doc = new Comment();
$doc->id = $path;
$this->dm->persist($doc);
$this->dm->flush();
$this->dm->clear();

$doc = $this->dm->find(null, $path);
$this->assertInstanceOf('Doctrine\Tests\Models\Translation\Comment', $doc);
$this->assertNull($doc->getText());
}

/**
* @expectedException \InvalidArgumentException
*/
Expand Down
6 changes: 4 additions & 2 deletions tests/Doctrine/Tests/ODM/PHPCR/Functional/Translation/TranslationHierarchyTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,11 @@ public function setUp()
$user = $this->node->addNode('thename');
$user->setProperty('phpcr:class', $this->type, \PHPCR\PropertyType::STRING);
$user->setProperty('phpcr_locale:fr-topic', 'french', \PHPCR\PropertyType::STRING);
$user->setProperty('phpcr_locale:frnullfields', array('text', 'settings'), \PHPCR\PropertyType::STRING);
$user->setProperty('phpcr_locale:fr-text', 'french text', \PHPCR\PropertyType::STRING);
$user->setProperty('phpcr_locale:frnullfields', array('nullable'), \PHPCR\PropertyType::STRING);
$user->setProperty('phpcr_locale:en-topic', 'english', \PHPCR\PropertyType::STRING);
$user->setProperty('phpcr_locale:ennullfields', array('text', 'settings'), \PHPCR\PropertyType::STRING);
$user->setProperty('phpcr_locale:en-text', 'english text', \PHPCR\PropertyType::STRING);
$user->setProperty('phpcr_locale:ennullfields', array('nullable'), \PHPCR\PropertyType::STRING);
$this->dm->getPhpcrSession()->save();
}

Expand Down
2 changes: 1 addition & 1 deletion tests/Doctrine/Tests/ODM/PHPCR/Functional/Translation/TranslationTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ public function testLoadAnnotations()

/**
* Test loading of a translatable document missing the @Locale annotation.
* @expectedException Doctrine\ODM\PHPCR\Mapping\MappingException
* @expectedException \Doctrine\ODM\PHPCR\Mapping\MappingException
*/
public function testLoadMissingLocaleAnnotation()
{
Expand Down

0 comments on commit 0d1999a

Please sign in to comment.