BUGFIX: Allow auto-created child nodes to be hidden

[suffle]

As a continuation to #2282 and https://neos-project.slack.com/archives/C050C8FEK/p1633943023444000?thread_ts=1633942822.443900&cid=C050C8FEK, there should be a possibility to activate visibility toggles for auto created child nodes, although hiding them is forbidden by default.

I introduced a new option for the parent element allowHideAutoCreatedChildNodes. This option is taken into account in the NodeInfoHelper in a new property disableChangeVisibility. This new property also takes the policy of the node property _hidden into account to prevent further mismatches between Inspector and NodeTree, which triggered the initial Bug.

By questioning this property from the UI it is again possible to hide auto created child nodes by adding

options.allowHideAutoCreatedChildNodes: true

to the Node creating these children (aka the parent node).

Edit:

It was suggested to rather configure the behaviour via parent node:

'Some.Package:Some.NodeType':
  childNodes:
    'childnode1':
      type: 'Some.Package:Some.NodeType'
      hideable: true

Resolves: #2282

[bwaidelich]

Thanks, I think this is a great approach!
I'm just wondering: From the description and code I cannot tell directly where exactly to set this option, i.e. is this configurable per node type – or, would we need a way to decide that per child node even, e.g.:

'Some.Package:Some.NodeType':
  childNodes:
    'childnode1':
      type: 'Some.Package:Some.NodeType'
      hideable: true
    'childnode2':
      type: 'Some.Package:Some.NodeType'

?

[suffle]

You are right, i did not give a very good example. I was not sure about the performance impact if we have to traverse the whole node type configuration, although I am not sure if it has any. Either way I think the benefits of a configuration per child node overweigh this. I changed it to work exactly as you suggested:

'Some.Package:Some.NodeType':
  childNodes:
    'childnode1':
      type: 'Some.Package:Some.NodeType'
      hideable: true

[mhsdesign]

above some code ideas...

and maybe visibilityCanBeToggled (inverted value) is a more descriptive name than: disableChangeVisibility

[mhsdesign]

i just looked up the Nodeinterface https://github.com/neos/neos-development-collection/blob/29971ce95314bbcebcafea255a11dc751c86b29a/Neos.ContentRepository/Classes/Domain/Model/NodeInterface.php and these methods seem to be depreceated but used everywhere still - but maybe a good idea to use the alternatives for new code...

[suffle]

above some code ideas...

and maybe visibilityCanBeToggled (inverted value) is a more descriptive name than: disableChangeVisibility

I added all the code ideas, and while I agree that visibilityCanBeToggled is more descriptive, I normally try to name these toggles to describe the non-default state. A node is hideable by default and disabling it, is the special behaviour. This also works, if the toggle is missing completely on the api side. But that should not stop the PR from going forward, so if that change is required, I will make it...

[lorenzulrich]

While investigating something that - according to the customer - broke with a bugfix release, I found this issue and realize that hiding auto-created child nodes was indeed broken (or fixed - depending on the point of view ;-)) in a bugfix release. Are there any plans to merge this change? Backporting UI changes is a little less funny than backporting only PHP changes.

[kitsunet]

I have strong reservations about this, as it makes hiding childnodes suddenly a valid concept that we would have to carry into the future. My (personal) understanding is that childNodes are not to be optional. They belong to their parent and together they create a unit that should either exist or not exist as a whole.
To me there are two directions: One would be to document how to make that an explicit property of said child nodes and just prevent rendering in fusion then, which means the concept of hiding such nodes doesn't exist and it's merely an integration detail to prevent them from showing if some property was set.
Or second way: we devise a coherent core concept for this, that has a (different) name and implement that. An "associatedNode" for example? That is more loosely coupled...

[suffle]

I absolutely understand your reservations about this and I agree that childNodes should always exist together with their parent database-wise (which is why you want them automatically created) and I don't want the editor to be able to remove them. But for me, autocreated childnodes are also a way to restrict editors to not add infinite amounts of the same nodetype, a concept which Neos does not provide otherwise. I also think there is a difference between "being there" in editing and in the frontend. Yes, the child has to be there for the editor to use and it has to exist in the database, but the frontend does not necessarily need it if the developer knows it might not be there, which is why you have to explicitly allow hiding it.

