references by uuid #734

ElectricMaxxx · 2017-01-17T21:38:09Z

This should wrap up #493

didn't implement the stuff, commented in code not to change.

But there are still the Questions of @dbu

I go top to button through the text/conversation, cause it is really hard to remember what the purpose was, even for me :-)

dantleech

Can you summarize the motivation behind this PR in the description? I don't really know the full implications of this change.

dantleech · 2017-01-17T21:50:10Z

composer.json

@@ -21,7 +21,8 @@
        "phpcr/phpcr-implementation": "^2.1.0",
        "phpcr/phpcr-utils": "^1.2.8",
        "doctrine/instantiator": "^1.0.1",
-        "symfony/console": "^2.3|^3.0"
+        "symfony/console": "^2.3|^3.0",
+        "jackalope/jackalope-doctrine-dbal": "~1.0"


Don't think this should be here...

dantleech · 2017-01-17T21:53:53Z

lib/Doctrine/ODM/PHPCR/UnitOfWork.php

-        $node = $this->session->getNode($this->determineDocumentId($document));
+        $pathOrUuid = $this->determineDocumentId($document);
+        $node = $this->getNodeByPathOrUuid($pathOrUuid);
+        if (UUIDHelper::isUUID($pathOrUuid)) {


Empty line before if statement

dantleech · 2017-01-17T21:59:40Z

lib/Doctrine/ODM/PHPCR/UnitOfWork.php

-        $frozen = $this->session->nodeExists($id) && $this->session->getNode($id)->isNodeType('nt:frozenNode');
+        try {
+            $node = $this->getNodeByPathOrUuid($pathOrUuid);
+            $frozen = $node instanceof  NodeInterface ? $node->isNodeType('nt:frozenNode') : false;


If the node is not an instance of NodeInterface then what is it? If we are expecting null then I think we should check forr null.

dantleech · 2017-01-17T22:05:32Z

lib/Doctrine/ODM/PHPCR/DocumentManager.php

@@ -917,13 +917,11 @@ public function initializeObject($document)
     */
    public function getNodeForDocument($document)
    {
-        if (!is_object($document)) {


Why do we remove this validation?

dantleech · 2017-01-17T22:05:54Z

lib/Doctrine/ODM/PHPCR/DocumentManager.php

@@ -917,13 +917,11 @@ public function initializeObject($document)
     */
    public function getNodeForDocument($document)
    {
-        if (!is_object($document)) {
-            throw new InvalidArgumentException('Parameter $document needs to be an object, '.gettype($document).' given');
+        if (!$identifier = $this->unitOfWork->getDocumentId($document)) {


if (null === $identifier = $this->...

dantleech · 2017-01-17T22:08:00Z

lib/Doctrine/ODM/PHPCR/UnitOfWork.php

@@ -674,7 +675,7 @@ public function getOrCreateProxy($targetId, $className, $locale = null)
        $metadata      = $this->dm->getClassMetadata($className);
        $proxyDocument = $this->dm->getProxyFactory()->getProxy($className, array($metadata->identifier => $targetId));

-        // register the document under its own id
+        // register the document under its own id/uuid


path/uuid.

dantleech · 2017-01-17T22:12:35Z

lib/Doctrine/ODM/PHPCR/UnitOfWork.php

@@ -3113,6 +3126,20 @@ public function getDocumentById($id)
    }

    /**
+     * Creates a node from a given path or an uuid


given path or UUID

dantleech · 2017-01-17T22:16:13Z

lib/Doctrine/ODM/PHPCR/UnitOfWork.php

+        } catch (ItemNotFoundException $e) {
+            $frozen = false;
+        } catch (PathNotFoundException $e) {
+            $frozen = false;


$frozen = false can be moved before the try block (doesn't need to be repeated twice)

dantleech · 2017-01-17T22:18:04Z

tests/Doctrine/Tests/ODM/PHPCR/UnitOfWorkTest.php

        $nodeData = array(
            "jcr:primaryType" => $primaryType,
            "jcr:system" => array(),
            'username' => $username,
        );
+        if (null !== $uuid) {


I prefer a blank line before most if statements.

dantleech · 2017-01-17T22:18:30Z

tests/Doctrine/Tests/ODM/PHPCR/UnitOfWorkTest.php

+                ->will($this->returnValue($node));
+        }
+
+


One blank line too many

ElectricMaxxx · 2017-01-17T23:09:38Z

This will fail for sure now ... Did a lot of changes

introduced a object hash - uuid map, which is filled when a document is registered by its uuid (on DocumentManager::getReference() call with proxies referencing documents by their uuid <- @dantleech purpose)
introduced a uuid - path map to find documents by their path in identity map, which are managed by uuid

so all documents still live in identityMap by their id, but the id can be the absolute paths directly or uuids which needs to be converted into paths (uuid - id map).

dantleech

Still not sure what the motivation of this is? Regarding the mapping I wonder if it is worth introducing a Registry class to encapsulate the various maps, reducing complication in this already very complicated class, something like: https://github.com/sulu/sulu-document-manager/blob/master/lib/DocumentRegistry.php.

But maybe something for a later iteration.

dantleech · 2017-01-18T20:00:37Z

lib/Doctrine/ODM/PHPCR/UnitOfWork.php

@@ -100,6 +101,18 @@ class UnitOfWork
    private $documentIds = array();

    /**
+     * @var array
+     */
+    private $documentUuids = array();


Ideally this would be better named $oidToUuidMap, but to be consistent this naming is fine for now I guess. But it should be commented.

dantleech · 2017-01-18T20:01:33Z

lib/Doctrine/ODM/PHPCR/UnitOfWork.php

+     *
+     * @var array
+     */
+    private $uuidIdMapping = array();


Id in uuidId is redundant. Better to name $uuidToPathMap.

dantleech · 2017-01-18T20:03:22Z

lib/Doctrine/ODM/PHPCR/UnitOfWork.php

@@ -3058,23 +3076,43 @@ private function unregisterDocument($document)
            $this->documentVersion[$oid]
        );

+        if (!empty($this->documentUuids[$oid]) && array_key_exists($this->documentUuids[$oid], $this->uuidIdMapping)) {


Why check for !empty?

use uuid build reference and register document by uuid

256f9af

dantleech reviewed Jan 17, 2017

View reviewed changes

try to introduce uuid -> path mapping with documents managed by uuid

b6853e7

ElectricMaxxx added 2 commits January 18, 2017 00:15

change method name

4b612d6

fix test driven node existence

461fee3

ElectricMaxxx changed the title ~~[WIP] references by uuid~~ references by uuid Jan 18, 2017

dantleech reviewed Jan 18, 2017

View reviewed changes

add one test to the crud tests

ea7923c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

references by uuid #734

references by uuid #734

ElectricMaxxx commented Jan 17, 2017

dantleech left a comment

dantleech Jan 17, 2017

dantleech Jan 17, 2017 •

edited

dantleech Jan 17, 2017

dantleech Jan 17, 2017

dantleech Jan 17, 2017

dantleech Jan 17, 2017

dantleech Jan 17, 2017

dantleech Jan 17, 2017

dantleech Jan 17, 2017

dantleech Jan 17, 2017

ElectricMaxxx commented Jan 17, 2017

dantleech left a comment •

edited

dantleech Jan 18, 2017

dantleech Jan 18, 2017

dantleech Jan 18, 2017

references by uuid #734

Are you sure you want to change the base?

references by uuid #734

Conversation

ElectricMaxxx commented Jan 17, 2017

dantleech left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dantleech Jan 17, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ElectricMaxxx commented Jan 17, 2017

dantleech left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dantleech Jan 17, 2017 •

edited

dantleech left a comment •

edited