Skip to content

Commit

Permalink
Merge pull request #244 from doctrine/fix-referrers-children-duplication
Browse files Browse the repository at this point in the history 
fix bug about createDocument and multilang
  • Loading branch information
@dbu
dbu committed Feb 23, 2013
2 parents 4cda01f + 1745d85 commit 7e310fe
Show file tree
Hide file tree
Showing 5 changed files with 137 additions and 54 deletions.
6 changes: 3 additions & 3 deletions lib/Doctrine/ODM/PHPCR/DocumentManager.php
View file
Expand Up @@ -308,7 +308,7 @@ public function find($className, $id)

$hints = array('fallback' => true);

return $this->unitOfWork->createDocument($className, $node, $hints);
return $this->unitOfWork->getOrCreateDocument($className, $node, $hints);
}

/**
Expand Down Expand Up @@ -353,7 +353,7 @@ public function findMany($className, array $ids)
// ignore on class mismatch
}
} elseif (isset($nodes[$id])) {
$documents[$id] = $this->unitOfWork->createDocument($className, $nodes[$id]);
$documents[$id] = $this->unitOfWork->getOrCreateDocument($className, $nodes[$id]);
}
}

Expand Down Expand Up @@ -402,7 +402,7 @@ public function findTranslation($className, $id, $locale, $fallback = true)

$hints = array('locale' => $locale, 'fallback' => $fallback);

return $this->unitOfWork->createDocument($className, $node, $hints);
return $this->unitOfWork->getOrCreateDocument($className, $node, $hints);
}

/**
Expand Down
4 changes: 2 additions & 2 deletions lib/Doctrine/ODM/PHPCR/ReferenceManyCollection.php
View file
Expand Up @@ -65,8 +65,8 @@ public function initialize()

foreach ($referencedNodes as $referencedNode) {
$proxy = $referencedClass
? $uow->createProxy($referencedNode->getPath(), $referencedClass)
: $uow->createProxyFromNode($referencedNode);
? $uow->getOrCreateProxy($referencedNode->getPath(), $referencedClass)
: $uow->getOrCreateProxyFromNode($referencedNode);
$referencedDocs[] = $proxy;
}

Expand Down
100 changes: 59 additions & 41 deletions lib/Doctrine/ODM/PHPCR/UnitOfWork.php
View file
Expand Up @@ -233,10 +233,11 @@ public function validateClassName($document, $className)
}

/**
* Create a document given class, data and the doc-id and revision
* Get the existing document or proxy of the specified class and node data
* or create a new one if not existing.
*
* Supported hints are
* - refresh: reload the fields from the database
* - refresh: reload the fields from the database if set
* - locale: use this locale instead of the one from the annotation or the default
* - fallback: whether to try other languages or throw a not found
* exception if the desired locale is not found. defaults to true if
Expand All @@ -248,15 +249,31 @@ public function validateClassName($document, $className)
*
* @return object
*/
public function createDocument($className, NodeInterface $node, array &$hints = array())
public function getOrCreateDocument($className, NodeInterface $node, array &$hints = array())
{
$requestedClassName = $className;
$className = $this->documentClassMapper->getClassName($this->dm, $node, $className);
$class = $this->dm->getClassMetadata($className);
$id = $node->getPath();

$document = $this->getDocumentById($id);

if ($document) {
if (empty($hints['refresh'])) {
// document already loaded and no need to refresh. return early

return $document;
}
$overrideLocalValuesOid = spl_object_hash($document);
} else {
$document = $class->newInstance();
// delay registering the new document until children proxy have been created
$overrideLocalValuesOid = false;
}
$this->validateClassName($document, $requestedClassName);

$documentState = array();
$nonMappedData = array();
$id = $node->getPath();

// second param is false to get uuid rather than dereference reference properties to node instances
$properties = $node->getPropertiesValues(null, false);
Expand Down Expand Up @@ -328,10 +345,10 @@ public function createDocument($className, NodeInterface $node, array &$hints =
$path = $referencedNode->getPath();
}

$proxy = $this->createProxy($path, $referencedClass);
$proxy = $this->getOrCreateProxy($path, $referencedClass);
} else {
$referencedNode = $node->getProperty($fieldName)->getNode();
$proxy = $this->createProxyFromNode($referencedNode);
$proxy = $this->getOrCreateProxyFromNode($referencedNode);
}
} catch (RepositoryException $e) {
if ($e instanceof ItemNotFoundException || isset($hints['ignoreHardReferenceNotFound'])) {
Expand Down Expand Up @@ -359,26 +376,16 @@ public function createDocument($className, NodeInterface $node, array &$hints =

if ($class->parentMapping && $node->getDepth() > 0) {
// do not map parent to self if we are at root
$documentState[$class->parentMapping] = $this->createProxyFromNode($node->getParent());
$documentState[$class->parentMapping] = $this->getOrCreateProxyFromNode($node->getParent());
}

foreach ($class->childMappings as $fieldName) {
$mapping = $class->mappings[$fieldName];
$documentState[$fieldName] = $node->hasNode($mapping['name'])
? $this->createProxyFromNode($node->getNode($mapping['name']))
? $this->getOrCreateProxyFromNode($node->getNode($mapping['name']))
: null;
}

$document = $this->getDocumentById($id);
if ($document) {
$overrideLocalValuesOid = empty($hints['refresh']) ? false : spl_object_hash($document);
} else {
$document = $class->newInstance();
$overrideLocalValuesOid = $this->registerDocument($document, $id);
}

$this->validateClassName($document, $requestedClassName);

foreach ($class->childrenMappings as $fieldName) {
$mapping = $class->mappings[$fieldName];
$documentState[$fieldName] = new ChildrenCollection($this->dm, $document, $mapping['filter'], $mapping['fetchDepth']);
Expand All @@ -389,13 +396,16 @@ public function createDocument($className, NodeInterface $node, array &$hints =
$documentState[$fieldName] = new ReferrersCollection($this->dm, $document, $mapping['referenceType'], $mapping['filter']);
}

if ($overrideLocalValuesOid) {
$this->nonMappedData[$overrideLocalValuesOid] = $nonMappedData;
foreach ($class->reflFields as $prop => $reflFields) {
$value = isset($documentState[$prop]) ? $documentState[$prop] : null;
$reflFields->setValue($document, $value);
$this->originalData[$overrideLocalValuesOid][$prop] = $value;
}
if (! $overrideLocalValuesOid) {
// registering the document needs to be delayed until the children proxies where created
$overrideLocalValuesOid = $this->registerDocument($document, $id);
}

$this->nonMappedData[$overrideLocalValuesOid] = $nonMappedData;
foreach ($class->reflFields as $prop => $reflFields) {
$value = isset($documentState[$prop]) ? $documentState[$prop] : null;
$reflFields->setValue($document, $value);
$this->originalData[$overrideLocalValuesOid][$prop] = $value;
}

// Load translations
Expand All @@ -415,23 +425,31 @@ public function createDocument($className, NodeInterface $node, array &$hints =
return $document;
}

public function createProxyFromNode(NodeInterface $node)
/**
* Get the existing document or proxy or create a new one for this PHPCR Node
*
* @param NodeInterface $node
*
* @return object
*/
public function getOrCreateProxyFromNode(NodeInterface $node)
{
$targetId = $node->getPath();
$className = $this->documentClassMapper->getClassName($this->dm, $node);

return $this->createProxy($targetId, $className);
return $this->getOrCreateProxy($targetId, $className);
}

/**
* Create a proxy instance or return an existing document
* Get the existing document or proxy for this id of this class, or create
* a new one.
*
* @param $targetId
* @param $className
* @param string $targetId
* @param string $className
*
* @return object
*/
public function createProxy($targetId, $className)
public function getOrCreateProxy($targetId, $className)
{
$document = $this->getDocumentById($targetId);

Expand All @@ -458,7 +476,7 @@ public function refreshDocumentForProxy($className, Proxy $document)
{
$node = $this->session->getNode($document->__getIdentifier());
$hints = array('refresh' => true);
$this->createDocument($className, $node, $hints);
$this->getOrCreateDocument($className, $node, $hints);
}

/**
Expand Down Expand Up @@ -1248,7 +1266,7 @@ private function doRefresh($document, &$visited)
$this->cascadeRefresh($class, $document, $visited);

$hints = array('refresh' => true);
$this->createDocument(get_class($document), $node, $hints);
$this->getOrCreateDocument(get_class($document), $node, $hints);
}

public function merge($document)
Expand All @@ -1268,7 +1286,7 @@ private function doMergeSingleDocumentProperty($managedCopy, $document, \Reflect
} else {
$targetClass = $this->dm->getClassMetadata(get_class($document));
$id = $targetClass->getIdentifierValues($document);
$proxy = $this->createProxy($id, $targetClass->name);
$proxy = $this->getOrCreateProxy($id, $targetClass->name);
$prop->setValue($managedCopy, $proxy);
$this->registerDocument($proxy, $id);
}
Expand Down Expand Up @@ -1710,7 +1728,7 @@ private function executeInserts($documents)
}
// make sure this reflects the id generator strategy generated id
if ($class->parentMapping && !$class->reflFields[$class->parentMapping]->getValue($document)) {
$class->reflFields[$class->parentMapping]->setValue($document, $this->createDocument(null, $parentNode));
$class->reflFields[$class->parentMapping]->setValue($document, $this->getOrCreateDocument(null, $parentNode));
}

if ($this->writeMetadata) {
Expand Down Expand Up @@ -1904,7 +1922,7 @@ private function executeUpdates($documents, $dispatchEvents = true)
if ($fieldValue === null) {
if ($node->hasNode($mapping['name'])) {
$child = $node->getNode($mapping['name']);
$childDocument = $this->createDocument(null, $child);
$childDocument = $this->getOrCreateDocument(null, $child);
$this->purgeChildren($childDocument);
$child->remove();
}
Expand Down Expand Up @@ -1961,7 +1979,7 @@ private function executeMoves($documents)
$class->setFieldValue($document, $class->nodename, $node->getName());
}
if ($class->parentMapping) {
$class->setFieldValue($document, $class->parentMapping, $this->createProxyFromNode($node->getParent()));
$class->setFieldValue($document, $class->parentMapping, $this->getOrCreateProxyFromNode($node->getParent()));
}

// update all cached children of the document to reflect the move (path id changes)
Expand Down Expand Up @@ -2097,7 +2115,7 @@ public function findVersionByName($className, $id, $versionName)
}

$hints = array('versionName' => $versionName, 'ignoreHardReferenceNotFound' => true);
$frozenDocument = $this->createDocument($className, $node, $hints);
$frozenDocument = $this->getOrCreateDocument($className, $node, $hints);
$this->dm->detach($frozenDocument);

$oid = spl_object_hash($frozenDocument);
Expand Down Expand Up @@ -2337,7 +2355,7 @@ public function getChildren($document, $filter = null, $fetchDepth = null, $igno
$childDocuments = array();
foreach ($childNodes as $name => $childNode) {
try {
$childDocuments[$name] = $this->createDocument(null, $childNode, $childrenHints);
$childDocuments[$name] = $this->getOrCreateDocument(null, $childNode, $childrenHints);
} catch (MissingTranslationException $e) {
if (!$ignoreUntranslated) {
throw $e;
Expand Down Expand Up @@ -2384,12 +2402,12 @@ public function getReferrers($document, $type = null, $name = null)

foreach ($referrerPropertiesW as $referrerProperty) {
$referrerNode = $referrerProperty->getParent();
$referrerDocuments[] = $this->createDocument(null, $referrerNode);
$referrerDocuments[] = $this->getOrCreateDocument(null, $referrerNode);
}

foreach ($referrerPropertiesH as $referrerProperty) {
$referrerNode = $referrerProperty->getParent();
$referrerDocuments[] = $this->createDocument(null, $referrerNode);
$referrerDocuments[] = $this->getOrCreateDocument(null, $referrerNode);
}

return new ArrayCollection($referrerDocuments);
Expand Down
56 changes: 56 additions & 0 deletions tests/Doctrine/Tests/ODM/PHPCR/Functional/ReferrerTest.php
View file
Expand Up @@ -24,6 +24,11 @@ class ReferrerTest extends \Doctrine\Tests\ODM\PHPCR\PHPCRFunctionalTestCase
*/
private $node;

protected $localePrefs = array(
'en' => array('en', 'fr'),
'fr' => array('fr', 'en'),
);

public function setUp()
{
$this->dm = $this->createDocumentManager();
Expand Down Expand Up @@ -565,6 +570,41 @@ public function testNamedRef()
$this->assertCount(1, $reference->referrers);
$this->assertEquals("referrerNamedPropTestObj", $reference->referrers[0]->name);
}

/**
* There was a bug that documents translated fields where overwritten when they are re-created
*
* depends testCreate
*/
public function testMultilangReferrers()
{
$this->dm->setLocaleChooserStrategy(new \Doctrine\ODM\PHPCR\Translation\LocaleChooser\LocaleChooser($this->localePrefs, 'en'));
$this->dm->setTranslationStrategy('attribute', new \Doctrine\ODM\PHPCR\Translation\TranslationStrategy\AttributeTranslationStrategy());

$referrerTestObj = new ReferrerTestObjMultilang();
$referrerRefTestObj = new ReferrerRefTestObj();

$referrerTestObj->id = "/functional/referrerTestObj";
$referrerTestObj->name = "referrer";
$referrerRefTestObj->id = "/functional/referrerRefTestObj";
$referrerRefTestObj->name = "referenced";

$referrerTestObj->reference = $referrerRefTestObj;

$this->dm->persist($referrerTestObj);
$this->dm->flush();
$this->dm->clear();

$referrer = $this->dm->find(null, '/functional/referrerTestObj');
$referenced = $this->dm->find(null, "/functional/referrerRefTestObj");

$referrer->name = 'changed';
$this->assertEquals('changed', $referrer->name);

$this->assertCount(1, $referenced->referrers);
$this->assertSame($referrer, $referenced->referrers->first());
$this->assertEquals('changed', $referrer->name);
}
}

/**
Expand Down Expand Up @@ -669,6 +709,22 @@ class ReferrerTestObj
public $reference;
}

/**
* @PHPCRODM\Document(translator="attribute")
*/
class ReferrerTestObjMultilang
{
/** @PHPCRODM\Id */
public $id;
/** @PHPCRODM\String(translated=true) */
public $name;
/** @PHPCRODM\ReferenceOne(targetDocument="ReferrerRefTestObj", cascade="persist") */
public $reference;

/** @PHPCRODM\Locale */
protected $locale;
}

/**
* @PHPCRODM\Document()
*/
Expand Down

0 comments on commit 7e310fe

Please sign in to comment.