Overhaul 9.0 NodeTypeManager

[bwaidelich]

Turn the NodeTypeManager into an immutable and type safe API.

The constructor currently has two properties:

    public function __construct(
        private readonly \Closure $nodeTypeConfigLoader,
        private readonly NodeLabelGeneratorFactoryInterface $nodeLabelGeneratorFactory
    ) {
    }

Especially the $nodeTypeConfigLoader closure should be replaced by some type safe, immutable construct.
This will also affect the NodeType class that needs a thorough rework

Edit: Updated by @bwaidelich on April 20th 2024, the original points:

Challenge 1: Supertypes and the full configuration is currently resolved lazily and we might have to keep it that way for performance reasons.. But I would suggest to start with the type-safe "eager-loading" version and see how that works out first

Challenge 2: The ObjectManager is currently passed through to the NodeType in order to resolve the "nodeLabelGenerator". This dependency should be avoided (see #4227)

Challenge 3: Prevent circular inheritance (superTypes)

Challenge 4: Should the "node type" builder/specification object be the same that you get back from NodeTypeManager::getNodeType()?

Challenge 5: (Neos integration) How to load only specified types (maybe specify Package Key(s) and load types specified there + all depend types. Or: allow/deny lists like Some.Package:Document.*

[mhsdesign]

Would you be fine by using the Psr\Container\ContainerInterface interface instead of the object manager?

[bwaidelich]

Would you be fine by using the Psr\Container\ContainerInterface interface instead of the object manager?

@mhsdesign your comment is quite old and in the meantime we already "solved" the ObjectManager issue. But for the record: IMO we should avoid service location in the core as much as possible. E.g. it would be totally fine in the ContentRepositoryRegistry but for everything in the CR Core a ContainerInterface basically means that we're missing a concept. We currently still use new .. in the NodeTypeManager but IMO that can be replaced:

Suggestions for an overhauled NodeType DTO

My first impulse was to create a DTO that represents a single NodeType.yaml configuration (with nice and dandy DTOs and unstructured arrays only for the Neos/UI specific options).

But that would mean, that we could not properly re-implement NodeType::isOfType() without having to reach out to the NodeTypeManager (because of the transitive super types that are not explicitly declared by the node type).

So I came to the conclusion that we should – once more – simplify the core and keep all this inheritance hassle to the higher levels.

That would mean, that we do the YAML/array to NodeType resolution completely in Neos (it should still be possible to do that lazily and for each node individually). Each built NodeType would be the result of the merged super type configuration and NodeType::superTypeNames would be the complete list of all super node type names

Concretely, I could imagine an API like this:

final readonly class NodeType {
    private function __construct(
        public NodeTypeName $name,
        public NodeTypeNames $superTypeNames,
        public TetheredNodeTypeDefinitions $tetheredNodeTypeDefinitions,
        public PropertyDefinitions $propertyDefinitions,
        public ReferenceDefinitions $referenceDefinitions,
        public bool $isAggregate,
        public NodeTypeConstraints $childNodeTypeConstraints,
        public array $metadata,
        public NodeTypeLabel|null $label,
        private NodeLabelGeneratorInterface|null $nodeLabelGenerator,
    ) {}
}

(with useful static constructors and withers)

TetheredNodeTypeDefinitions is a type-safe set of:

final readonly class TetheredNodeTypeDefinition {
    public function __construct(
        public NodeName $name,
        public NodeTypeName $nodeType,
        public NodeTypeConstraints $nodeTypeConstraints,
    ) {}
}

PropertyDefinitions is a type-safe set of:

final readonly class PropertyDefinition {
    /**
     * @param int|float|string|bool|array<int|string,mixed>|null $defaultValue
     */
    public function __construct(
        public PropertyName $name,
        public string $type,
        public PropertyScope $scope,
        public int|float|string|bool|array|null $defaultValue,
        public array $metadata,
    ) {}
}

ReferenceDefinitions is a type-safe set of:

final readonly class ReferenceDefinition {
    public function __construct(
        public ReferenceName $name,
        public string $type,
        public PropertyScope $scope,
        public array $metadata,
    ) {}
}

I might still miss some details, but basically like this.

For the public methods of NodeType this would mean:

isAbstract() & isFinal()

would no longer exist, since that would be a Neos notion that's only relevant for the construction

getDeclaredSuperTypes()

This would have to be removed because it requires the NodeType to have access to other NodeTypes which makes it really hard to implement and might lead to performance issues even.
Instead, the NodeTypeManager should provide means to resolve super types, e.g. public function getSuperNodeTypesRecursively(string|NodeTypeName $nodeTypeName): NodeTypes

In most cases it's probably enough to access the super type names.
E.g. in the ContentCacheFlusher

protected function getAllImplementedNodeTypeNames(NodeType $nodeType): array {
    $self = $this;
    $types = array_reduce(
        $nodeType->getDeclaredSuperTypes(),
        function (array $types, NodeType $superType) use ($self) {
            return array_merge($types, $self->getAllImplementedNodeTypeNames($superType));
        },
        [$nodeType->name->value]
    );

    return array_unique($types);
}

could be replaced with a simple

$nodeType->superTypeNames

isOfType(string|NodeTypeName $nodeTypeName)

Could be kept but might be deprecated and simplified to

/**
 * @deprecated use $nodeType->superTypeNames->contain() instead
 */
public function isOfType(string|NodeTypeName $nodeTypeName): bool {
    return $this->superTypeNames->contain($nodeTypeName);
}

getFullConfiguration(), hasConfiguration(), getConfiguration()

Could be kept as b/c layer but deprecated.
internally getFullConfiguration() could do something like:

public function getFullConfiguration(): array {
    $configuration = $this->metadata;
    $configuration['label'] = $this->label->value,
    $configuration['properties'] = $this->propertyDefinitions->map(fn (PropertyDefinition $propertyDefinition) => (...));
    // ...
    return $configuration;
}

### `getLocalConfiguration()`

Would be removed (since the configuration would always be merged in the higher levels already).
I have no clue why this was added as public method ([10 years ago](https://github.com/neos/neos-development-collection/commit/eb1f704095fd4e22e1e6805758c56e4eb1ffa795)), it is currently not used by Neos

### `getLabel()`

Could be kept as shortcut to `$this->label->value`

### `getOptions()`

Could be kept as
```php
return $this->metadata['options'] ?? [];

getNodeLabelGenerator()

Could be replaced by something that keeps this interface more local, e.g.

public function renderNodeLabel(Node $node): string
{
    return $this->nodeLabelGenerator !== null ? $this->nodeLabelGenerator->getLabel($node) : mb_substr($node->nodeTypeName->value, mb_strrpos($node->nodeTypeName->value, '.') + 1);
}

(this way we can make this custom generator optional and spare the default NodeTypeNameNodeLabelGenerator implementation)

getProperties(), getPropertyType(), getDefaultValuesForProperties(), hasProperty()

Could be kept as b/c like:

/**
 * @deprecated use $nodeType->propertyDefinitions instead
 */
public function getProperties(): array {
  return $this->propertyDefinitions->map(fn (PropertyDefinition $definition) => $definition->toArray());
}

/**
 * @deprecated use $nodeType->propertyDefinitions->contain() instead
 */
public function hasProperty(string $propertyName): bool {
  return $this->->propertyDefinitions->contain($propertyName);
}

/**
 * @deprecated use $nodeType->propertyDefinitions->get($propertyName)->type instead
 */
public function getPropertyType(string $propertyName): string
  return $this->propertyDefinitions->get($propertyName)->type;
}

public function getDefaultValuesForProperties(): array {
  return $this->propertyDefinitions->map(fn (PropertyDefinition $definition) => $definition->defaultValue);
}

getReferences(), hasReference()

Could be removed in favor of $nodeType->referenceDefinitions. No b/c required, because they didn't exist in Neos < 9

hasTetheredNode(), getNodeTypeNameOfTetheredNode()

Could be removed in favor of $nodeType->tetheredNodeDefinitions. No b/c required, because they didn't exist in Neos < 9

allowsChildNodeType()

Could all be kept as (deprecated) b/c layer (like above)

[bwaidelich]

one of the bigger changes, that might not be clear from the above comment alone:
Node::superTypeNames would be the completely, expanded list of super type names.
In the core there would be no check whether those exist as actual, instantiatable node type because it doesn't matter.
It's just: This is a node type schema with it's property-, reference- and tethered node definitions (potentially merged from some super types) and it belongs to this set of super type names (which merely become a way to classify multiple types for organizational purposes at read time)

[kitsunet]

be no check whether those exist as actual,

That's fine by me, might be nice to comment at the method but I don't see a problem there.

[mhsdesign]

thanks for this write-down ;) yes the NodeType is the last ugly piece in our codebase :D ... i just lately added unit tests to assert partially odd behaviour, that should help us to refactor the NodeType better.

1.) NodeType::hasTetheredNode was indeed introduced as replacement for hasAutoCreatedChildNode with 9.0 via #4520 but there is also a rector migration which has to be adjusted in case we remove / rename this thing again ... just fyi ^^

https://github.com/neos/rector/blob/4843c45f5bd901bd63aad0d1d0367df394e2cd1e/config/set/contentrepository-90.php#L277-L282

2.) Im wondering if we really need NodeType::isAggregate as core concept? Wasn't there lately a discussion that we are unsure about this concept currently and dont use it once in content repository core yet?
Related neos/neos-ui#3587
-> but then again lets not split hairs... the property exists in 8.3 so it might as well in 9.0 ... we dont have to go into that rabbit hole as well.

3.) NodeType::isOfType is currently in bigger 8.3 installs quite slow

seb and me use a hacky private static $isOfTypeCache = []; cache patch to work around that.
We should definitely keep this current performance problem in our heads and i wonder if your draft already fixes it conceptional?

4.) NodeType::getLocalConfiguration is marked as internal (kindof) and it seems we would get rid of it with no replacement?

5.) its a good thing to get rid of all the $this->initialize(); calls as they cause weird glitches sometimes if left out #4333

6.) The *Definition look really clean and would safe us from constantly creating cr core value objects from the node type from the outside like PropertyScope and PropertyName

... Also the unstructured $metaData seems a good name and explicitly states what is cr api / structure and what not...
But the metaData is not directly declared on the NodeType and all other keys are just taken into account which is a bit weird but legacy:

type: string # core
defaultValue: abc # core
required: true # future core ... but now part of metadata
ui: # metadata
  inspector: ... # metadata
arbitraryStuff: # metadata
validators: # metadata

metaData would be an array with the keys required,ui,arbitraryStuff,validators

7.) we should definitely have the NodeType either not attached to the Node (which is architectural preferred but more breaking) or have a legacy mechanism that Node::getNodeType will return a NodeType but lazily.

As we have already a migration from 8.3 Node::getNodeType to Node::nodeType i could very well imagine to adjust this migration to use the nodeType manager

https://github.com/neos/rector/blob/4843c45f5bd901bd63aad0d1d0367df394e2cd1e/config/set/contentrepository-90.php#L145

8.) In Neos.Neos we currently use the NodeTypeWithFallbackProvider trait to fake the fallback nodetype handling. If we fully remove the NodeType from the Node people will migrate to use this trait as well, but im currently not happy with this new trait because ... trait ...

9.) While reading nodetypes from yaml is of course mainly a Neos.Neos concern, standalone projects might also profit from this declarative syntax so maybe keep some part of the final / abstract yaml merging / processing in the core and let neos use / extend it?

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.

[bwaidelich]

@mhsdesign Thanks a lot for your feedback!

1.) NodeType::hasTetheredNode was indeed introduced as replacement for hasAutoCreatedChildNode [...] but there is also a rector migration which has to be adjusted

Yes, thanks, I added it as todo to #4999

2.) Im wondering if we really need NodeType::isAggregate as core concept?

Currently we only use in a single place in Neos.Neos within in NodesController::addExistingNodeVariantInformationToResponse() it seems – but we do rely on it.

But it could be replaced with isOfType('Neos.Neos:Document') I assume...

3.) NodeType::isOfType is currently in bigger 8.3 installs quite slow

This would become really fast now, because it's just $this->superTypeNames->contain($nodeTypeName)

4.) NodeType::getLocalConfiguration is marked as internal (kindof) and it seems we would get rid of it with no replacement?

Yes, I agree – there is no sensible way to re-implement this with the suggested approach

But the metaData is not directly declared on the NodeType and all other keys are just taken into account which is a bit weird

Not 100% sure what you mean? That meta- and core data are mixed on one level? That would only be the case for the legacy configuration array – in the core we should replace this by calls to the DTOs where possible.

Re

required: true # future core ... but now part of metadata

Good catch – that should be part of the core props IMO and the Neos NodeTypeProvider could already translate NonEmptyValidators to that flag

7.) [...] or have a legacy mechanism that Node::getNodeType will return a NodeType but lazily.

I don't know how that could work lazily in a clean way, we'd still have to expose some ugly injection object to the Node model..

As we have already a migration from 8.3 Node::getNodeType to Node::nodeType i could very well imagine to adjust this migration to use the nodeType manager

+1

8.) In Neos.Neos we currently use the NodeTypeWithFallbackProvider trait to fake the fallback nodetype handling. If we fully remove the NodeType from the Node people will migrate to use this trait as well, but im currently not happy with this new trait because ... trait ...

I agree.. IMO a trait is almost never the best option ;)
We could consider adding sth like NodeTypeManager::getNodeTypeOrFallback(NodeTypeName $nodeTypeName, NodeTypeName $fallbackNodeTypeName)
But IMO that part should be left out of this (already quite large) undertaking

9.) While reading nodetypes from yaml is of course mainly a Neos.Neos concern, standalone projects might also profit from this declarative syntax so maybe keep some part of the final / abstract yaml merging / processing in the core and let neos use / extend it?

Yes, I agree. As a first step, I would like to get the Neos NodeTypeProvider to work, but as a follow-up we could try to extract the lazy-loading and abstract/final handling from the Neos specific things (NodeTypeEnrichmentService, ObjectManagerBasedNodeLabelGeneratorFactory, ...)

10.) Idk why node label generation is THE most complex thing and it should rather be a Neos concern?

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'] ?? ...

[mhsdesign]

In parallel, we have been progressing nicely regarding the label topic #5020 and the node attached NodeType. Node::getLabel as well as Node::nodeType will be removed: #5019.

I was interested what your ideas were and to prevent rotting it i kept #4999 up to date after the changes.

A few things came to my eye when i further examined the draft:

1.) Current problems with NodeTypes

In addition to points 2 and 3 of my previous comment

1.1) Legacy namings and transformations like "childNodes" and reference properties are part of the core

1.2) The forgiving (sometimes forgotten) logic to create a valid NodeName by transliterating from a string, that even empty child nodes work is problematic but currently part of the core #4344

1.3) The type of a property can be changed in the inheritance chain completely, not only narrowed to a more specific say php class implementation #4722

1.4) Properties cannot be unset in the inheritance chain but childNodes can be unset #4618

1.5) The NodeType and NodeTypeManager operate the heaviest possible way constantly only on the yaml input array and its all sprinkled over several places :)

1.6) The API NodeTypeManager::getNodeTypes with the flag includeAbstract is weird, and we have optimise abstract NodeTypes a few times out of the system so they do not bloat the Neos Ui schema.

1.7) Calling NodeTypeManager::hasNodeType actually loads the NodeType in memory instead of checks if the name exists

2.) Concepts to move to Neos

Following things are not needed by the cr core and should be part of Neos (and thus the Neos.ContentRepositoryRegistry so its also helpful for other flow installs)

• The NodeTypeEnrichmentService handles translation but also adds Neos Ui configuration
  -> actually this could be a pure Neos.Neos thing and not of use for other projects.
• The NodeTypePostprocessorInterface provides an API to hook into the raw array node type configuration. As the core must work with defined PHP value objects this part will be added as process from Neos.
  -> als the post processors are currently instantiated via new (aka using the object manager) and this is only legit in Neos an not the core.
• Certain legacy transformations like properties with type: references being treated as ReferenceDefinition is a pure Neos legacy layer.
• Bastians initial idea was also to make abstract: true and final: true concepts of the NodeTypeProvider and thus Neos

3.) Standalone PHP only NodeType schema definition

I tried to imagine the stand-alone cr use case and wondered how one would use the API to define the following NodeType schema:

'Neos.ContentRepository.Testing:AbstractNode':
  abstract: true
  properties:
    text:
      defaultValue: 'abstract'
      type: string
'Neos.ContentRepository.Testing:SubNode':
  superTypes:
    'Neos.ContentRepository.Testing:AbstractNode': true
  properties:
    other:
      type: string
'Neos.ContentRepository.Testing:Node':
  superTypes:
    'Neos.ContentRepository.Testing:AbstractNode': true
  childNodes:
    child-node:
      type: 'Neos.ContentRepository.Testing:SubNode'
  properties:
    text:
      defaultValue: 'test'
'Neos.ContentRepository.Testing:Page':
  superTypes:
    'Neos.ContentRepository:Root': true

First i thought, well this is simple, by just instantiating the NodeType instances that should be available.
But while progressing I noticed a tonn of problems and uncertainties (and maybe this was never you idea at all but discussing is probably worth it ^^).
The discussion points are marked with // 3.*) and elaborated below.

$nodeTypeManager = new NodeTypeManager(
    DefaultNodeTypeProvider::createFromNodeTypes(
        NodeType::create(name: NodeTypeName::fromString(NodeTypeName::ROOT_NODE_TYPE_NAME)), // 3.1) 
        NodeType::create(
            name: NodeTypeName::fromString('Neos.ContentRepository.Testing:AbstractNode'), // 3.2)
            propertyDefinitions: PropertyDefinitions::fromArray([
                PropertyDefinition::create(
                    name: PropertyName::fromString('text'),
                    type: 'string',
                    defaultValue: 'abstract',
                )
            ])
        ),
        NodeType::create(
            name: NodeTypeName::fromString('Neos.ContentRepository.Testing:SubNode'),
            superTypeNames: NodeTypeNames::fromStringArray(['Neos.ContentRepository.Testing:AbstractNode']),
            propertyDefinitions: PropertyDefinitions::fromArray([
                PropertyDefinition::create(
                    name: PropertyName::fromString('other'),
                    type: 'string',
                )
                // 3.3)
            ])
        ),
        NodeType::create(
            name: NodeTypeName::fromString('Neos.ContentRepository.Testing:Node'),
            superTypeNames: NodeTypeNames::fromStringArray(['Neos.ContentRepository.Testing:AbstractNode']),
            tetheredNodeTypeDefinitions: TetheredNodeTypeDefinitions::fromArray([
                new TetheredNodeTypeDefinition(
                    name: NodeName::fromString('child-node'),
                    nodeTypeName: NodeTypeName::fromString('Neos.ContentRepository.Testing:SubNode'),
                    nodeTypeConstraints: NodeTypeConstraints::createEmpty()
                )
            ]),
            propertyDefinitions: PropertyDefinitions::fromArray([
                PropertyDefinition::create(
                    name: PropertyName::fromString('text'),
                    type: 'string', // 3.4)
                    defaultValue: 'test',
                )
            ])
        ),
        NodeType::create(
            name: NodeTypeName::fromString('Neos.ContentRepository.Testing:Page'),
            superTypeNames: NodeTypeNames::fromStringArray([NodeTypeName::ROOT_NODE_TYPE_NAME]),
        ),
    )
);

3.1) does the root type have to be specified, or can be overridden and has own meta-data?

3.2) as the abstract state should not be part of the core there would be no constraints against instantiating such abstract like or mixing like node type

3.3) should property 'text' be actually inherited by logic in the node type manager? Otherwise, this would be an unset property which we discussed to not support on Neos level: #4618

3.4) has the type to be specified or should be taken from inherited?
-> to ensure consistency in the value object it has to be set as otherwise it could mean properties having no type at runtime.

4.) SuperType merging vs not merging

So using pure value objects as api also has a downside regarding the supertypes behaviour.
To keep the implementation simple we would only handle the merging in the Neos adapter and standalone use-cases would have to use the above syntax where no merging is applied.
But to what extent does the above definition make sense in that case?

Merging only in the Neos adapter and keeping the core simple

• 4.1) The above in 1.) mentioned problems regarding merging and their complexity doesnt affect the core
• 4.2) The NodeType schema as standalone case will be used differently, with less typical inheritance, and it will not do good to compare it to the classic yaml definition with merging.
  -> For sharing properties and co other solutions will arise, and we would see more actual composition to create the schema: createImageProperty('myImage'): PropertyDefinition
• 4.3) The superTypeNames can be used as flexible tag and evolve to a different feature and will become NodeTypeName independent.
• 4.4) A disadvantage would be that we currently use abstract NodeTypes (abstract: true) and inheritance of properties in our behat test suite (superTypes). This calls for a mini Neos-like NodeType provider backport. But before having two array transforming beasts with merging feature we should rather adjust the test suite to not use merging and just simple definitions that can be instantiated via fromArray. Property sharing is possible via yaml references.
• 4.5) On the other hand one can say that the PHP NodeType API without the merging feature is not really the NodeType DSL but something too complex, and one might prefer the yaml syntax also for other standalone projects.
• 4.6) In a standalone case having a mixing that should symbolise that all inheritors define a property will not be enforced. That means filtering after such a mixin is no guarantee that the node has the thought property.

The NodeTypeManger will keep merging capabilities
... and accept LocalNodeType objects (with all files always nullable, meaning they will be inherited) that will be transformed internally.

• 4.7) The behat tests could stay as they are still using much inheritance and abstract: true
• 4.8) The merging behaviour will be part of the core which makes the core more complex but the feature hopefully more hardened and better tested

5.) Conclusion

Wow this topic is super complex. We might not get this done for Neos 9.0.
But we should not leave any stones in our way to do it at a later point.
I found following things that should be adjusted:

• 5.1) The wip idea introduced a new NodeTypeConstraints and removes the first of the existing helpers NodeTypeConstraints and ExpandedNodeTypeCriteria
• 5.2) The PropertyDefinition::scope uses the PropertyScope enum which lives in Core\Feature\NodeModification\Dto and should be moved
• 5.3) The PropertyDefinition::type is a string but should probably use Neos\ContentRepository\Core\Infrastructure\Property\PropertyType and move it
• 5.4) The newly introduced TetheredNodeTypeDefinition should have an @internal constructor as well probably reconsider the construction and introduce static factories etc
• 5.5) The hasReferences and co might be already forward compatible changed to ReferenceDefinitions and we might as well introduce a PropertyDefinitions
• 5.6) NodeType::getLocalConfiguration is marked as internal but must be absolutely deprecated
• 5.7) NodeType post processors cannot have access to the initialized NodeType instance (calling anything would lead to recursion anyway) ... only $nodeType->name is safe
• 5.8) Fix hasNodeType see 1.7
• 5.9) Avoid fetching the NodeType if not absolutely required. See q().propert() will check if the property is a reference before using $node->getProperty
• 5.10) Deprecate NodeType::getFullConfiguration and getConfiguration and hasConfiguration