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

!!! FEATURE: NodeTypeManager remove NodeType fallback handling #4466

Merged
merged 31 commits into from
Sep 29, 2023

Conversation

nezaniel
Copy link
Member

@nezaniel nezaniel commented Sep 1, 2023

This moves node type fallback handling from the CR to the outer applications, e.g. Neos.

!!! Nodes now have an optional node type which is null if the NodeTypeName is not known to the NodeTypeManager.

Neos has been adjusted to

  • ask the NodeTypeManager for the node type
  • deal with NodeTypeNotFound itself by applying fallback to Neos.Neos:Fallback

For 3rd party applications relying on fallback NodeTypes, there now is the Neos\Neos\Utility\NodeTypeWithFallbackProvider trait available

Related: #4447

Checklist

  • Code follows the PSR-12 coding style
  • Tests have been created, run and adjusted as needed
  • The PR is created against the lowest maintained branch
  • Reviewer - PR Title is brief but complete and starts with FEATURE|TASK|BUGFIX
  • Reviewer - The first section explains the change briefly for change-logs
  • Reviewer - Breaking Changes are marked with !!! and have upgrade-instructions

Copy link
Member

@mhsdesign mhsdesign left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

THANK YOU SO MUCH for this quick implementation.

I did a full visual review - it was a little harder because you also adjusted more code to use the NodeTypeNameFactory and avoid now magic strings like 'Neos.Neos:Document' but thank you for that as well.

I have a few points to discuss left - but overall i approve mostly already ;)

Copy link
Contributor

@dlubitz dlubitz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me by reading! Thank you

mhsdesign added a commit that referenced this pull request Sep 2, 2023
related #4311

based upon this pr, which must be merged first: #4466
…rotected var pattern in trait

Introducing the pattern with the additional `ContentRepositoryRegistryProvider` makes things just more complex and less obvious.
While not perfectly "clean" we instead import in the `NodeTypeWithFallbackProvider` trait the `ContentRepositoryRegistry` via $_contentRepositoryRegistry.
This pattern is already used across the codebase.
Copy link
Member

@mhsdesign mhsdesign left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adjusting to the feedback from the weekly.

But i think we misunderstood each other about the traits.
With 364a230 you added the ContentRepositoryRegistryProvider and i think you wanted to implement bastians suggestion, but this would require an abstract method getContentRepositoryById.

Instead i propose to delay this discussion - or do nothing fancy here at all and just import the ContentRepositoryRegistry directly in the NodeTypeWithFallbackProvider as prefixed variable.

This pattern - while not especially pretty - has already been used across the codebase and we might just as well do it here too before blocking this pr any longer with this discussion.

Edit: i just found out that phpstan will perfectly analyze the traits, so we only have to use the property in the trait and declare it at another place: https://phpstan.org/blog/how-phpstan-analyses-traits

I implemented the suggestion here - feel free to merge it into this pr: :D
#4559

@mhsdesign mhsdesign changed the title !!!FEATURE: Node type fallback handling !!! FEATURE: NodeTypeManager remove NodeType fallback handling Sep 25, 2023
@mhsdesign
Copy link
Member

mhsdesign commented Sep 25, 2023

The neos ui has to be adjusted as well. I removed the Headline nodeType from the demo site to test the fallback but the whole request fails (in the neos backend):

Call to a member function isOfType() on null

Exception Code | 0
-- | --
Error
Data/Temporary/Development/Cache/Code/Flow_Object_Classes/Neos_Neos_Ui_ContentRepository_Service_WorkspaceService.php
210
Packages/Application/Neos.Neos.Ui/Classes/ContentRepository/Service/WorkspaceService.php

This seems to be tested nowhere as the Neos Ui e2e tests pass and the neos ui has no phpstan 😢

Also the fusion rendering in the frontend is not really successful after removing the Headline

image

Im not sure, but this is not the behavior of Neos 8.x -> the node should just be not rendered.

thats because we access the nodeType in fusion:

the content case needs to be adjusted to (or do we actually need a better way of getting the nodes nodetype in fusion?)

prototype(Neos.Neos:ContentCase) < prototype(Neos.Fusion:Case) {
  default {
    @position = 'end'
    condition = true
    type = ${node.nodeType.name || "Neos.Neos:FallbackNode"}
  }
}

@mhsdesign
Copy link
Member

mhsdesign commented Sep 25, 2023

I think we’ll need a NodeType for node helper for eel aswell? ${Neos.Node.getNodeType(node)}

im not sure how we handle node.nodeType is the use deprecated? Should we migrate Fusion already? And for how long is it guaranteed that the node knows it’s nodeType if fetched via the official ways - until Neos 10 and the well remove it?

Those questions need to be answered so that our users now what to expect and we know when to deprecate/remove it.

Copy link
Member

@bwaidelich bwaidelich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes a lot of sense to me, I just wonder about the changed linter rules

.composer.json Show resolved Hide resolved
Copy link
Member

@mhsdesign mhsdesign left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In combination with neos/neos-ui#3632 the neos ui e2e tests succeed locally

@mhsdesign mhsdesign merged commit d0067c7 into 9.0 Sep 29, 2023
7 checks passed
@mhsdesign mhsdesign deleted the nodeTypeFallbackHandling branch September 29, 2023 15:12
ahaeslich added a commit to neos/neos-seo that referenced this pull request Oct 8, 2023
ahaeslich added a commit to neos/neos-seo that referenced this pull request Oct 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants