[WIP] Split Referrers into MixedReferrers and Referrers with cascading #252

Merged
merged 15 commits into from Mar 6, 2013

2 participants

@dbu
Doctrine member

this replaces #234

we now have two distinct mappings:

  • MixedReferrers behaves like the Referrers we had, but has no filter attribute. It is an immutable collection of any referrers to this document
  • Referrers is a fully mapped field like ReferenceMany where you can add and remove items. instead of filter the attribute is properly named mappedBy and required.
@dbu dbu commented on the diff Feb 24, 2013
...ODM/PHPCR/Mapping/Annotations/DoctrineAnnotations.php
@@ -230,6 +233,11 @@ class TranslatableProperty extends Property
*/
class Reference
{
+ /**
+ * The PHPCR property name to use
+ * @var string
+ */
+ public $name;
@dbu
Doctrine member
dbu added a note Feb 24, 2013

this is not directly related to the referrers change, but i noticed we forgot that a reference property is a property and thus could be mapped to a name different than the document field storing it.

@lsmith77
Doctrine member

ok

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dbu dbu commented on the diff Feb 24, 2013
lib/Doctrine/ODM/PHPCR/Mapping/ClassMetadata.php
@@ -773,6 +802,13 @@ public function validateClassMapping()
throw new MappingException("You must define a locale mapping for translatable document '".$this->name."'");
}
}
+
+ // we allow mixed referrers on non-referenceable documents. maybe the mix:referenceable is just not mapped
@dbu
Doctrine member
dbu added a note Feb 24, 2013

does this make sense? i had to do it to have the Generic document still working - as that one does not declare referenceable=true but has its referrers mapped. i guess it makes kind of sense.

@lsmith77
Doctrine member

not sure i understand .. the Generic document is still not referenceable, so shouldn't it know trigger this exception?

@dbu
Doctrine member
dbu added a note Feb 25, 2013

i only look at referrersMappings, not at mixedReferrersMappings to throw the exception. the Generic document maps the mixed referrers. as the mixed referrers are immutable anyways, it does not matter. and for something like the Generic document it really makes sense to do it, we don't know if its referenceable or not.
for the editable referrers it makes less sense to map on a document that is not sure to be referenceable - it would still work if the document is made referenceable on PHPCR level.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dbu dbu and 1 other commented on an outdated diff Feb 24, 2013
lib/Doctrine/ODM/PHPCR/UnitOfWork.php
@@ -394,7 +394,12 @@ public function getOrCreateDocument($className, NodeInterface $node, array &$hin
foreach ($class->referrersMappings as $fieldName) {
$mapping = $class->mappings[$fieldName];
- $documentState[$fieldName] = new ReferrersCollection($this->dm, $document, $mapping['referenceType'], $mapping['filter']);
+ $documentState[$fieldName] = new ReferrersCollection($this->dm, $document, $mapping['referenceType'], $mapping['mappedBy']);
+ }
+ foreach ($class->mixedReferrersMappings as $fieldName) {
+ $mapping = $class->mappings[$fieldName];
+ // this should be an immutable collection. can we do that in a sane way?
@dbu
Doctrine member
dbu added a note Feb 24, 2013

can we do immutable collections ?

@lsmith77
Doctrine member

i dont think there is an immutable collection in common, but we could write one.

@dbu
Doctrine member
dbu added a note Feb 25, 2013

the problem is that it needs to be a ReferrersCollection too. so it would be the ImmutableReferrersCollection which feels weird. or we could see if we can inject a strategy for getting children, references or referrers into a generic collection and then have a simple collection and a immutable collection where we can inject those.

but honestly i think for now its just good enough like this. you do get an exception if you accidentally modify the collection, just a bit late in the process.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dbu dbu commented on the diff Feb 24, 2013
lib/Doctrine/ODM/PHPCR/UnitOfWork.php
@@ -1003,6 +1008,14 @@ private function computeChangeSet(ClassMetadata $class, $document)
}
}
}
+ foreach ($class->mixedReferrersMappings as $fieldName) {
+ if ($actualData[$fieldName]
+ && $actualData[$fieldName] instanceof PersistentCollection
+ && $actualData[$fieldName]->isDirty()
+ ) {
+ throw new PHPCRException("The immutable mixed referrer collection in field $fieldName is dirty");
@dbu
Doctrine member
dbu added a note Feb 24, 2013

this is the late detection if the immutable collection was modified.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dbu dbu commented on the diff Feb 24, 2013
lib/Doctrine/ODM/PHPCR/UnitOfWork.php
@@ -1419,7 +1432,19 @@ private function doMerge($document, array &$visited, $prevManagedCopy = null, $a
$this->dm,
$managedCopy,
$mapping['referenceType'],
- $mapping['filter']
+ $mapping['mappedBy']
+ );
+ $prop->setValue($managedCopy, $managedCol);
+ $this->originalData[$managedOid][$fieldName] = $managedCol;
+ }
+ $this->cascadeMergeCollection($managedCol, $mapping);
+ } elseif ('mixedreferrers' === $mapping['type']) {
@dbu
Doctrine member
dbu added a note Feb 24, 2013

do i understand correctly that we need to merge mixed referrers as well even though its immutable, as there might be new ones coming from the backend?

@lsmith77
Doctrine member

yeah .. i guess it makes sense to merge this as well.

@dbu
Doctrine member
dbu added a note Feb 25, 2013

good, thats what we do, so then its fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dbu dbu and 2 others commented on an outdated diff Feb 24, 2013
lib/Doctrine/ODM/PHPCR/UnitOfWork.php
+ * reference this document, so we have to update its
+ * referencing property to contain the uuid of this
+ * document
+ */
+ foreach ($fieldValue as $fv) {
+ if ($fv === null) {
+ continue;
+ }
+
+ $referencingNode = $this->session->getNode($this->getDocumentId($fv));
+ $referencingMeta = $this->dm->getClassMetadata(get_class($fv));
+ $referencingField = null;
+ foreach ($referencingMeta->referenceMappings as $fieldName) {
+ $field = $referencingMeta->getAssociation($fieldName);
+
+ if ($field['name'] === $mapping['mappedBy']) {
@dbu
Doctrine member
dbu added a note Feb 24, 2013

mappedBy goes against the PHPCR property name, not the document field name as we do not know what documents refer to us.

@lsmith77
Doctrine member

does this makes sense though? why not also mandate a targetDocument ?

@dbu
Doctrine member
dbu added a note Feb 25, 2013

hm, you have a point here. but on the other hand it offers a lot of flexibility without the targetDocument. should we leave it open to the user and provide a mappedByPhpcr, and a mappedBy + targetDocument and you need to specify either one or the other?

i think targetDocument can not be an interface, it could only be a common base document class, right? mapping inheritance only works on class inheritance, i can't implement an interface to pull in more mappings, or can i?

@lsmith77
Doctrine member

i think the flexibility gets us back to a lot of the problems we are trying to fix with this approach. i mean i want things to be clear and predictable as this is easily quite confusing. if all else fails people will simply need to manage this manually like they need to today.

I think having just the field opens up unpredictable behavior, e.g. in the use case of "(ContentObject)->$routes" if there is another document which just happens to have the field name "routeContent" it will break the system.

Personally I would add a targetDocument (or targetDocuments) and make it mandatory. It seems to me very "fuzzy" otherwise.

@dbu
Doctrine member
dbu added a note Feb 26, 2013

ok, i think you are right. otherwise we mix the layers completely. targetDocuments sounds interesting, but would be weird. what if some of the documents do not use the same phpcr field for the reference? rather have the user create several Referrers annotations. anyways it does not help towards the flexibility of finding referencing documents that this document knows nothing about. i guess for that use case people will just have to go through the DM.
or we could add a phpcrName to MixedReferrers for the use case? would still be readonly but that is good enough. wdyt?

now what to call things? i wonder if targetDocument is right, its the document referencing us, so its rather sourceDocument. would that make sense to you? it would stress the directional nature of references in PHPCR. but its harder to guess for users not familiar with phpcr-odm - but then its required so we can explain in the exception when its missing.
we can get rid of the weak/hard for Referrers, as that information is in the Reference mapping already.
how do we read the other field? if we request the metadata of the sourceDocument (or targetDocument) in there, we risk endless loops if the other document has a referrer annotation for this document (for example self-referencing). can we handle that once the metadata is already registered with the document manager?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dbu dbu commented on an outdated diff Feb 24, 2013
lib/Doctrine/ODM/PHPCR/UnitOfWork.php
+ }
+ $uuid = $node->getIdentifier();
+
+ $type = $referencingField['strategy'] == 'weak' ? PropertyType::WEAKREFERENCE : PropertyType::REFERENCE;
+ if ($referencingField['type'] === ClassMetadata::MANY_TO_ONE) {
+ $ref = $referencingMeta->getFieldValue($fv, $referencingField['fieldName']);
+ if ($ref !== null && $ref !== $document) {
+ throw new PHPCRException(sprintf('Conflicting settings for referrer and reference: Document %s field %s points to %s but document %s has set first document as referrer on field %s', self::objToStr($fv, $this->dm), $referencingField['fieldName'], self::objToStr($ref, $this->dm), self::objToStr($document, $this->dm), $mapping['fieldName']));
+ }
+ // update the referencing document field to point to this document
+ $referencingMeta->setFieldValue($fv, $referencingField['fieldName'], $document);
+ // and make sure the reference is not deleted in this change because the field could be null
+ unset($this->documentChangesets[spl_object_hash($fv)]['fields'][$referencingField['fieldName']]);
+ $referencingNode->setProperty($referencingField['name'], $uuid, $type);
+ } elseif ($referencingField['type'] === ClassMetadata::MANY_TO_MANY) {
+ // TODO: need to update referencing node too
@dbu
Doctrine member
dbu added a note Feb 24, 2013

how exactly do we handle this? just add a reference to us to the field of the document? what about killing the invalid changeset, can that happen too?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dbu dbu commented on an outdated diff Feb 24, 2013
lib/Doctrine/ODM/PHPCR/UnitOfWork.php
+ unset($this->documentChangesets[spl_object_hash($fv)]['fields'][$referencingField['fieldName']]);
+ $referencingNode->setProperty($referencingField['name'], $uuid, $type);
+ } elseif ($referencingField['type'] === ClassMetadata::MANY_TO_MANY) {
+ // TODO: need to update referencing node too
+ if ($referencingNode->hasProperty($referencingField['name'])) {
+ if (! in_array($uuid, $referencingNode->getPropertyValue($referencingField['name']), PropertyType::STRING)) {
+ $referencingNode->getProperty($referencingField['name'])->addValue($uuid); // property should be correct type already
+ }
+ } else {
+ $referencingNode->setProperty($referencingField['name'], array($uuid), $type);
+ }
+ } else {
+ throw new PHPCRException(sprintf('Field "%s" of document "%s" is not a reference field. Error in referrer annotation: '.self::objToStr($document, $this->dm), $mapping['mappedBy'], get_class($fv)));
+ }
+ }
+ }
@dbu
Doctrine member
dbu added a note Feb 24, 2013

this only handles / cascades adding. (does it handle adding just the reference if we do not cascade adding?) we should handle removals as well, just delete the reference in the other field if not cascading, remove the referencing document if cascading.

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

we still have the failing tests from #234 and i still don't understand why.

@dbu dbu commented on an outdated diff Feb 24, 2013
lib/Doctrine/ODM/PHPCR/UnitOfWork.php
+ continue;
+ }
+
+ $referencingNode = $this->session->getNode($this->getDocumentId($fv));
+ $referencingMeta = $this->dm->getClassMetadata(get_class($fv));
+ $referencingField = null;
+ foreach ($referencingMeta->referenceMappings as $fieldName) {
+ $field = $referencingMeta->getAssociation($fieldName);
+
+ if ($field['name'] === $mapping['mappedBy']) {
+ $referencingField = $field;
+ break;
+ }
+ }
+ if (! $referencingField) {
+ throw new PHPCRException(sprintf('Document %s set as a referrer of %s has no field mapped to the PHPCR property %s', $this->getDocumentId($fv), self::objToStr($document, $this->dm), $mapping['mappedBy']));
@dbu
Doctrine member
dbu added a note Feb 24, 2013

actually we could ignore that we found no field and just set the phpcr property. a reference/referrer relation could just be mapped with the referrer mapping and that would work just as well. the problem is that a typo would go unnoticed and lead to confusion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dbu dbu commented on an outdated diff Feb 27, 2013
lib/Doctrine/ODM/PHPCR/Mapping/ClassMetadata.php
+ // we allow mixed referrers on non-referenceable documents. maybe the mix:referenceable is just not mapped
+ if (count($this->referrersMappings)) {
+ if (!$this->referenceable) {
+ throw new MappingException('You can not have referrers mapped on document "'.$this->name.'" as the document is not referenceable');
+ }
+
+ foreach ($this->referrersMappings as $mapping) {
+ /** @var $referringDocument ClassMetadata */
+ $referringDocument = $this->metadataFactory->getMetadataFor($mapping['referringDocument']);
+ if (! $referringDocument->hasAssociation($mapping['referencedBy'])) {
+ throw new MappingException(sprintf('Invalid referrer mapping on document "%s" for field "%s": The referringDocument "%s" has no field "%s"', $this->name, $mapping['fieldName'], $mapping['referringDocument'], $mapping['referencedBy']));
+ }
+ $referencingField = $referringDocument->getAssociation($mapping['referencedBy']);
+ if (ClassMetadata::MANY_TO_MANY !== $referencingField['type'] && ClassMetadata::MANY_TO_ONE !== $referencingField['type']) {
+ throw new MappingException(sprintf('Invalid referrer mapping on document "%s" for field "%s": Field "%s" of document "%s" is not a reference field', $this->name, $mapping['fieldName'], $mapping['referencedBy'], $mapping['referringDocument']));
+ }
@dbu
Doctrine member
dbu added a note Feb 27, 2013

we just do reflection at this point to avoid cyclic dependencies

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
dbu added some commits Feb 27, 2013
@dbu dbu WIP 834e3f2
@dbu dbu Merge branch 'master' into split-referrer
Conflicts:
	lib/Doctrine/ODM/PHPCR/DocumentManager.php
	lib/Doctrine/ODM/PHPCR/UnitOfWork.php
ccd741c
@dbu dbu cleanup handling, cascade everything along with the references 06cb5a2
@dbu dbu commented on the diff Mar 3, 2013
lib/Doctrine/ODM/PHPCR/ImmutableReferrersCollection.php
+use Doctrine\Common\Collections\ArrayCollection;
+
+/**
+ * Immutable referrer collection class
+ *
+ * This class represents a collection of referrers of a document that can be
+ * mixed and thus never can be persisted.
+ */
+class ImmutableReferrersCollection extends ReferrersCollection
+{
+ public function __construct(DocumentManager $dm, $document, $type = null, $locale = null)
+ {
+ parent::__construct($dm, $document, $type, null, $locale);
+ }
+
+ // TODO: overwrite all methods that would modify this collection and throw exceptions
@dbu
Doctrine member
dbu added a note Mar 3, 2013

i just add the class for now, as discussed. so that client code can at least see from the class if this is an immutable collection.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dbu dbu commented on the diff Mar 3, 2013
lib/Doctrine/ODM/PHPCR/Mapping/ClassMetadata.php
if (empty($mapping['cascade'])) {
$mapping['cascade'] = 0;
}
+
+ $mapping['sourceDocument'] = $this->name;
+ if (isset($mapping['referringDocument']) && strpos($mapping['referringDocument'], '\\') === false && strlen($this->namespace)) {
+ $mapping['referringDocument'] = $this->namespace . '\\' . $mapping['referringDocument'];
+ }
@dbu
Doctrine member
dbu added a note Mar 3, 2013

i do this here as validateAndCompleteAssociation does it only on targetDocument. or should i make the validate method take a parameter for the field name that could contain a relative class name?

@lsmith77
Doctrine member
lsmith77 added a note Mar 5, 2013

think its fine like that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dbu dbu commented on the diff Mar 3, 2013
lib/Doctrine/ODM/PHPCR/Mapping/ClassMetadata.php
$this->referrersMappings[] = $mapping['fieldName'];
}
+ public function mapMixedReferrers(array $mapping, ClassMetadata $inherited = null)
+ {
+ if (!(array_key_exists('referenceType', $mapping) && in_array($mapping['referenceType'], array(null, "weak", "hard")))) {
+ throw new MappingException("You have to specify a 'referenceType' for the '" . $this->name . "' mapping which must be null, 'weak' or 'hard': ".$mapping['referenceType']);
+ }
+
+ if (isset($mapping['referredBy'])) {
+ throw new MappingException('MixedReferrers has no referredBy attribute, use Referrers for this: ' . $mapping['fieldName']);
+ }
+ if (empty($mapping['cascade'])) {
+ $mapping['cascade'] = null;
+ }
@dbu
Doctrine member
dbu added a note Mar 3, 2013

the annotation does not allow cascade. should we cascade anything here ever?

@lsmith77
Doctrine member
lsmith77 added a note Mar 5, 2013

ever is such an infinite absolute .. but i doubt it ..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dbu dbu commented on the diff Mar 3, 2013
lib/Doctrine/ODM/PHPCR/UnitOfWork.php
@@ -751,7 +766,7 @@ private function doRemove($document, &$visited)
private function cascadeRemove(ClassMetadata $class, $document, &$visited)
{
- foreach ($class->referenceMappings as $fieldName) {
+ foreach (array_merge($class->referenceMappings, $class->referrersMappings) as $fieldName) {
@dbu
Doctrine member
dbu added a note Mar 3, 2013

i merge references and referrer mappings wherever we might need to cascade referrers. i hope this makes sense like this?

@lsmith77
Doctrine member
lsmith77 added a note Mar 5, 2013

yes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dbu dbu commented on the diff Mar 3, 2013
lib/Doctrine/ODM/PHPCR/UnitOfWork.php
@@ -1944,6 +1986,77 @@ private function executeUpdates($documents, $dispatchEvents = true)
}
}
}
+ } elseif ('referrers' === $mapping['type']) {
@dbu
Doctrine member
dbu added a note Mar 3, 2013

could you please review this method again? i think i refactored as we discussed, but i might have confused or forgotten something.

@lsmith77
Doctrine member
lsmith77 added a note Mar 5, 2013

looks good to me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dbu
Doctrine member
dbu commented Mar 3, 2013

i went through this again, and i think its mostly finished. i have one failing test which surpises me, seems something is somewhere eating away triggering an event. any idea why this happens? seems not to happen on master so it is probably related to the changes here...

@lsmith77 lsmith77 commented on the diff Mar 5, 2013
tests/Doctrine/Tests/Models/CMS/CmsPage.php
@@ -20,7 +20,7 @@ class CmsPage
public $content;
/** @PHPCRODM\String(name="title") */
public $title;
- /** @PHPCRODM\Referrers(referenceType="hard", cascade="persist") */
+ /** @PHPCRODM\MixedReferrers(referenceType="hard") */
@lsmith77
Doctrine member
lsmith77 added a note Mar 5, 2013

this is why your test is failing.

doing the following fixes it:


diff --git a/tests/Doctrine/Tests/ODM/PHPCR/Functional/EventManagerTest.php b/tests/Doctrine/Tests/ODM/PHPCR/Functional/EventManagerTest.php
index 8050446..0f3b12d 100644
--- a/tests/Doctrine/Tests/ODM/PHPCR/Functional/EventManagerTest.php
+++ b/tests/Doctrine/Tests/ODM/PHPCR/Functional/EventManagerTest.php
@@ -81,6 +81,7 @@ class EventManagerTest extends PHPCRFunctionalTestCase
         $item->documentTarget = $page;

         $page->content = "short story";
+        $this->dm->persist($item);
         $page->addItem($item);

         $this->dm->persist($page);
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@lsmith77
Doctrine member

ready to merge from my POV

@lsmith77 lsmith77 Merge remote-tracking branch 'origin/master' into split-referrer
* origin/master:
  removed scheduledAssociationUpdates

Conflicts:
	lib/Doctrine/ODM/PHPCR/UnitOfWork.php
70d714d
@dbu dbu merged commit 900b8ca into master Mar 6, 2013

1 check passed

Details default The Travis build passed
@dbu dbu deleted the split-referrer branch Mar 6, 2013
@dbu dbu added a commit that referenced this pull request Mar 6, 2013
@dbu dbu changelog for referres refactoring #252 61bd13f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment