neos / neos-development-collection

The unified repository containing the Neos core packages, used for Neos development.
https://www.neos.io/
GNU General Public License v3.0
260 stars 218 forks source link

!!! TASK: Streamline `ContentGraph::findRootNodeAggregateByType` #5158

Open mhsdesign opened 1 week ago

mhsdesign commented 1 week ago

to be nullable like findRootNodeByType in the subgraph

Upgrade instructions

Review instructions

I wondered why is findRootNodeByType in the subgraph nullable while findRootNodeAggregateByType in the content graph is not nullable and throws an RootNodeAggregateDoesNotExist exception? 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.

  1. findRootNodeAggregateByType was nullable introduced via https://github.com/neos/neos-development-collection/commit/1f4b464ec6e5c515ae94c240205fbcced077a907#diff-a623327bb4acbff32c1cbab65ef64b94e682baddd7505e56ca5f08b08e80ecc2

  2. nullability was removed via https://github.com/neos/neos-development-collection/commit/8ab8a1b4a794cab412a9b631c1ea3920db95527f#diff-a623327bb4acbff32c1cbab65ef64b94e682baddd7505e56ca5f08b08e80ecc2

last refactoring https://github.com/neos/neos-development-collection/pull/4339 and https://github.com/neos/neos-development-collection/pull/4129

Checklist

kitsunet commented 1 day ago

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.

mhsdesign commented 1 day ago

jip my argument would be that there is no find method that throws if you try to find something that doesnt exist (because of bad user input).

The only thing that throws is getContentGraph but we discussed that we liked it there.

Handling nullability yourself is also way easier than knowing which exceptions to catch and which not... phpstan will help you. Im really glad that findParent etc are now nullable (unlike once proposed in the traversableNodeInterface) and this should probably be aligned to behave the same. The question is, why was it nullable once and that has been removed??!

kitsunet commented 1 day ago

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!