references by uuid#734
Conversation
dantleech
left a comment
dantleech
left a comment
There was a problem hiding this comment.
Can you summarize the motivation behind this PR in the description? I don't really know the full implications of this change.
| "doctrine/instantiator": "^1.0.1", | ||
| "symfony/console": "^2.3|^3.0" | ||
| "symfony/console": "^2.3|^3.0", | ||
| "jackalope/jackalope-doctrine-dbal": "~1.0" |
There was a problem hiding this comment.
Don't think this should be here...
| $node = $this->session->getNode($this->determineDocumentId($document)); | ||
| $pathOrUuid = $this->determineDocumentId($document); | ||
| $node = $this->getNodeByPathOrUuid($pathOrUuid); | ||
| if (UUIDHelper::isUUID($pathOrUuid)) { |
There was a problem hiding this comment.
Empty line before if statement
| $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; |
There was a problem hiding this comment.
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.
| */ | ||
| public function getNodeForDocument($document) | ||
| { | ||
| if (!is_object($document)) { |
There was a problem hiding this comment.
Why do we remove this validation?
| { | ||
| if (!is_object($document)) { | ||
| throw new InvalidArgumentException('Parameter $document needs to be an object, '.gettype($document).' given'); | ||
| if (!$identifier = $this->unitOfWork->getDocumentId($document)) { |
There was a problem hiding this comment.
if (null === $identifier = $this->...
| $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 |
| return false; | ||
| } | ||
|
|
||
| /** |
| } catch (ItemNotFoundException $e) { | ||
| $frozen = false; | ||
| } catch (PathNotFoundException $e) { | ||
| $frozen = false; |
There was a problem hiding this comment.
$frozen = false can be moved before the try block (doesn't need to be repeated twice)
| "jcr:system" => array(), | ||
| 'username' => $username, | ||
| ); | ||
| if (null !== $uuid) { |
There was a problem hiding this comment.
I prefer a blank line before most if statements.
| ->will($this->returnValue($node)); | ||
| } | ||
|
|
||
|
|
|
This will fail for sure now ... Did a lot of changes
so all documents still live in |
There was a problem hiding this comment.
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.
|
|
||
| /** | ||
| * @var array | ||
| */ |
There was a problem hiding this comment.
Ideally this would be better named $oidToUuidMap, but to be consistent this naming is fine for now I guess. But it should be commented.
| * Mapping to find ids (abs. path) of document managed by its uuid. | ||
| * | ||
| * @var array | ||
| */ |
There was a problem hiding this comment.
Id in uuidId is redundant. Better to name $uuidToPathMap.
| $this->documentVersion[$oid] | ||
| ); | ||
|
|
||
| if (!empty($this->documentUuids[$oid]) && array_key_exists($this->documentUuids[$oid], $this->uuidIdMapping)) { |
2 participants
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 :-)