search

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
267 stars 222 forks source link

Overhaul 9.0 `NodeTypeManager` #4228

Open bwaidelich opened 1 year ago

bwaidelich commented 1 year ago

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 commented 1 year ago

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

bwaidelich commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 https://github.com/neos/neos-development-collection/pull/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 https://github.com/neos/neos-ui/issues/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 https://github.com/neos/neos-development-collection/issues/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 commented 7 months ago

@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 commented 6 months ago

In parallel, we have been progressing nicely regarding the label topic https://github.com/neos/neos-development-collection/pull/5020 and the node attached NodeType. Node::getLabel as well as Node::nodeType will be removed: https://github.com/neos/neos-development-collection/issues/5019.

I was interested what your ideas were and to prevent rotting it i kept https://github.com/neos/neos-development-collection/pull/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 https://github.com/neos/neos-development-collection/issues/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 https://github.com/neos/neos-development-collection/issues/4722

1.4) Properties cannot be unset in the inheritance chain but childNodes can be unset https://github.com/neos/neos-development-collection/pull/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)

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: https://github.com/neos/neos-development-collection/pull/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

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.

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: