FEATURE: Include Subtree Tags + Tests #5654
Conversation
robinroloff
changed the title
|
first draft as discussed @skurfuerst |
|
I will give feedback tonight (or merge into my branch :)) |
There was a problem hiding this comment.
Hey @robinroloff ,
very cool, thanks for your contribution :) I have added some notes to make this all easier to review and reason about.
Thanks and all the best, Sebastian
| public function findDescendantNodes(NodeAggregateId $entryNodeAggregateId, FindDescendantNodesFilter $filter): Nodes | ||
| { | ||
| ['queryBuilderInitial' => $queryBuilderInitial, 'queryBuilderRecursive' => $queryBuilderRecursive, 'queryBuilderCte' => $queryBuilderCte] = $this->buildDescendantNodesQueries($entryNodeAggregateId, $filter); | ||
| [ |
There was a problem hiding this comment.
can you please revert the auto-format? in this file? It's very hard to find out the actual change :)
| $queryBuilder->setParameter('tagPath' . $i, '$."' . $includedTag->value . '"'); | ||
| $i++; | ||
| } | ||
| $where = '(' . implode(' OR ', $includedParts) . ')'; |
There was a problem hiding this comment.
hm, unsure if we should rather use the Nested query Builders using the expr() API here :) How are complex queries in this file otherwise constructed? (I'd keep it similar)
There was a problem hiding this comment.
Okay, changed that, I'm not sure if I did it right though. Kinda feels more hacky than before.
| Background: | ||
| Given using the following content dimensions: | ||
| | Identifier | Values | Generalizations | | ||
| | language | mul, de, en, ch | ch->de->mul, en->mul | |
There was a problem hiding this comment.
Given no content dimensions (or the like) -> makes the test easier
There was a problem hiding this comment.
Yes, to be honest I just copied the background from another feature. I will remove all the unnecessary stuff.
| Given using the following content dimensions: | ||
| | Identifier | Values | Generalizations | | ||
| | language | mul, de, en, ch | ch->de->mul, en->mul | | ||
| And using the following node types: |
There was a problem hiding this comment.
can we come up with a minmal example? :) i.e. remove all node types you do not need to make the test easier to read
| | a2b | a2b | Neos.ContentRepository.Testing:Page | a2 | {} | {} | | ||
| | a2b1 | a2b1 | Neos.ContentRepository.Testing:Page | a2b | {} | {} | | ||
| | b | b | Neos.ContentRepository.Testing:Page | home | {} | {} | | ||
| And the command DisableNodeAggregate is executed with payload: |
There was a problem hiding this comment.
do we need disable + move? I think not :)
| * @param SubtreeTags $excluded A set of {@see SubtreeTag} instances that will be _excluded_ from the results of any content graph query | ||
| * @param SubtreeTags $included A set of {@see SubtreeTag} instances that will be _included_ in the results of any content graph query | ||
| */ | ||
| public static function excludeAndIncludeSubtreeTags(SubtreeTags $excluded, SubtreeTags $included): self |
There was a problem hiding this comment.
I am still very unsure about the public API here. IMHO we need a better wording for "include and exclude is both set" -> something which makes really clear that "included" comes first and excluded always win. Maybe something like "base subtree tags" and "excluded".
-> we can first merge it now, and later rename (before we merge the upstream PR)
There was a problem hiding this comment.
Me too, I wasn't sure either, this is the main reason why this PR is still a draft.
My suggestion would be to make the VisibilityConstraints a builder pattern, but as I didn't find them anywhere else in the code base, I wasn't sure what your standard for that is.
However, I feel like it would make a lot of sense for easy usage and for future extensibility.
|
FYI (for anybody stumbling across this PR here): NOT merging into Neos 9, but into our dev branch! |
2 participants
This PR adds the option to filter for included subtree tags. It also adds the first tests for filtering custom subtree tags.
Checklist
FEATURE|TASK|BUGFIX!!!and have upgrade-instructions