Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

9.0 Should a direct or indirect NodeType constraint allowing the node creation win? #4351

Closed
mhsdesign opened this issue Jun 21, 2023 · 0 comments · Fixed by #4361
Closed

Comments

@mhsdesign
Copy link
Member

mhsdesign commented Jun 21, 2023

Given you have this NodeType definition

'Flowpack.NodeTemplates:Content.RestrictedContentCollection':
  superTypes:
    'Neos.Neos:ContentCollection': true
  constraints:
    nodeTypes:
      '*': false
      # declare some things that you want to be allowed ...

#
# or directly overriding this option in `Neos.Neos:ContentCollection` and use `Neos.Neos:ContentCollection` below
#
# 'Neos.Neos:ContentCollection:
#  constraints:
#    nodeTypes:
#      '*': false


'Flowpack.NodeTemplates:Content.ConstrainedChildNodes':
  superTypes:
    'Neos.Neos:Content': true
  childNodes:
    content:
      type: 'Flowpack.NodeTemplates:Content.RestrictedContentCollection'
      constraints:
        nodeTypes:
          # additionally allow this nt
          'Flowpack.NodeTemplates:Content.Text': true

now when you want to create the following node structure (either by the ui or the correct commands)
(Basically we want to insert a Text node with a text into the content collection)

{
    "nodeTypeName": "Flowpack.NodeTemplates:Content.ConstrainedChildNodes",
    "childNodes": [
        {
            "nodeTypeName": "Flowpack.NodeTemplates:Content.RestrictedContentCollection",
            "nodeName": "content",
            "childNodes": [
                {
                    "nodeTypeName": "Flowpack.NodeTemplates:Content.Text",
                    "properties": {
                        "text": "Text"
                    }
                }
            ]
        }
    ]
}

you will find out that this succeeds in the "old" CR Neos 8 but will throw a NodeConstraintException in Neos 9:

Neos\ContentRepository\Core\SharedModel\Exception\NodeConstraintException: Node type "Flowpack.NodeTemplates:Content.Text" is not allowed for child nodes of type Flowpack.NodeTemplates:Content.RestrictedContentCollection

on Node creation the constraint check in 8.3 looks like this:

/**
* Checks if the given $nodeType would be allowed as a child node of this node according to the configured constraints.
*
* @param NodeType $nodeType
* @return boolean true if the passed $nodeType is allowed as child node
* @throws NodeTypeNotFoundException
*/
public function isNodeTypeAllowedAsChildNode(NodeType $nodeType): bool
{
if ($this->isAutoCreated()) {
return $this->getParent()->getNodeType()->allowsGrandchildNodeType($this->getName(), $nodeType);
} else {
return $this->getNodeType()->allowsChildNodeType($nodeType);
}
}

in Neos 9 we assert in requireConstraintsImposedByAncestorsAreMet

first that requireNodeTypeConstraintsImposedByParentToBeMet AND the requireNodeTypeConstraintsImposedByGrandparentToBeMet

Discussion

I want to discuss, which behavior is correct.

@nezaniel and me already started the conversation in slack:

Bernhard
Maybe that's kind of a breaking change I guess; Direct AND indirect constraints are checked independently, you can only get stricter

Me
Was this discussed somewhere with a larger group? Because I’m not really sure if this is the right decision. I personally don’t have an issue with it in a project atm, but it’s hard to explain to our users that the NodeTypes now function differently. (I was expecting them to be a constant factor)
See $someone using it in Flowpack/Flowpack.NodeTemplates#66 (comment)
(I initially noticed this change in behavior because of the above issue. And the above issue was reported, because i to tried to build NodeTemplates v2 for Neos8.3 100% with the same behavior as for Neos9, so i copied the constraint checks from Neos 9 - i evaluate constraints beforehand to avoid any errors at command handling time / with the real soft constraints).

Bernhard
IIRC the whole concept of grandchild constraints itself was questioned because you could replace it with custom collections and the validation is painfully.
For convenience we decided on the current state, with the idea of everything being a default content collection with explicit constraints and something like a constrained content collection not existing.
That being said, I think your case could be supported, we just have to be very careful because it changes a core invariant

mhsdesign added a commit that referenced this issue Jun 23, 2023
@mhsdesign mhsdesign moved this to In Progress 🚧 in Neos 9.0 Release Board Jul 10, 2023
@mhsdesign mhsdesign moved this from In Progress 🚧 to Under Review 👀 in Neos 9.0 Release Board Sep 1, 2023
@mhsdesign mhsdesign moved this from Under Review 👀 to In Progress 🚧 in Neos 9.0 Release Board Sep 19, 2023
mhsdesign added a commit that referenced this issue Feb 1, 2024
@mhsdesign mhsdesign moved this from In Progress 🚧 to Under Review 👀 in Neos 9.0 Release Board Feb 1, 2024
@mhsdesign mhsdesign removed the 9.0beta2 label Feb 8, 2024
@github-project-automation github-project-automation bot moved this from Under Review 👀 to Done ✅ in Neos 9.0 Release Board Mar 11, 2024
mhsdesign added a commit to Flowpack/Flowpack.NodeTemplates that referenced this issue Jun 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

1 participant