And I would be absolutely fine with hiding the content in fusion if it was solely about page content. The problem is, as soon as I use the concept on document-nodes, just preventing rendering of this one node is not enough. I also need to prevent rendering in menus, add my own error handling or change the FrontendNodeRoutePartHandler accordingly while also making sure to not impact the editing experience. All this is already solved with the concept of hiding a (document) node. And for me preventing a child node from being hidden was not a simple bugfix, we heavily relied on that feature.
The use case of hiding a child that is not ready for publishing yet, while the parent (and other elements) are already finished might contradict your view of a child node, which I totally get, and I don't have a problem with adding a new concept that restricts editors to a given amount of one NodeType. Your idea of an "associatedNode" also sounds like a great solution, but I don't think that will happen soon and it can be realized already with the solution provided here.

[bwaidelich]

Allow my to repeat my POV from last October:

Yes, editors should not be able to simply delete/disable nodes at their will.
But IMO that's the responsibility of the integrator..
Already now you can accidentally "destroy" a whole website if you hide the homepage in case those options weren't disabled explicitly (like it is the case for neos.io btw).
I would suggest to change the default with Neos 8.0 to hide the visibility properties unless the integrator choses to explicitly use some mixin.
And (for the same reason why a homepage is also expected to be there) they should be free to use that mixin for auto created child nodes too

I still think this is true.

Regarding @kitsunet s alternative suggestions:

document how to make that an explicit property of said child nodes and just prevent rendering in fusion then

Four our cases that wouldn't work as @suffle wrote

we devise a coherent core concept for this, that has a (different) name and implement that. An "associatedNode" for example?

Having "auto-created child nodes", to become "tethered nodes" and then another concept called "associated node" would be utterly confusing IMO.

Our case we could probably also solve with NodeTemplates and a relatively small addition to node constraints that allows the number of child nodes of a certain type to be specified, maybe like:

'Some.Package:Some.NodeType':
  options:
    template:
      childNodes:
       'fixed':
          type: 'Some.Package:Some.NodeType'
          properties:
            title: 'xyz'
  constraints:
    nodeTypes:
      'Neos.Neos:Document': false
      'Some.Package:Some.NodeType':
        min: 1
        max: 1

maybe that's a direction to follow?

[bwaidelich]

TODO: We should check whether this is a performance issue

[kitsunet]

I no longer oppose this, a discussion has been had and I find the reasoning good, and can see the benefit of having this option.

[markusguenther]

Rebased the change and resolved the merge conflicts. As the issue has been there for a long time, and it is a bug fix, why we target 8.2 and not 7.3?

[mhsdesign]

@markusguenther i still have reservations against this.

Also if we were to allow this i would rather consider this a feature which should land in 8.4

The reason i am against this is that no fusion integration will handle this gracefully.

Testing it in the neos ui will result in fatal errors in the frontend rendering:

When hiding the main content collection: (which already sounds baaaaad)

No content collection of type Neos.Neos:ContentCollection could be found in the current node (/[99f81405-8781-4f3b-a6ba-8606a8be66d6]) or at the path "main"

When hiding a column in a two column element.

No content collection of type Neos.Neos:ContentCollection could be found in the current node (/[99f81405-8781-4f3b-a6ba-8606a8be66d6]/neosdemo/main) or at the path "column0".

Id argue this definitely defeats the purpose and guarantees we make with tethered nodes.

content = Neos.Neos:ContentCollection {
   nodePath = "main"
}

will become unsafe!

Also @nezaniel agrees with me that this is a "bug" or rather undefined behaviour and should be caught by constraint checks.

[mhsdesign]

we should probably use a non negated variable name like isHidable
(with inverted value) as a more descriptive name than: disableChangeVisibility.

[bwaidelich]

Also if we were to allow this i would rather consider this a feature which should land in 8.4

