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

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)

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)

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.

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.

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