!!! TASK: Node name consistency adjustments #5018
Conversation
...to enforce uniqueness of names on aggregate level
... and prevent manual, erroneous structure adjustments
mhsdesign
changed the title
|
Is this ready for review? |
|
I'd like #5025 merged first, afterwards, yes, I think so |
|
now open for review |
nezaniel
requested review from
mhsdesign,
kitsunet,
skurfuerst,
bwaidelich,
ahaeslich and
grebaldi
|
Oh no. This turns out to be in MAJOR conflict with @kitsunet month old #5028 pr
As there are also refactorings made regarding query building and stuff im not sure how to continue. Maybe even merge this one first as this contains real sql changes while the other one cosmetic ones and THESE changes must not get lost.... |
Neos.ContentGraph.DoctrineDbalAdapter/src/Domain/Projection/NodeRecord.php
Show resolved
Hide resolved
|
Wasn't that bad after all. I'd like #5040 merged first, though, to resolve the expected additional conflicts here |
There was a problem hiding this comment.
Looks good already ;) Thanks for including the adjustments!
The Neos Ui e2e also pass without any adjustments
(as we want to release a new beta soon today tomorrow? i would not want to include it into that because of the replay, but it can be merged the second the beta was released)
Neos.ContentGraph.DoctrineDbalAdapter/src/DoctrineDbalContentGraphSchemaBuilder.php
Show resolved
Hide resolved
| if ( | ||
| $nodeName | ||
| && $parentsNodeType->hasTetheredNode($nodeName) | ||
| && !$this->getNodeTypeManager()->getTypeOfTetheredNode($parentsNodeType, $nodeName)->name->equals($nodeType->name) | ||
| ) { | ||
| throw new NodeConstraintException( | ||
| 'Node type "' . $nodeType->name->value . '" does not match configured "' | ||
| . $this->getNodeTypeManager()->getTypeOfTetheredNode($parentsNodeType, $nodeName)->name->value | ||
| . '" for auto created child nodes for parent type "' . $parentsNodeType->name->value | ||
| . '" with name "' . $nodeName->value . '"', | ||
| 1707561404 | ||
| ); | ||
| } |
There was a problem hiding this comment.
why were these constraint checks removed? I came across those many times and last time i read them they seemed correct?
There was a problem hiding this comment.
They were replaced by an even stricter requireNodeTypeNotToDeclareTetheredChildNodeName (see above)
There was a problem hiding this comment.
Okay i see but my feeling says requireNodeTypeNotToDeclareTetheredChildNodeName is not called often enough? Only once in the Move handling?
There was a problem hiding this comment.
It's called three times, at all relevant locations:
- NodeCreation: You can't manually create a missing tethered node anymore
- NodeMove: You can't move a node to a parent where a tethered child of the same name is missing
- NodeRenamig: You can't assign the name of a missing tethered sibling to a node anymore
There was a problem hiding this comment.
Wait but what about handleCopyNodesRecursively and handleChangeNodeAggregateType? they had a call previously to requireConstraintsImposedByAncestorsAreMet but not to requireNodeTypeNotToDeclareTetheredChildNodeName?
There was a problem hiding this comment.
Good point, I'll have to check what copy does with names.
Type change is unrelated though as it does not affect the name
Neos.ContentRepository.Core/Classes/Feature/Common/TetheredNodeInternals.php
Show resolved
Hide resolved
mhsdesign
changed the title
Is there any particular reason why the replay should be in the next beta instead of this one? |
|
idk but it seems no replay for this beta is not possible see: #5025 (comment) was just thinking about keeping things simple when beta hopping and putting all migrate related stuff in one batch but youre rm as well so was just a slight thought |
|
As this has to be merged (rather sooner than later) anyway, I'd say we can do so already |
draft until #4993 is mergedbuilds on #4993
resolves #4150
This improves node name handling consistency by adjusting behavior in certain locations.
Unique node names for children on aggregate level
Child node aggregates must no longer share a name, even if they don't share covered dimension space points.
This also means that
ContentGraphInterface::findChildNodeAggregateByNamenow returns a single NodeAggregate, if anyEnforcement and respective tests have been adjusted
Node names are now stored in the contentgraph's node table
Changing the node name now trigger a copy on write
Manual structure adjustments regarding tethered nodes are now prohibited
Previously, it was possible to manually "fix" missing tethered nodes by renaming a node of the proper type or change the type of a node with the proper name. This is now no longer possible and instead to be performed globally via structure adjustments
Upgrade instructions
The method
ContentGraphInterface::findChildNodeAggregateByNamenow returns a single NodeAggregate, if any and has been renamed:./flow cr:setupand./flow cr:replay contentgraphis required due to node names now being stored in the node instead of hierarchy relation tableReview instructions
coming soon
Checklist
FEATURE|TASK|BUGFIX!!!and have upgrade-instructions