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 Workspace aware NodeCacheEntryIdentifier #5083

Merged

Conversation

dlubitz
Copy link
Contributor

@dlubitz dlubitz commented May 21, 2024

No description provided.

@github-actions github-actions bot added the 9.0 label May 21, 2024
@dlubitz dlubitz requested a review from mhsdesign May 21, 2024 06:59
@mhsdesign
Copy link
Member

Im currently digging into the history of this NodeCacheEntryIdentifier what is it and since when? Its def new with. 9.0

@bwaidelich
Copy link
Member

Nice. Such a simple change.
I would be much more confident if we had a test for this, preferably a behat test. But I'm not sure how that would look like?!

@mhsdesign
Copy link
Member

mhsdesign commented May 21, 2024

originally the Neos 8.3 node was a CacheAwareInterface:

public function getCacheEntryIdentifier(): string

With this pr 8336726 the method was removed from the Neos 9-dev Node (in preparation for standalone).

A few days later Neos.Caching.entryIdentifierForNode was introduced 62555a1

And code was rewritten to use this new helper

  @cache {
     mode = 'cached'
     entryIdentifier {
-       documentNode = ${node}
+       documentNode = ${Neos.Caching.entryIdentifierForNode(node)}

@mhsdesign
Copy link
Member

While on it i would also like to move it to Neos\Neos\Fusion\Cache as well? :D (but could do so as well :D)

@mhsdesign
Copy link
Member

I know this pr is kindof exploding with comments relative to the lines changed but imo thats a good part. For example until now i was not really aware of this breaking change from 8.3 and so with this in mind we could possibly back-port Neos.Caching.entryIdentifierForNode to make live easier ;)

Also to better fit the fusion pattern: being rather short we could also consider using Neos.Caching.entryIdentifier(node) or implement magic so people wont have to use this helper at all? Or implement a flowquery.

Imo fusion is the part were we should put much dedication into as well, that means knowing all breaking changes and designing a well suited api. (The original change was obscured in a 148 additions and 71 deletions change set which would not even make it to auto generated release notes)

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.

On the other hand Neos.Caching.entryIdentifierForNode vs Neos.Caching.entryIdentifier doesnt make that big of a difference and would cause just extra work right now.

I think we can keep it and i took the time to create a list of things like this, which have to be migrated manually and possibly backported to 8.3

neos/rector#67

@dlubitz dlubitz merged commit 8cdcc5f into neos:9.0 Jun 14, 2024
9 checks passed
@dlubitz dlubitz deleted the 90/bugfix/node-cache-entry-identifier-workspace-aware branch June 14, 2024 11:58
mhsdesign added a commit to mhsdesign/neos-development-collection that referenced this pull request Jun 23, 2024
The change was taken too lightly and the whole content cache flushing depends on the assumption that after publishing or discard, everything will be rendered.
With this knowledge and the added test we should be able to refactor it correctly in the feature without introducing a regression again.
mhsdesign added a commit that referenced this pull request Jun 23, 2024
…amid-to-NodeCacheEntryIdentifier

BUGFIX: Fix content cache for discard, revert #5083 for now
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.

4 participants