Just a reminder: This was possible before (and still is, just not through the default UI any longer) #2282
I agree that the suggested implementation is more of a feature than a bugfix though.

No content collection of type Neos.Neos:ContentCollection could be found in the current node

That is not thrown in production mode, is it?

will become unsafe!

Of course you could create invalid or rather unusual setups with this. Just like with node migrations, removed/changed node type definitions etc. But I wouldn't call it "unsafe" – instead I would argue that in Fusion land we should always expect nodes to be potentially missing.

Also: You have to explicitly allow this. And if a developer really wants to make a column of a multi-column node hidable.. well, we might as well just let them – they'll find a way in any case :)

However, I would be fine with only allowing this for document nodes

[nezaniel]

I'd rather introduce configurable constraints (like number of allowed children by type) instead of (ab)using the concept of tethered children for this purpose, but I'm also pretty late to the party and am not entirely sure which way we decided for the ESCR as there are no tests covering this

[bwaidelich]

Allow me to outline one of the use cases we came across once again:
We want to allow the editors to add new "competition"-pages. Those have a common structure with sub-pages (aka tethered document nodes) for rules, forms, legal stuff, ...
One of the pre-defined sub-pages is the "winner" of the competition (which should always be of a certain type and has the URL /.../<competition-id>/winner).
Being able to create that page already and allow the editors to enable it once the competition is over was a really easy way to achieve this and editors understand it very well (the page already exists, can be filled with content and is clearly marked as not visible to the public).

I'd rather introduce configurable constraints (like number of allowed children by type)

That would be a great solution too, but I think it will be quite hard to achieve (think about changed node types, migrations, imports, ...)

[nezaniel]

In that example, what benefit do tethered nodes have over NodeTemplates? If the structure / type is the important part, that should suffice, wouldn't it? That was your suggestion anyway :)

[bwaidelich]

With Node Templates you have to allow the child nodes. That basically means, that an edit could add twp "winner" pages etc

[nezaniel]

That could be prevented with UI config, e.g. by hiding that specific node type from the creation dialog. That would ofc be rather hacky too, but imho at a better location than compromising CR integrity

[bwaidelich]

but imho at a better location than compromising CR integrity

I think that the "winner" page in the example above is a perfectly valid tethered node but that it's just disabled for the time being.

I think we just have to agree to disagree on whether that disintegrates the CR – personally I don't need this (we just solved it with a custom editor) and will rest my case :)

[mhsdesign]

The same usecase for this would be the error page of the neos demo:
neos/Neos.Demo#178

but I doubt as well if it’s worth for that to compromise the integrity.
Hear me out, I think I found a good reasoning: We consider tethered nodes of the new cr to be especially consistent and always present. More so and better validated than in the old cr.

Additionally the hiding is now more a disabling and should rather be interpreted as the node is completely absent, like in the bin see neos/neos-development-collection#4312 for a definition.

But those two concepts together don’t seem to make sense if the node still can be disabled as out of perspective of a subgraph it will be totally vanished and the cr seems inconsistent with its definition.

So I would advocate to enforce to not allow disabling nodes on php api level and also not allow hiding for earlier version just to remove it later.

I very much like Bastians count restriction #3004 (comment), which has been requested a lot via slack over the years. So that is definitely something I look forward too.

[bwaidelich]

@mhsdesign and co thanks for being so persistent. This is the only way to get to a good solution.
I still don't agree 100% but I get your points (and it seems I'm the minority anyways).

I wish we had a better replacement, i.e. a way to pre-create the structure for some subtree allowing the editors to pre-populate and publish it selectively. But there are other ways to work around this.

I very much like Bastians count restriction

I also still think that this would be a very useful extension.
Calling it a "relatively small addition to node constraints" was a naive thought of me though considering dimensions, workspaces, imports etc :)

[mhsdesign]

Okay thanks for the discussion. I will close this now and reopen neos/neos-development-collection#4821 as a reminder to take care of implementing constraint-checks in neos 9 to ultimately forbid this behaviour even from the php side.