!!!TASK: Remove NodeTypeManager from NodeType #4520

kitsunet · 2023-09-15T18:08:41Z

Disentangle NodeTypeManager from NodeType.

Upgrade instructions

This is breaking because it removes API methods on the NodeType. If you previously used one of:

getAutoCreatedChildNodes
hasAutoCreatedChildNode
getTypeOfAutoCreatedChildNode
allowsGrandchildNodeType

the NodeTypeManager can provide replacements:

- $nodeType->getAutoCreatedChildNodes();
+ $nodeTypeManager->getTetheredNodesConfigurationForNodeType($nodeType);

- $nodeType->hasAutoCreatedChildNode($nodeName);
+ $nodeType->hasTetheredNode($nodeName);

- $parentsNodeType->getTypeOfAutoCreatedChildNode($nodeName)->name;
+ $parentsNodeType->getNodeTypeNameOfTetheredNode($nodeName);

- $parentsNodeType->getTypeOfAutoCreatedChildNode($nodeName);
+ $nodeTypeManager->getTypeOfTetheredNode($parentsNodeType, $nodeName);

- $grandParentsNodeType->allowsGrandchildNodeType($parentNodeName->value, $nodeType);
+ $nodeTypeManager->isNodeTypeAllowedAsChildToTetheredNode($grandParentsNodeType, $parentNodeName, $nodeType);

Resolves: #4515
Resolves: #4570

Disentangle NodeTypeManager from NodeType. This is breaking because it removes API methods. Resolves: neos#4515

kitsunet · 2023-09-15T18:10:25Z

Silly me, after removing everything that uses it I forgot to remove the actual dependency.... will push an update to that effect.

kitsunet · 2023-09-15T18:11:04Z

Neos.ContentRepository.Core/Classes/NodeType/ConstraintCheck.php

+     * @param array<string,mixed> $constraints
+     */
+    public function __construct(
+        private readonly array $constraints


Would be neat to have this be a DTO with the constraints configuration.

yes or at lest an example how this looks would be cool ^^

+1 for DTO
Also there seem to be some overlaps with the NodeTypeConstraints DTO

regression from 266fc1a

mhsdesign · 2023-09-16T07:48:45Z

@kitsunet i did adjust two minor things as discussed in slack in your branch, i hope that was okay ^^

kitsunet · 2023-09-16T08:31:07Z

@kitsunet i did adjust two minor things as discussed in slack in your branch, i hope that was okay ^^

Of course, good stuff

kitsunet · 2023-09-17T09:18:49Z

I just realize also @bwaidelich how would we rector this, given that we would need to insure you have a nodeTypeManager available to you?

mhsdesign · 2023-09-17T09:41:48Z

Hmmm ... id nearly argue that the few times people from outside use getAutoCreatedChildNodes its not worth to write a migration for.

For example there are 0 migrations for anything more complex. Every person with a custom importer / exporter most likely has to rewrite it. Even Node::setProperty has no migration.

The rector migrations are mostly currently for simple read operations.

So bevor migrating the dilemma getAutoCreatedChildNodes ^^ i would say we tackle more important migrations?

kitsunet · 2023-09-17T09:50:26Z

I would perfectly agree, I thought @bwaidelich saw this as condition for this change? Anyways, will fix the linter problems and then this should be good to go...

kitsunet · 2023-09-17T13:57:19Z

neos/neos-ui#3621 Adaption of UI

kitsunet · 2023-09-17T13:58:00Z

Wait why does my exception class need an internal or api annotation? (see linter error)

bwaidelich · 2023-09-17T14:45:29Z

how would we rector this

There are similar cases where we add injection code for the CR registry

I thought @bwaidelich saw this as condition for this change?

No, that was a different one (context.inBackend). I'd still love some migrations but only if it's not too much work ofc

why does my exception class need an api or internal annotation?

@skurfuerst added this rule IIRC so that we explicitly put all core classes into one of the two buckets. I think it's a great idea.
If the new exception is expected to be handled in userland code, it should be api IMO

kitsunet · 2023-09-17T23:04:51Z

Right but how I understand it and how I adjusted the code, you would need a CR registry and build the code to get to a CR id from which then you can get the respective NodeTypeManager? If it was just a simple inject for NodeTypeManager sure, that should be doable.

mhsdesign · 2023-09-18T06:57:27Z

@kitsunet yes and this is clearly impossible to adjust for every case like:

function something(NodeType $nt) {
  $nt->getAutoCreatedChildNodes();
}

mhsdesign · 2023-09-18T07:13:10Z

Hi ;) I have two little things:

