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

BUGFIX: Serialised node fully qualified for fusion uncached mode #4734

Conversation

mhsdesign
Copy link
Member

@mhsdesign mhsdesign commented Nov 12, 2023

Resolves: #4732

discussion should happen in #4732 and once we agree on a solution i will update this pr description

Upgrade instructions

Review instructions

test that this works:

prototype(Neos.Demo:Content.Headline) < prototype(Neos.Neos:ContentComponent) {
    renderer = afx`...`

    @cache {
        mode = 'uncached'
        context {
            1 = 'node'
        }
    }
}

Checklist

  • Code follows the PSR-2 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

@kitsunet
Copy link
Member

not a fan doing this specifically for fusion/the property mapper, why don't we replace the serialization format here \Neos\Neos\FrontendRouting\NodeAddress::serializeForUri with a fully qualified one?

@kitsunet
Copy link
Member

kitsunet commented Nov 12, 2023

Or yes, we introduce NodeIdentity and drop the serialization format from NodeAddress, that would be even neater

-> #4564

@mhsdesign mhsdesign marked this pull request as draft November 12, 2023 20:18
@mhsdesign mhsdesign force-pushed the task/4732-serializedNodeFullyQualifiedForFusionUncachedMode branch from 8d76984 to ae02d83 Compare November 12, 2023 20:51
@mhsdesign
Copy link
Member Author

Yes a NodeIdentity is the solution. And then we make use of json_serialise directly like the value object support for flow_json_array :D

@mhsdesign mhsdesign force-pushed the task/4732-serializedNodeFullyQualifiedForFusionUncachedMode branch from ae02d83 to 07ffe14 Compare February 3, 2024 16:25
@mhsdesign mhsdesign changed the title Task: #4732 serialized node fully qualified for fusion uncached mode Bugfix: #4732 serialized node fully qualified for fusion uncached mode Feb 3, 2024
@mhsdesign mhsdesign marked this pull request as ready for review February 3, 2024 17:20
@github-actions github-actions bot added the Bug label Feb 3, 2024
@mhsdesign
Copy link
Member Author

Solved it by removing the hardcoded property mapper from the fusion cache and using symfony's interfaces

@mhsdesign mhsdesign force-pushed the task/4732-serializedNodeFullyQualifiedForFusionUncachedMode branch from 5bba6e8 to 6a99952 Compare February 3, 2024 17:24
@kitsunet
Copy link
Member

kitsunet commented Feb 4, 2024

Wait what do we gain here? Another object? Like apart from abstracting the propertyMapper away here nothing has changed?

@mhsdesign
Copy link
Member Author

Oh this isnt just a new object.
This is so much more than an object :D

By adding a layer of abstraction Fusion standalone will use the property mapper yes, but Neos.Neos implements an own serialiser so that it can serialise nodes without bothering the propertymapper.
That allows us to remove property mapping for nodes e.g. NewNodeConverter and its college.
In my opinion its super important that people dont accidentally use the property mapper with the new nodes.
Allowing mapping would render all of our strictness and new explicitness useless as it would allow again magical nodes in controllers:

public function nodeAction(Node $node): {}

which will already now confusion for example about the nodes format when serialised ...

Lets to it rather explicit this time please :D

@mhsdesign mhsdesign force-pushed the task/4732-serializedNodeFullyQualifiedForFusionUncachedMode branch 2 times, most recently from 23b69c9 to fce8851 Compare February 4, 2024 17:51
@mhsdesign mhsdesign mentioned this pull request Feb 5, 2024
35 tasks
@kitsunet
Copy link
Member

kitsunet commented Feb 5, 2024

Allowing mapping would render all of our strictness and new explicitness useless as it would allow again magical nodes in controllers:

I think that is a core expectation though, countless projects will have controllers expecting a node and I don#t see why for the time being as the propertyMapper IS still the suggested and only way to do controller marshalling build into Flow, this should work

@mhsdesign
Copy link
Member Author

Huh i didnt know we werent on the same side :D I thought we discussed this already someplace but couldnt find it so i opened a separate issue: #4873

Under that aspect, i will revert the removal of the property mappers with this pr.

@mhsdesign
Copy link
Member Author

We discussed that we dislike property mapping directly to a node via the flow property mapper. For fusion we have the usecase of unserialising the node @cache.context { 0 = 'node' }.
So injecting a dedicated specialised serialiser into the RuntimeContent cache should be the first step.
As followup as part of #4564 we should unify the node serialisation across all places.

@mhsdesign mhsdesign force-pushed the task/4732-serializedNodeFullyQualifiedForFusionUncachedMode branch from fce8851 to 2a2d068 Compare February 11, 2024 10:24
@mhsdesign
Copy link
Member Author

This is now ready for review. Once we have a central NodeIdentity i will adjust the code to use that serialiser from the NeosFusionContextSerializer.
So please review the pr under that aspect and see if the general concepts make sense.

For example i decided against using a sympfony serializer wrapper for the normalizers, as its not configurable via objects yaml.
In the end this is a nice refactoring to centralise the serialisation to happen at one place (not RuntimeContentCache AND ContentCache)

@mhsdesign mhsdesign force-pushed the task/4732-serializedNodeFullyQualifiedForFusionUncachedMode branch from 997f42d to ebfbbcc Compare February 13, 2024 09:05
@mhsdesign mhsdesign requested review from Sebobo and kitsunet February 13, 2024 09:41
Copy link
Member

@Sebobo Sebobo left a comment

Choose a reason for hiding this comment

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

Fine by reading with my limited understanding of this specific topic.

@mhsdesign mhsdesign force-pushed the task/4732-serializedNodeFullyQualifiedForFusionUncachedMode branch from ebfbbcc to c5b2493 Compare February 14, 2024 16:39
@mhsdesign mhsdesign changed the title Bugfix: #4732 serialized node fully qualified for fusion uncached mode BUGFIX: Serialised node fully qualified for fusion uncached mode Feb 14, 2024
Copy link
Member

@kitsunet kitsunet left a comment

Choose a reason for hiding this comment

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

Seems fine I guess, bit weird that the serializer creates an array but I guess that's legacy from how it was before

@mhsdesign
Copy link
Member Author

mhsdesign commented Feb 15, 2024

creates an array

actually thats new :D As the content cache doesnt care as long as its json serialise able i decided for array.
And this is just an intermediate step, with #4868 we can use the NodeIdentity dto here :D

Thats how the symfony denormaliser thing is allowed to work.

@mhsdesign mhsdesign merged commit ab3f386 into neos:9.0 Feb 15, 2024
10 checks passed
@mhsdesign mhsdesign deleted the task/4732-serializedNodeFullyQualifiedForFusionUncachedMode branch February 15, 2024 08:25
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.

9.0 Avoid getActiveRequestHandler in node property mapper for uncached fusion rendering
3 participants