[WIP] fix referrers cascade persist #234

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants
Member

dbu commented Jan 31, 2013

this now almost works, except that we seem to hit some jackalope bug which i seem to confused to understand.

note that atm this breaks two other tests:

  • Doctrine\Tests\ODM\PHPCR\Functional\EventManagerTest::testTriggerEvents => wft is https://github.com/doctrine/phpcr-odm/blob/master/tests/Doctrine/Tests/ODM/PHPCR/Functional/EventManagerTest.php#L129 trying to achieve? is this leftover code from when referrers where not cascading yet? if we really want to test something about referrers there, we now need to define the filter option to the mapping as cascading persist only works if we know the name of the referring property.
  • Doctrine\Tests\ODM\PHPCR\Functional\MergeTest::testMergeNewDocument => here i am not sure why this fails and did work before
Member

lsmith77 commented Jan 31, 2013

do i understand properly that we need the filter only for writing? maybe we can keep it optional but then just not support writing? also shouldnt we then also call it mappedBy if its semantically similar to the ORM?

Member

dbu commented Jan 31, 2013

i want to rename it to mappedBy but did not want to mix too many things into one pull request.

and we only need the filter/mappedBy if we want to cascade persisting. in that case it is not optional (we need to know the field name of the referencing document to set the id of this document to it to build the reference). if we do not cascade persists we don't care. we can even cascade other operations actually, as we only need the document for that, not the property referencing this document.

if you have feedback on why merge could have this problem now or understand what the EventManagerTest is supposed to do that would be great.

btw, i was expecting to get a travis build on this PR but aparently not...

Member

lsmith77 commented Jan 31, 2013

travis is having issues because of the rubygem.org hack.

Member

lsmith77 commented Jan 31, 2013

i think the issue with the merge test is fixed by:

diff --git a/lib/Doctrine/ODM/PHPCR/UnitOfWork.php b/lib/Doctrine/ODM/PHPCR/UnitOfWork.php
index f9ab9a8..2a77aba 100644
--- a/lib/Doctrine/ODM/PHPCR/UnitOfWork.php
+++ b/lib/Doctrine/ODM/PHPCR/UnitOfWork.php
@@ -1265,14 +1265,12 @@ class UnitOfWork

     private function cascadeMergeCollection($managedCol, array $mapping)
     {
-        if (!$managedCol instanceof PersistentCollection ) {
+        if (!$managedCol instanceof PersistentCollection) {
             return;
         }

         if ($mapping['cascade'] & ClassMetadata::CASCADE_MERGE > 0) {
-            $managedCol->initialize();
-            if (!$managedCol->isEmpty()) {
-                // clear managed collection, in casacadeMerge() the collection is filled again.
+            if ($managedCol->isInitialized() && !$managedCol->isEmpty()) {
                 $managedCol->unwrap()->clear();
                 $managedCol->setDirty(true);
             }

i am not sure why i forced the initialization before .. especially since isEmpty() would do it anyway.

but with the fix i pushed for the EventManagerTest::testTriggerEvents test and the above fix .. i have your new test failing in either case.

Member

lsmith77 commented Jan 31, 2013

i have thought about this some more and i am no longer sure that it makes sense to support cascading for referrers. with this solution it really only means that we support the case when all the referrers map to a single property name. yet the referrers collection could map to various different document classes with different property names. so it seems to me like we cannot really provide a solid general solution. so i wonder if we shouldnt just drop the idea.

instead we could document and encourage the following pattern:

<?php

namespace Doctrine\Tests\Models\CMS;

use Doctrine\ODM\PHPCR\Mapping\Annotations as PHPCRODM;

/**
 * @PHPCRODM\Document(repositoryClass="Doctrine\Tests\Models\CMS\CmsPageRepository", referenceable=true)
 */
class CmsPage
{
    /** @PHPCRODM\Referrers(referenceType="hard") */
    public $items = array();

    public function addItem($item)
    {
        $item->setDocumentTarget($this);
        $this->items[] = $item;
    }
}
Member

dbu commented Feb 1, 2013

the problem with that is that you still would have to persist $item separately, as otherwise you will get the error about not cascading and unpersisted item found. leaving the impl as it is right now with cascading persist but not creating the backlink, it really hides the problem.

your model is still possible in case you do have mixed reference property names, when not cascading the referrers and persisting the other entity manually.

we can still allow referrers without filter, but have to enforce that if you want to cascade persist, you have to specify it. to me it seems like an acceptable compromise. we can enforce valid configuration for all situations at the time when we parse the mapping (except we don't know if the other entity will have the mapped property we filter referrers on - but this is the price of the flexibility of phpcr).

but i think in reality we most of the time will use the filter/mappedBy attribute of the referrers mapping. otherwise you can not distinguish what type of reference points to this document. (assume i would implement a todo-list bundle that creates reference documents that point to content documents. they would end up mixed with the routes. then we could filter on instanceof, but that creates a lot of dependency which in other cases than routes is again not desirable or even impossible)

Member

lsmith77 commented Feb 1, 2013

yes it would mean its your responsibility for persisting them. not sure if there is a way for users to get some level of "automation" via events. but specifying a filter/mappedBy doesnt cover all the cases, which is why i think this approach is flawed. it only works if all your referrers really only map to a single property name.

Member

dbu commented Feb 1, 2013

doctrine events is not the right thing i fear, doing such things in the middle of a flush is tricky.

in my opinion we could still implement this. it is a valid use case to have a mappedBy. if you don't have it, then you need the more manual solution you outlined. it involves more work (like in sonata admin overwriting some method to wire things together and persist related pieces) and thus i think it would make sense to allow the cascade in the case where we actually can. we can clearly identify the situations when parsing the mapping, so there is no late errors or silently failing behaviour.

Member

lsmith77 commented Feb 1, 2013

we cannot know at mapping parsing what will be placed into the referrer collection. so at most we can determine at runtime that something went wrong. furthermore I am not sure if we don't also have to then make the class name explicit too and then essentially forbid cascading mixed referrer collection

Member

dbu commented Feb 1, 2013

but if the user wires the documents together manually and tries to use a
document that does not have the method to attach itself to the other
document, he will get a fatal error, and only at runtime too.
so the only difference is whether the error happens in userland code or
in phpcr-odm where we can explain with the exception why it did not
work. i don't see a big difference in this. obviously we need to
document it well, but thats not harder than documenting how to implement
yourself things to work around a missing feature.

Member

lsmith77 commented Feb 8, 2013

was just discussing this topic with @rryter.
what we really want is to be able to filter by class/node type.
since this is currently not supported by JCR/PHPCR what we could do is fallback to using SQL2/QOM to find the relevant uuid's

so we would support something like Referrers(targetDocument="Cart", mappedBy="customer"). now the targetDocument would be a child document. however for mappedBy we would then no longer be able to support glob syntax. so we could also allow to optionally use filter instead of targetDocument/mappedBy.

@lsmith77 lsmith77 closed this Feb 8, 2013

@lsmith77 lsmith77 reopened this Feb 8, 2013

Member

dbu commented Feb 8, 2013

i don't think the referrer property name ever is in glob syntax, that is why i want to rename that to mappedBy to avoid confusion.

about the nodetype/class filtering we could also get all referrers and then filter in userland. this should work fine in the cases where you want to be certain to get the right thing, but will of course not work with huge numbers of references to the same document.
filtering for a class but only when mappedBy is used sounds rather pointless to me. if you know the property name, you can probably also separate the referrers by that. the tricky thing is when you have various properties that hold the reference.

Member

lsmith77 commented Feb 8, 2013

filter indeed seems to require a full name atm. so yeah then mappedBy is better. but by targetDocument is also quite important. f.e. to be able to handle this case:
https://github.com/symfony-cmf/MenuBundle/blob/master/Document/MenuNode.php#L48

i might want to have a collection of all MenuNode instances .. no matter if its a weak or strong reference.

Member

dbu commented Feb 19, 2013

regarding the TODO i have about other modifications, i just created http://www.doctrine-project.org/jira/browse/PHPCR-90

Member

dbu commented Feb 20, 2013

another approach could be:

  • Referrers has the mappedBy attribute required and is mutable and can cascade
  • MixedReferrers has no mappedBy and is an immutable collection of all referrers
Member

lsmith77 commented Feb 20, 2013

i think this sounds like a good solution

Member

dbu commented Feb 27, 2013

there is #252 now

@dbu dbu closed this Feb 27, 2013

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