Conversation
to be nullable like findRootNodeByType in the subgraph
|
Mmm, I guess there are arguments in both directions, I generally like it as is, with the Exception, but it makes sense to have things similar between the subgraph and the content graph. |
|
jip my argument would be that there is no The only thing that throws is Handling nullability yourself is also way easier than knowing which exceptions to catch and which not... phpstan will help you. Im really glad that |
|
Good question yes, I guess in some cases were the error case is fine you can skip an additional null check, but fine for me to merge it, we need ot adapt the use cases as well though! |
| NodeTypeNameFactory::forSites() | ||
| ); | ||
| if (!$sitesNodeAggregate) { | ||
| // nothing to prune, we could probably also return here directly? |
There was a problem hiding this comment.
That would be the current behavior with the exception I guess? Or rather we might actually throw here to tell people that this site does not exist?
There was a problem hiding this comment.
yes we throw currently. But as we want to remove a child node either way it seems fine to say, well we cant remove a thing here.
There was a problem hiding this comment.
I think, skipping is the better behavior.
But maybe we could throw at the end of this method if no node was actually removed?
There was a problem hiding this comment.
I dont think so. But with #4470 we can definitely discuss how the full behaviour should be and adjust edge cases.
bwaidelich
left a comment
bwaidelich
left a comment
There was a problem hiding this comment.
The question is, why was it nullable once and that has been removed??!
It was never nullable, it was introduced with:
1f4b464#diff-a623327bb4acbff32c1cbab65ef64b94e682baddd7505e56ca5f08b08e80ecc2
But it replaced the previous findRootNodeByType that was nullable.
Mmm, I guess there are arguments in both directions
I'm also torn sometimes between exception and nullable, since the latter is slightly less explicit.
But for these cases it totally makes sense IMO (as you can tell from the simplified resulting code) and also we had that soft guideline that find* returns nullable types and get* throws.
Just some minor remarks that might be ignored
| NodeTypeNameFactory::forSites() | ||
| ); | ||
| if (!$sitesNodeAggregate) { | ||
| // nothing to prune, we could probably also return here directly? |
There was a problem hiding this comment.
I think, skipping is the better behavior.
But maybe we could throw at the end of this method if no node was actually removed?
| $rootNode = $subgraph->findRootNodeByType($sitesNodeTypeName); | ||
| if ($rootNode === null) { | ||
| throw RootNodeAggregateDoesNotExist::butWasExpectedTo($sitesNodeTypeName); | ||
| throw new \RuntimeException(sprintf('No sites root node found in content repository "%s"', $contentRepository->id->value), 1719047148); |
There was a problem hiding this comment.
Just wondering: Why don't we keep the RootNodeAggregateDoesNotExist exception and throw it here? (not sure if it's needed but I'm curious about your reasons to replace it)
There was a problem hiding this comment.
we are in a different package here (the timeable stuff) and its imo not needed here to have a special exception class. Its a not ever gonna happen case and just a save guard.
…GraphFindRootNodeAggregateByType
nezaniel
left a comment
nezaniel
left a comment
There was a problem hiding this comment.
I prefer having find*ers nullable consistently, so +1 from me.
In this case especially, getOrCreateRootNodeAggregate (see above) gets a lot more comprehensible.
Tests should again be green as well.
mhsdesign
deleted the
task/streamLineContentGraphFindRootNodeAggregateByType
branch
4 participants
to be nullable like findRootNodeByType in the subgraph
Upgrade instructions
Review instructions
I wondered why is
findRootNodeByTypein the subgraph nullable whilefindRootNodeAggregateByTypein the content graph is not nullable and throws anRootNodeAggregateDoesNotExistexception? That seems the only place where we throw deliberately exceptions in the read side if a user of the cr put wrong stuff in the query.findRootNodeAggregateByTypewas nullable introduced via 1f4b464#diff-a623327bb4acbff32c1cbab65ef64b94e682baddd7505e56ca5f08b08e80ecc2nullability was removed via 8ab8a1b#diff-a623327bb4acbff32c1cbab65ef64b94e682baddd7505e56ca5f08b08e80ecc2
last refactoring #4339 and #4129
Checklist
FEATURE|TASK|BUGFIX!!!and have upgrade-instructions