Skip to content
This repository

References (request for comments) #52

Merged
merged 28 commits into from over 2 years ago

4 participants

Johannes Stark Lukas Kahwe Smith Benjamin Eberlei David Buchmann
Johannes Stark
starkj commented July 30, 2011

No description provided.

Lukas Kahwe Smith

this looks like a typo

Lukas Kahwe Smith

would be good to split the condition over several lines

Lukas Kahwe Smith

missing a space after if

lib/Doctrine/ODM/PHPCR/Proxy/ReferenceProxyFactory.php
((285 lines not shown))
  285
+    {
  286
+        if (!$this->__isInitialized__ && $this->__doctrineDocumentManager__) {
  287
+            $this->__isInitialized__ = true;
  288
+            $this->__doctrineDocumentManager__->createDocumentForProxy($this);
  289
+            unset($this->__doctrineDocumentManager__);
  290
+        }
  291
+    }
  292
+
  293
+    <methods>
  294
+
  295
+    public function __sleep()
  296
+    {
  297
+        <sleepImpl>
  298
+    }
  299
+
  300
+    public function __set($name, $value) {
3
Lukas Kahwe Smith Collaborator
lsmith77 added a note July 30, 2011

{` needs to be on the next line ..

aside from this .. @beberlei what do you think of this? @starkj said that the other ODM's do not handle __set() and __get()

Benjamin Eberlei Owner
beberlei added a note July 30, 2011

They do handle it, because it naturally appears in $reflClass->getMethods().

Johannes Stark
starkj added a note July 30, 2011

the old proxy classes that were created didn't contain get and __set methods. but we need to adjust them so that they will execute __doctrineLoad()
in adition to that, I had to unset the document classes attributes.
it's working, but is this a nice solution? or could it be done better?

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

maybe add a TODO item that these (__set() and __get()) should actually not be needed

Lukas Kahwe Smith lsmith77 commented on the diff July 30, 2011
lib/Doctrine/ODM/PHPCR/Mapping/Driver/AnnotationDriver.php
@@ -142,6 +142,11 @@ class AnnotationDriver implements Driver
142 142
         }
143 143
         $class->setNodeType($documentAnnot->nodeType);
144 144
 
  145
+        if (isset($documentAnnot->referenceable) && $documentAnnot->referenceable)
1
Lukas Kahwe Smith Collaborator
lsmith77 added a note July 30, 2011

one more CS violation :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
tests/Doctrine/Tests/ODM/PHPCR/Functional/ReferenceTest.php
((25 lines not shown))
  25
+            $root->getNode('functional')->remove();
  26
+            $this->session->save();
  27
+        }
  28
+
  29
+        $this->node = $root->addNode('functional');
  30
+
  31
+        $this->session->save();
  32
+    }
  33
+
  34
+    public function testCreate()
  35
+    {
  36
+        $refTestObj = new RefTestObj();
  37
+        $refRefTestObj = new RefRefTestObj();
  38
+
  39
+        $refTestObj->id = "/functional/refTestObj";
  40
+       $refRefTestObj->id = "/functional/refRefTestObj";
1
Lukas Kahwe Smith Collaborator
lsmith77 added a note July 30, 2011

one more CS violation :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
lib/Doctrine/ODM/PHPCR/UnitOfWork.php
@@ -458,7 +544,7 @@ class UnitOfWork
458 544
 
459 545
             $changed = false;
460 546
             foreach ($actualData as $fieldName => $fieldValue) {
461  
-                if (!isset($class->fieldMappings[$fieldName]) && !isset($class->childMappings[$fieldName])) {
  547
+                if (!isset($class->fieldMappings[$fieldName]) && !isset($class->childMappings[$fieldName]) && !isset($class->associationsMappings[$fieldName])) {
1
Lukas Kahwe Smith Collaborator
lsmith77 added a note July 30, 2011

maybe break this condition on multiple lines

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

@beberlei: it would be great if you could do a final review before we merge this.

and others added some commits July 30, 2011
cli-config.php.dist
@@ -7,7 +7,9 @@ $user = 'admin';
7 7
 $pass = 'admin';
8 8
 
9 9
 /* bootstrapping the repository implementation. for jackalope, do this: */
10  
-$repository = new \Jackalope\Repository(new \Jackalope\Factory, $url);
  10
+$factory = new \Jackalope\Factory;
  11
+$transport = new \Jackalope\Transport\Davex\Client($factory, $url);
  12
+$repository = new \Jackalope\Repository($factory, $transport);
2
Johannes Stark
starkj added a note August 01, 2011

or would it be better to use the factory instead to get the transport and repository object?

Lukas Kahwe Smith Collaborator
lsmith77 added a note August 03, 2011

yes .. you might want to rebase to master when you do that, since some things changed there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Benjamin Eberlei beberlei commented on the diff August 02, 2011
lib/Doctrine/ODM/PHPCR/Proxy/ReferenceProxyFactory.php
((285 lines not shown))
  285
+    {
  286
+        if (!$this->__isInitialized__ && $this->__doctrineDocumentManager__) {
  287
+            $this->__isInitialized__ = true;
  288
+            $this->__doctrineDocumentManager__->createDocumentForProxy($this);
  289
+            unset($this->__doctrineDocumentManager__);
  290
+        }
  291
+    }
  292
+
  293
+    <methods>
  294
+
  295
+    public function __sleep()
  296
+    {
  297
+        <sleepImpl>
  298
+    }
  299
+
  300
+    public function __set($name, $value)
4
Benjamin Eberlei Owner
beberlei added a note August 02, 2011

This is still not necessary. __set/__get are overloaded if they are present in the proxied class, otherwise this just leads to weird behavior.

Johannes Stark
starkj added a note August 03, 2011

What I wanted to achieve exactly is that you are able to use the references in this way:

a)
$referrer = $this->dm->find($this->referrerType, '/functional/refTestObj');
echo $referrer->reference->name;

and in that way:

b)
echo $referrer->reference->getName();

Did I get you right, that if I want to do it like in a) it's better to define __set() and __get() already in the main class? And so we have to give users a hint in the doc that they have to define the magic methods themselves.

Lukas Kahwe Smith Collaborator
lsmith77 added a note August 05, 2011

ok .. this seems to be the last remaining open question.

Johannes Stark
starkj added a note August 05, 2011

while adding some more tests i just found an other issue concerning deleting one out of many references (testcase testDeleteOneInMany)
but its fixed now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
lib/Doctrine/ODM/PHPCR/UnitOfWork.php
((28 lines not shown))
197 231
                 // TODO figure this one out which collection should be used
198  
-                $documentState[$class->associationsMappings[$assocName]['fieldName']] = null;
  232
+
  233
+                $config = new Configuration();
2
Benjamin Eberlei Owner
beberlei added a note August 02, 2011

What is this for?

Johannes Stark
starkj added a note August 03, 2011

yes, $this->dm->getConfiguration() is better.
still not fully aware of the whole architecture ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
lib/Doctrine/ODM/PHPCR/UnitOfWork.php
((40 lines not shown))
  242
+                $referencedId = $referencedNode->getPath();
  243
+
  244
+                $referencedClass = $this->dm->getMetadataFactory()->getMetadataFor(ltrim($assocOptions['targetDocument'], '\\'));
  245
+                $proxyDocument = $this->referenceProxyFactory->getProxy($referencedClass->name, $referencedId);
  246
+
  247
+                // register the referenced document under its own id
  248
+                $this->registerManaged($proxyDocument, $referencedId, null);
  249
+
  250
+                $documentState[$class->associationsMappings[$assocName]['fieldName']] = $proxyDocument;
  251
+
  252
+                // save node for the case that the referenced document will be created
  253
+                $proxyOid = spl_object_hash($proxyDocument);
  254
+                $this->nodesMap[$proxyOid] = $referencedNode;
  255
+
  256
+            } elseif ($assocOptions['type'] & ClassMetadata::MANY_TO_MANY) {
  257
+                $config = new Configuration();
1
Benjamin Eberlei Owner
beberlei added a note August 02, 2011

here again, why not grab the config from $this->dm->getConfiguration() ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Benjamin Eberlei beberlei commented on the diff August 02, 2011
lib/Doctrine/ODM/PHPCR/UnitOfWork.php
((48 lines not shown))
  250
+                $documentState[$class->associationsMappings[$assocName]['fieldName']] = $proxyDocument;
  251
+
  252
+                // save node for the case that the referenced document will be created
  253
+                $proxyOid = spl_object_hash($proxyDocument);
  254
+                $this->nodesMap[$proxyOid] = $referencedNode;
  255
+
  256
+            } elseif ($assocOptions['type'] & ClassMetadata::MANY_TO_MANY) {
  257
+                $config = new Configuration();
  258
+                $this->referenceProxyFactory = new Proxy\ReferenceProxyFactory($this->dm, $config->getProxyDir(), $config->getProxyNamespace(), true);
  259
+
  260
+                if (! $node->hasProperty($assocOptions['fieldName'])) {
  261
+                    continue;
  262
+                }
  263
+
  264
+                // get the already cached referenced nodes
  265
+                $proxyNodes = $node->getPropertyValue($assocOptions['fieldName']);
3
Benjamin Eberlei Owner
beberlei added a note August 02, 2011

Shouldn't there be a type check for is_array here or similar?

Johannes Stark
starkj added a note August 03, 2011

yes i will observe this again, and a test needs to be added for a reference-many-property with just one reference

Johannes Stark
starkj added a note August 03, 2011

Properties that references many nodes will always return the nodes as array, even if just one node is referenced.
I added an additional check in line 416 to make that definitely sure.
And if the repository is inconsistent, it's checked here again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Benjamin Eberlei beberlei commented on the diff August 02, 2011
lib/Doctrine/ODM/PHPCR/UnitOfWork.php
((5 lines not shown))
488 576
         foreach ($class->childMappings as $name => $childMapping) {
489 577
             if ($this->originalData[$oid][$name]) {
490 578
                 $this->computeChildChanges($childMapping, $this->originalData[$oid][$name], $id);
491 579
             }
492 580
         }
  581
+
  582
+        foreach ($class->associationsMappings as $assocName => $assoc) {
  583
+            if ($actualData[$assocName]) {
2
Benjamin Eberlei Owner
beberlei added a note August 02, 2011

Can you guarantee this exists?

Johannes Stark
starkj added a note August 03, 2011

yes, I think so
$actualData is filled with values from the reflection class.
so if the document has a reference property, the field in $actualData is set
at least with null, as there is no reference

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Benjamin Eberlei beberlei commented on the diff August 02, 2011
lib/Doctrine/ODM/PHPCR/UnitOfWork.php
@@ -515,6 +615,26 @@ class UnitOfWork
515 615
     }
516 616
 
517 617
     /**
  618
+     * Computes the changes of a reference.
  619
+     *
  620
+     * @param mixed $reference the referenced document.
  621
+     */
  622
+    private function computeReferenceChanges($mapping, $reference, $referrerId)
  623
+    {
  624
+        $targetClass = $this->dm->getClassMetadata(get_class($reference));
  625
+        $state = $this->getDocumentState($reference);
  626
+        $oid = spl_object_hash($reference);
  627
+        if ($state == self::STATE_NEW) {
2
Benjamin Eberlei Owner
beberlei added a note August 02, 2011

Shouldnt that be done if ccasce persist is set only?

Johannes Stark
starkj added a note August 03, 2011

I removed the option to cascade as I don't think it's necessary
If you're working with references and do some changes on them, why shouldn't that be persisted as well?
Correct me, if I'm wrong :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Benjamin Eberlei beberlei commented on the diff August 02, 2011
lib/Doctrine/ODM/PHPCR/UnitOfWork.php
@@ -608,6 +733,12 @@ class UnitOfWork
608 733
                 $node->addMixin("mix:versionable");
609 734
             }
610 735
 
  736
+            if ($class->referenceable) {
  737
+                $node->addMixin("mix:referenceable");
  738
+                // TODO make shure uuid is unique
2
Benjamin Eberlei Owner
beberlei added a note August 02, 2011

arent uuids unique by definition?

Johannes Stark
starkj added a note August 03, 2011

yes they are
but we calculate them ourselves and don't let Jackrabbit do that for us
so it might be possible to calculate a UUID twice

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
lib/Doctrine/ODM/PHPCR/UnitOfWork.php
((10 lines not shown))
668  
-                    } elseif ($class->associationsMappings[$fieldName]['type'] & ClassMetadata::TO_MANY) {
669  
-                        if ($class->associationsMappings[$fieldName]['isOwning']) {
670  
-                            // TODO: Optimize when not initialized yet! In ManyToMany case we can keep track of ALL ids
671  
-                            $ids = array();
672  
-                            if (is_array($fieldValue) || $fieldValue instanceof \Doctrine\Common\Collections\Collection) {
673  
-                                foreach ($fieldValue as $relatedObject) {
674  
-                                    $ids[] = $this->getDocumentId($relatedObject);
675  
-                                }
  795
+
  796
+                    if ($node->hasProperty($class->associationsMappings[$fieldName]['fieldName']) && is_null($fieldValue)) {
  797
+                        $node->getProperty($class->associationsMappings[$fieldName]['fieldName'])->remove();
  798
+                        continue;
  799
+                    }
  800
+
  801
+                    if($class->associationsMappings[$fieldName]['weak']) {
  802
+                        $type = \PHPCR\PropertyType::valueFromName('weakreference');
1
Benjamin Eberlei Owner
beberlei added a note August 02, 2011

Why not use the constants here directly?

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

Generally looks good. I fear the number of functional tests might be a bit small for such large features. Additionally i am a bit concerned about the "createDocumentforPRoxy" method naming and that its public from the DocumentManager. This is nothing more than a refresh but the naming sort of implies something new is created.

Johannes Stark

@beberlei
Thank you very much for having a close look on my pull request! Yes, there needs to be more unit test definitely. I will work on that. And I will make up my mind about createDocumentforProxy.

Johannes Stark

so, well I remove createDocumentForProxy from the DocumentManager and renamed the remaining methods to refreshDocumentForProxy

added some commits August 04, 2011
David Buchmann
Collaborator
dbu commented August 08, 2011

is this now ready to be merged or any open things?
@beberlei: would you mind checking if all concerns you noted are aptly handled now?

Johannes Stark

no, there are still a few things open
need to wait for benjamin's answer

David Buchmann
Collaborator
dbu commented August 08, 2011

once this is merged, we should close the following issues in the issue tracker:

annotation for referrers is not covered by this pull request, right? http://www.doctrine-project.org/jira/browse/PHPCR-16

Johannes Stark

no, referrers are not coverd

all right, i will remember the issue closing

Johannes Stark

@beberlei guess you're very busy at the moment but would you mind to give again some feedback on my changes and on the remaining questions? thanks a lot!

Johannes Stark

sorry guys, I screwed the commit history up a little bit.
Some commits appear twice but luckily it seems that this has no effect on the code.
I'm a little bit afraid to repair it using git rebase as the sequence of the commits listed by git is shuffled. :-(

btw referrer annotation is almost done. once this here is merged I can open another pull request for it

David Buchmann dbu merged commit 1083b50 into from August 23, 2011
David Buchmann dbu closed this August 23, 2011
David Buchmann
Collaborator
dbu commented August 23, 2011

thanks a lot for this feature, johannes!

Johannes Stark

No worries, it was a pleasure for me! :-)

Lukas Kahwe Smith
Collaborator

@starkj .. did you do this commit?

meaning in that case you are http://dlm.beberlei.de/licenses/author/346 ?

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

Showing 28 unique commits by 3 authors.

Jul 27, 2011
References with using proxy objects c1ec524
Jul 30, 2011
typo fixed, TODO added 9757384
Merge remote branch 'upstream/master' into references 9fae93d
get referenced nodes within one call to increase performance f6c1f1a
updating to latest jackalope a3bdb2c
cv fixing 7c9d172
Aug 01, 2011
adjusted to latest jackalope api 62c5c44
Aug 03, 2011
some changes according to the comments of beberlei 525dee2
Aug 04, 2011
Merge remote branch 'doctrine/master' into references
Conflicts:
	lib/Doctrine/ODM/PHPCR/Configuration.php
bced468
adjusted to latest jackalope api using factory ae612ce
Aug 05, 2011
more tests, issue with deleting one out of many references fixed 52dc0fc
Aug 08, 2011
some more test and bug fixes db12f15
Aug 15, 2011
bugfix, load referenced documents only once if you access them by the…
…ir id and later by their reference
a044537
Aug 16, 2011
trailing whitespaces removed ;-) 289fa0c
Aug 19, 2011
References with using proxy objects fa1b77c
typo fixed, TODO added 331ad4b
get referenced nodes within one call to increase performance 898727b
cv fixing 14a194d
adjusted to latest jackalope api 9770c25
some changes according to the comments of beberlei f43f6d1
adjusted to latest jackalope api using factory b59833f
more tests, issue with deleting one out of many references fixed 61919d2
some more test and bug fixes 64cd97d
bugfix, load referenced documents only once if you access them by the…
…ir id and later by their reference
0d4f2d4
trailing whitespaces removed ;-) 3eae731
Merge remote branch 'origin/references' into references fa319be
removing comma again b0031c1
Aug 23, 2011
Merge remote branch 'doctrine/master' into references a8b5761
Something went wrong with that request. Please try again.