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

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

Open
mhsdesign opened this issue Apr 30, 2024 · 2 comments
Open
Labels

Comments

@mhsdesign
Copy link
Member

mhsdesign commented Apr 30, 2024

With #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
Copy link
Member Author

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:

if ($nodeType->hasConfiguration('label.generatorClass')) {
$nodeLabelGeneratorClassName = $nodeType->getConfiguration('label.generatorClass');
$nodeLabelGenerator = $this->objectManager->get($nodeLabelGeneratorClassName);
if (!$nodeLabelGenerator instanceof NodeLabelGeneratorInterface) {
throw new \InvalidArgumentException(
'Configured class "' . $nodeLabelGeneratorClassName . '" does not implement the required '
. NodeLabelGeneratorInterface::class,
1682950942
);
}

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
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: No status
Development

No branches or pull requests

3 participants