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 221 forks source link

Decouple `Node` read-model from `NodeType` (remove `Node::nodeType` field) #5019

Closed mhsdesign closed 1 week ago

mhsdesign commented 2 months ago

With https://github.com/neos/neos-development-collection/pull/4466 the fallback nodetype handling was removed from the core nodetype provider and extracted to neos. (inside a trait NodeTypeWithFallbackProvider) That required to make the nodeType field nullable on the NodeType and we also deprecated it.

Staying in this intermediate step for longer has the following downsides:

Same / similar points phrased slightly different from @bwaidelich in slack:

There are two more architectural challenges due to technical debt, that I would like to address with this change:

1.) Node::nodeType it's a nullable property because the actual node type might no longer exist. IMO it would make a lot of sense to get rid of this in favor of $nodeTypeManager->getNodeType($node->nodeTypeName) – again, it should be fairly easy(tm) to replace that with a corresponding helper. And as a result we would not have to load and parse the node type configuration if it is not needed which....

2.) ...will be the case, except that the implementation of Node::getLabel() currently requires the node type instance: return $this->getNodeType()->getNodeLabelGenerator()->getLabel($this) . I would like to get rid of this oddity as well somehow. I'm not sure about the best way, but once again my tendency is another helper that does e.g. $label = $nodeTypeManager->getNodeType($node->nodeTypeName)->renderNodeLabel($node) . An alternative would be to pass the label/label generator to the Node instance at creation time. But that would mean, that we'd always have to load the whole NodeType schema again

mhsdesign commented 2 months ago

As discussed in here

10.) Idk why node label generation is THE most complex thing and it should rather be a Neos concern? It will be tricky to crack that nut while keeping everything lazy. But i dont think that the NodeLabelGeneratorInterface should be constructed with the NodeType and have the Node passed on the actual call. That seems like duplicate information.

I'm not sure what you mean. You wonder why it should be a Neos concern or you suggest to make it a Neos concern? I think the latter, right? And my gut feeling is the same.

$node->getLabel()

would have to be replaced with s.th. like

$nodeLabelRenderer->renderNodeLabel($node);

which would do sth like

$nodeTypeManager->getNodeType($node->nodeTypeName)->metadata['label.generatorClass'] ?? ...

Perfect. For node label generation we introduce a standalone Neos' singleton NodeLabelRenderer which serves as factory for the registered label.generatorClass (the actual extendable NodeLabelGeneratorInterface) and will just require the $node as argument:

https://github.com/neos/neos-development-collection/blob/3fb0d57fd568539f746ee642b944319f8ba8f562/Neos.ContentRepositoryRegistry/Classes/Factory/NodeTypeManager/ObjectManagerBasedNodeLabelGeneratorFactory.php#L23-L32

For Fusion we can create a migration that rewrites node.label to the (new) helper method Neos.Node.label(node) and we are lucky that Node::getLabel is not actually used in the ESCR core itself but only in Neos and Ui, which will be fairly easy to adapt.

bwaidelich commented 2 months ago

Node::getLabel is not actually used in the ESCR core itself but only in Neos and Ui, which will be fairly easy to adapt.

In the core we could easily fix it anyways. The real issue is, that it is used out in the wild. But with rector we should be able to fix fusion and PHP usages hopefully

mhsdesign commented 1 week ago

Fixed via https://github.com/neos/neos-development-collection/pull/5021