just as info:

the method getNodeTypeNameOfAutoCreatedChildNode has a bug #4344 and a bugfix is on its way but ill recreated it #4482 -> tldr; we cannot do isset($this->fullConfiguration['childNodes'][$nodeName->value]['type']) as we first need to transliterate the keys childNodes of the original configuration.

Im also wondering since the allowsGrandchildNodeType method is gone if this method should stay here in this place?
Its also not often used in userland.

I also noticed once, that the method namings allowsGrandchildNodeType and allowsChildNode might not fit anymore our current naming schema. (See command handling where we rather use wordings like areNodeTypeConstraintsImposedByGrandparentValid areNodeTypeConstraintsImposedByParentValid)

So when we already move the methods around the whole world we might also rename them to fit better? What do you say @skurfuerst maybe that was your intention when writing those "wrapper" methods?

bwaidelich · 2023-09-19T08:19:28Z

you would need a CR registry and build the code to get to a CR id from which then you can get the respective NodeTypeManager?

For migrations from older versions (where there was only a single CR) we just use the default CR

this is clearly impossible to adjust for every case like [...]

But this is what rector is for, isn't it?
We do similar things already, see for example: https://github.com/neos/rector/blob/main/tests/ContentRepository90/Rules/ContentDimensionCombinatorGetAllAllowedCombinationsRector/Fixture/some_class.php.inc

bwaidelich

This is great already, thanks for the effort!
Just a couple of comments after first quick review

Neos.ContentRepository.Core/Classes/NodeType/NodeType.php

bwaidelich · 2023-09-22T08:45:14Z

@kitsunet just checking: do you still plan to work on this one for the beta1 or shall we try to find someone to take over?

(the migration topic can be skipped of course, we can always add it later if we find the time)

mhsdesign · 2023-09-23T07:47:02Z

@bwaidelich what do you say about my comment above?

Im also wondering since the allowsGrandchildNodeType method is gone if this method should stay here in this place?
Its also not often used in userland.

I also noticed once, that the method namings allowsGrandchildNodeType and allowsChildNode might not fit anymore our current naming schema. (See command handling where we rather use wordings like areNodeTypeConstraintsImposedByGrandparentValid areNodeTypeConstraintsImposedByParentValid)

So when we already move the methods around the whole world we might also rename them to fit better?

bwaidelich

Some more inline comments.

And @mhsdesign:

Im also wondering since the allowsGrandchildNodeType method is gone if this method should stay here in this place?
Its also not often used in userland.

I don't really understand which method you refer to and what you suggest to replace it with

And re

I also noticed once, that the method namings allowsGrandchildNodeType and allowsChildNode might not fit anymore our current naming schema

I agree, but "areNodeTypeConstraintsImposedByGrandparentValid" is a mouthful and not also leaves room for interpretation.

Maybe we should look at the using code again and rethink what we want to evaluate in order to find a better name

bwaidelich · 2023-09-23T08:57:10Z

Neos.ContentRepository.Core/Classes/NodeType/ConstraintCheck.php

+     * @param array<string,mixed> $constraints
+     */
+    public function __construct(
+        private readonly array $constraints


+1 for DTO
Also there seem to be some overlaps with the NodeTypeConstraints DTO

Neos.ContentRepository.Core/Classes/NodeType/NodeType.php

kitsunet · 2023-09-23T10:37:09Z

Hi ;) I have two little things:

just as info:

the method getNodeTypeNameOfAutoCreatedChildNode has a bug #4344 and a bugfix is on its way but ill recreated it #4482 -> tldr; we cannot do isset($this->fullConfiguration['childNodes'][$nodeName->value]['type']) as we first need to transliterate the keys childNodes of the original configuration.

Im also wondering since the allowsGrandchildNodeType method is gone if this method should stay here in this place? Its also not often used in userland.

I am not married to leaving it there, but it is genuinly information the nodetype has, so it felt alright to have it in there, despite everything else being "displaced" now.

@bwaidelich I will continue :) But if anyone wants to do something feel free.

mhsdesign · 2023-09-23T10:45:59Z

Sorry @bwaidelich I accidentally omitted the key word for understanding my comment

Im also wondering since the allowsGrandchildNodeType method is gone if this method should stay here in this place?
Its also not often used in userland.

I don't really understand which method you refer to and what you suggest to replace it with

I’m referring to the allowsChildNode method, and thought about moving it too to the nodetype manager to have the two methods at one place.(also the allowsChildNode has some complexity and I thought we wanted to make the nodetype less complex)

kitsunet · 2023-10-09T07:12:23Z

Sooo did you two discuss this further or can we do that this week?

mhsdesign · 2023-10-09T08:08:27Z

no lets discuss our big plan for the nodeType/Manager cleanups ^^ i think @bwaidelich continued already a bit with his other pr.

kitsunet · 2023-10-10T11:15:07Z

Collecting some open questions we should deal with and then get this merged:

getAutoCreatedChildNodesFor vs getAutoCreatedChildNodesForNodeType
remaining methods in NodeType, do we want to move them to the NodeTypeManager (where else), to me seems to make the NTM even more "godlike", and the information the methods provides is naturally in the NodeType anyways, so I would like to keep it the way it is now
renaming childNode stuff to tetheredNode stuff ?

mhsdesign · 2023-10-10T11:34:42Z

thanks for the summary, my opinion would be

use getAutoCreatedChildNodesForNodeType as described above
the method NodeType::allowsChildNode should stay as is, as you argued. Further discussions of the "meta" topic whether we want to strip down the nodeType any further should not block this pr -> there are definitely points we might consider like the implicit object manager dependency when using new for the post-processors. Turn NodeType into a less magic self-translating, lazy-loading beast -> readonly immutable #4523
this is indeed closely related and i made a separate issue !!! NodeType rename autoCreated naming to tethered #4570
but i would be fine to address the naming in another pr - with one downside: we will also rename your newly introduced getTypeOfAutoCreatedChildNode again and thats kind of smelly as the upgrade instructions here would be outdated.

kitsunet · 2023-10-11T09:39:59Z

I think in that case lets put the renaming in here so it's a single breaking change for those.

kitsunet · 2023-10-13T17:39:02Z

UI change is up to date with the latest changes.

mhsdesign · 2023-10-16T09:37:46Z

Thank you so much for adjusting this and fixing the tests ❤️ I assume this is now ready for a final review?

kitsunet · 2023-10-16T10:27:04Z

Yep all ready from my side :)

nezaniel

Fine by me

mhsdesign

Thank you for taking care and also adjusting the naming to tethered ❤️

I ran the e2e tests locally of the ui and they succeed and also the neos demo runs fine.

Additionally i had a look at your latest rename adjustments and can only approve ;)

FYI: Neos ui part: neos/neos-ui#3642

mhsdesign · 2023-10-17T07:32:06Z

Hmm im just wondering if we should allow a union here of NodeType|NodeTypeName.

otherwise one might need to write this, which feels odd doesnt it?

$nodeTypeManager->getTetheredNodesConfigurationForNodeType($nodeTypeManager->getNodeType($nodeTypeName))

Followup to neos#4520

…nger !!! TASK: Followup #4520 Introduce `NodeType::tetheredNodeTypeDefinitions`

!!!TASK: Remove NodeTypeManager from NodeType

ed2bb0f

Disentangle NodeTypeManager from NodeType. This is breaking because it removes API methods. Resolves: neos#4515

github-actions bot added the 9.0 label Sep 15, 2023

kitsunet commented Sep 15, 2023

View reviewed changes

kitsunet and others added 3 commits September 15, 2023 20:18

Remove constructor injection of NodeTypeManager in NodeType

e9d2fac

TASK: Remove accidental public declaration of NodeType::$abstract

5acb733

regression from 266fc1a

TASK: Declare construction of NodeType @internal

23d715f

Adjust signature of populateNodeAggregateIds

5686898

Fix linting errors

0a2a126

kitsunet marked this pull request as ready for review September 17, 2023 13:53

bwaidelich reviewed Sep 19, 2023

View reviewed changes

Neos.ContentRepository.Core/Classes/NodeType/NodeType.php Show resolved Hide resolved

Neos.ContentRepository.Core/Classes/NodeType/NodeType.php Outdated Show resolved Hide resolved

mhsdesign mentioned this pull request Sep 19, 2023

WIP: FEATURE: Clean up NodeType implementation #4183

Closed

bwaidelich reviewed Sep 23, 2023

View reviewed changes

Address PR comments

4222a9b

mhsdesign mentioned this pull request Sep 29, 2023

!!! NodeType rename autoCreated naming to tethered #4570

Closed

kitsunet added 5 commits October 13, 2023 16:41

Merge branch '9.0' into task/90-remove-nodetypemanager-from-node-4515

56e1b32

Rename methods to use tethered instead of child node

a2a0447

Adjust new DefaultPropertyEditorPostProcessorTest

53a5ad7

Fix syntactic error in DisallowedChildNodeAdjustment

c7aea4e

Fix TetheredNodeAdjustments after wrong merge

6b28944

Fix merge error in DisallowedChildNodeAdjustment

0742577

mhsdesign self-requested a review October 16, 2023 09:37

nezaniel approved these changes Oct 16, 2023

View reviewed changes

mhsdesign approved these changes Oct 16, 2023

View reviewed changes

mhsdesign mentioned this pull request Oct 17, 2023

Critical migrations neos/rector#25

Closed

9 tasks

mhsdesign merged commit f04a914 into neos:9.0 Oct 17, 2023
8 checks passed

dlubitz mentioned this pull request Oct 23, 2023

Remove NodeTypeManager from NodeType neos/rector#35

Closed

kitsunet deleted the task/90-remove-nodetypemanager-from-node-4515 branch February 13, 2024 08:35

mhsdesign mentioned this pull request Feb 20, 2024

!!! TASK: Followup #4520 Introduce NodeType::tetheredNodeTypeDefinitions #4906

Merged

6 tasks

mhsdesign added a commit to mhsdesign/neos-development-collection that referenced this pull request Apr 3, 2024

!!! TASK: Refine NodeTypeManager API to use NodeTypeName as parameters

ec8f4ef

Followup to neos#4520

mhsdesign mentioned this pull request Apr 22, 2024

Overhaul 9.0 NodeTypeManager #4228

Open

mhsdesign added a commit to mhsdesign/neos-development-collection that referenced this pull request May 1, 2024

!!! TASK: Refine NodeTypeManager API to use NodeTypeName as parameters

258f177

Followup to neos#4520

mhsdesign added a commit to mhsdesign/neos-development-collection that referenced this pull request May 13, 2024

!!! TASK: Refine NodeTypeManager API to use NodeTypeName as parameters

a26feb7

Followup to neos#4520

mhsdesign added a commit to mhsdesign/neos-development-collection that referenced this pull request May 14, 2024

!!! TASK: Refine NodeTypeManager API to use NodeTypeName as parameters

b4fe67e

Followup to neos#4520

kitsunet added a commit that referenced this pull request May 17, 2024

Merge pull request #4906 from mhsdesign/task/followup-4520-nodeTypeMa…

b3d32bf

…nger !!! TASK: Followup #4520 Introduce `NodeType::tetheredNodeTypeDefinitions`

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

!!!TASK: Remove NodeTypeManager from NodeType #4520

!!!TASK: Remove NodeTypeManager from NodeType #4520

kitsunet commented Sep 15, 2023 •

edited by mhsdesign

Loading

kitsunet commented Sep 15, 2023 •

edited

Loading

kitsunet Sep 15, 2023

mhsdesign Sep 18, 2023

bwaidelich Sep 23, 2023

mhsdesign commented Sep 16, 2023

kitsunet commented Sep 16, 2023

kitsunet commented Sep 17, 2023

mhsdesign commented Sep 17, 2023

kitsunet commented Sep 17, 2023

kitsunet commented Sep 17, 2023

kitsunet commented Sep 17, 2023 •

edited

Loading

bwaidelich commented Sep 17, 2023

kitsunet commented Sep 17, 2023

mhsdesign commented Sep 18, 2023

mhsdesign commented Sep 18, 2023

bwaidelich commented Sep 19, 2023

bwaidelich left a comment

bwaidelich commented Sep 22, 2023

mhsdesign commented Sep 23, 2023

bwaidelich left a comment

bwaidelich Sep 23, 2023

kitsunet commented Sep 23, 2023

mhsdesign commented Sep 23, 2023 •

edited

Loading

kitsunet commented Oct 9, 2023

mhsdesign commented Oct 9, 2023

kitsunet commented Oct 10, 2023

mhsdesign commented Oct 10, 2023

kitsunet commented Oct 11, 2023

kitsunet commented Oct 13, 2023 •

edited

Loading

mhsdesign commented Oct 16, 2023

kitsunet commented Oct 16, 2023

nezaniel left a comment

mhsdesign left a comment •

edited

Loading

mhsdesign commented Oct 17, 2023

!!!TASK: Remove NodeTypeManager from NodeType #4520

!!!TASK: Remove NodeTypeManager from NodeType #4520

Conversation

kitsunet commented Sep 15, 2023 • edited by mhsdesign Loading

kitsunet commented Sep 15, 2023 • edited Loading

kitsunet Sep 15, 2023

Choose a reason for hiding this comment

mhsdesign Sep 18, 2023

Choose a reason for hiding this comment

bwaidelich Sep 23, 2023

Choose a reason for hiding this comment

mhsdesign commented Sep 16, 2023

kitsunet commented Sep 16, 2023

kitsunet commented Sep 17, 2023

mhsdesign commented Sep 17, 2023

kitsunet commented Sep 17, 2023

kitsunet commented Sep 17, 2023

kitsunet commented Sep 17, 2023 • edited Loading

bwaidelich commented Sep 17, 2023

kitsunet commented Sep 17, 2023

mhsdesign commented Sep 18, 2023

mhsdesign commented Sep 18, 2023

bwaidelich commented Sep 19, 2023

bwaidelich left a comment

Choose a reason for hiding this comment

bwaidelich commented Sep 22, 2023

mhsdesign commented Sep 23, 2023

bwaidelich left a comment

Choose a reason for hiding this comment

bwaidelich Sep 23, 2023

Choose a reason for hiding this comment

kitsunet commented Sep 23, 2023

mhsdesign commented Sep 23, 2023 • edited Loading

kitsunet commented Oct 9, 2023

mhsdesign commented Oct 9, 2023

kitsunet commented Oct 10, 2023

mhsdesign commented Oct 10, 2023

kitsunet commented Oct 11, 2023

kitsunet commented Oct 13, 2023 • edited Loading

mhsdesign commented Oct 16, 2023

kitsunet commented Oct 16, 2023

nezaniel left a comment

Choose a reason for hiding this comment

mhsdesign left a comment • edited Loading

Choose a reason for hiding this comment

mhsdesign commented Oct 17, 2023

kitsunet commented Sep 15, 2023 •

edited by mhsdesign

Loading

kitsunet commented Sep 15, 2023 •

edited

Loading

kitsunet commented Sep 17, 2023 •

edited

Loading

mhsdesign commented Sep 23, 2023 •

edited

Loading

kitsunet commented Oct 13, 2023 •

edited

Loading

mhsdesign left a comment •

edited

Loading