allow to add uuid as id to proxies #493
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -313,28 +313,33 @@ public function getClassMetadata($className) | |
| */ | ||
| public function find($className, $id) | ||
| { | ||
| // document still mapped -> return that when class name is valid | ||
|
There was a problem hiding this comment. why "still"? i would move that inside the if and say "// document already mapped -> check class" |
||
| $document = $this->unitOfWork->getDocumentById($id); | ||
| if ($document) { | ||
| $document = $this->documentHasValidClassName($document, $className) ? $document : null; | ||
| return $document; | ||
|
There was a problem hiding this comment. cs: needs a blank line before the return statement. |
||
| } | ||
|
|
||
| // id can either be an path or an uuid, so we can have a look if one of both does have an entry | ||
| if (UUIDHelper::isUUID($id)) { | ||
| try { | ||
| $id = $this->session->getNodeByIdentifier($id)->getPath(); | ||
| } catch (ItemNotFoundException $e) { | ||
| return null; | ||
| } | ||
| } elseif (strpos($id, '/') !== 0) { | ||
| $id = '/'.$id; | ||
|
There was a problem hiding this comment. In what cases would the path not be absolute? Surely we want to throw an Exception here (or let the PHPCR\Session throw an exception for us) There was a problem hiding this comment. Same stuff as before, I just put the regquest for an existing document on top.
There was a problem hiding this comment. ah now i understand your question. I just keep that ExceptionManagement cause i don't understand it at all. There was a problem hiding this comment. @dantleech reading the above removed code, it seems we already did the same before. not sure why but i would not change that behaviour in this refactoring. it should not break any BC. |
||
| } | ||
|
|
||
| // document mapped by id = absolute Path -> return that when class name is valid | ||
| $document = $this->unitOfWork->getDocumentById($id); | ||
| if ($document) { | ||
| $document = $this->documentHasValidClassName($document, $className) ? $document : null; | ||
| return $document; | ||
| } | ||
|
|
||
| // no document mapped, then try to fetch it from session and create a new one | ||
| try { | ||
| $node = $this->session->getNode($id); | ||
| } catch (PathNotFoundException $e) { | ||
| return null; | ||
|
|
@@ -349,6 +354,25 @@ public function find($className, $id) | |
| } | ||
| } | ||
|
|
||
| /** | ||
| * Just a little helper to validate the documents class name with a | ||
| * boolean answer. | ||
| * | ||
| * @param object $document | ||
| * @param string $className | ||
| * @return bool | ||
| */ | ||
| private function documentHasValidClassName($document, $className) | ||
|
There was a problem hiding this comment. i think this is not the right way to solve things. i would take out the actual logic form validateClassName into isValidClassName (in the UoW) that just returns a boolean. then you can call that directly. and valdiateClassName just calls There was a problem hiding this comment. 👍 i just didn't like that long try-catch sections. There was a problem hiding this comment. i totally agree with you, just think the solution should be to not even There was a problem hiding this comment. Here i recognized a problem, some tests and some situations are waiting for some exceptions and i have got no clue when catch an exception and just return null (or some equal) or doesn't catch it. So the |
||
| { | ||
| try { | ||
| $this->unitOfWork->validateClassName($document, $className); | ||
|
|
||
| return true; | ||
| } catch(ClassMismatchException $e) { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Finds many documents by id. | ||
| * | ||
|
|
@@ -1190,23 +1214,40 @@ public function initializeObject($document) | |
| } | ||
|
|
||
| /** | ||
| * Return the node of the given object or its hash. | ||
| * | ||
| * This node is fetched by the objects uuid or its id means the absolute path. | ||
|
|
||
| * | ||
| * @param object|string $document | ||
| * | ||
| * @return \PHPCR\NodeInterface | ||
| * | ||
| * @throws InvalidArgumentException if $document isn't mapped in UoW. | ||
| * @throws PHPCRException if $document is not managed | ||
| */ | ||
| public function getNodeForDocument($document) | ||
| { | ||
| if (!$identifier = $this->unitOfWork->getDocumentId($document)) { | ||
| throw new InvalidArgumentException('Parameter document should have an entry in identityMap.'); | ||
|
There was a problem hiding this comment. Lets say: "This document is not managed by this manager" or something like this. |
||
| } | ||
|
|
||
| return $this->getNodeByPathOrUuid($identifier); | ||
| } | ||
|
|
||
| /** | ||
| * Creates a node from a given path or an uuid | ||
| * | ||
| * @param $pathOrUuid | ||
| * @return \PHPCR\NodeInterface | ||
| */ | ||
| public function getNodeByPathOrUuid($pathOrUuid) | ||
| { | ||
| if (UUIDHelper::isUUID($pathOrUuid)) { | ||
| $node = $this->session->getNodeByIdentifier($pathOrUuid); | ||
| } else { | ||
| $node = $this->session->getNode($pathOrUuid); | ||
|
There was a problem hiding this comment. i think this is wrong. its moving session dependency into the document manager, and duplicating getDocumentId. and it will only work if the document was already flushed, but not for new managed documents, as UoW::getDocumentId. we should move the logic to handle uuid into getDocumentId and make the uow look in the right places. There was a problem hiding this comment. You mean put that code into the getDocumentId(), ok would make sense. |
||
| } | ||
|
|
||
| return $node; | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm, the refacotring in this method is making things messier without really helping much. i would shove the whole code you have here below into a method cleanId($id), to prepare for further refactoring. but keep the order of things as before.