Skip to content

BUGFIX: Adapt cache flushing to Neos 9 cache flushing#27

Merged
mficzel merged 2 commits intomainfrom
bugfix/cache-flushing
Jan 7, 2025
Merged

BUGFIX: Adapt cache flushing to Neos 9 cache flushing#27
mficzel merged 2 commits intomainfrom
bugfix/cache-flushing

Conversation

@dlubitz
Copy link
Contributor

@dlubitz dlubitz commented Dec 30, 2024

Fixes #26

@dlubitz dlubitz self-assigned this Dec 30, 2024
Copy link
Member

@mficzel mficzel left a comment

Choose a reason for hiding this comment

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

According to the composer.json the package still supports Neos 8.0 and 9.0. That is why we probably should intercept the following methods of the ContentCacheFlusher:

  • addTagToFlush (with a deprecation notice for removal after 8.x support is dropped)
  • flushTagsImmediately
  • collectTagsForFlushOnShutdown

AFAIK the aspects react gracefully when a join point does not match so we still can support Neos 8 and 9 from a single version of FullPageCache.

@dlubitz
Copy link
Contributor Author

dlubitz commented Dec 31, 2024

Hmm, but this will not work. The point in time when the tags are actually get flushed have changed. In Neos 8 it was always on shutdown. In Neos 9 its either immediate OR on shutdown.

If we do not match this, it could lead to unwanted cache entries (re-written cache with outdated data).

I would prefer to make it easy and make this version Neos 9 only. But we could make it downwards compatible by checking if the property "tagsToFlush" exists within the "onShutdown" aspect.

@mficzel
Copy link
Member

mficzel commented Jan 2, 2025

Looked again and you are right. The single aspect on flushTagsImmediately is totally fine for Neos 9 as this is called in shutdownObject aswell. So aspecting that single method also gets the timing right.

For Neos 8 i suggest to keep the code as previously implemented but use the @Flow\Before("method(Neos\Neos\Fusion\Cache\ContentCacheFlusher->commit())") instead of @Flow\Before("method(Neos\Neos\Fusion\Cache\ContentCacheFlusher->shutdownObject())") in the pointcut. That will never be triggered in Neos 9 and can be deprecated for removal once Neos 8 support is dropped.

@mficzel
Copy link
Member

mficzel commented Jan 2, 2025

Hmmm ... if the ContentCacheFlusher would trigger a signal for the flushed tags we could get rid of the aspect entirely. That would make sense api wise as this is also needed by Varnish or CloudFlare adapters.

The last thing that would need a "proper" solution then would be the extraction of cache metadata that is currently done by the metadataAwareStringFrontend which does weird things to work around the fact that during reading we have no way to get the remaining lifetime and tags of a cache item.

@dlubitz
Copy link
Contributor Author

dlubitz commented Jan 2, 2025

Yes, the signal would make sence, but would also be a breaking change and only work for 9.0, right?

@mficzel
Copy link
Member

mficzel commented Jan 2, 2025

yap ... and also it only makes sense with a way to get the needed metadata from the FusionContentCache in a clean way (Probably also via Signal) and not by adding a custom cache-frontend.

Probably way too late for 9.0 maybe we can introduce the needed apis in 9.1.
For now i think we should keep this compatible with ~8.0 || ~9.0

@dlubitz
Copy link
Contributor Author

dlubitz commented Jan 2, 2025

Ok, I'll adapt this PR accordingly.

@dlubitz
Copy link
Contributor Author

dlubitz commented Jan 3, 2025

@mficzel Done. Woul be great if you can test it with a Neos 8 instance.

Copy link
Member

@mficzel mficzel left a comment

Choose a reason for hiding this comment

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

Looks fine and i can confirm that the tag-flushing still works in Neos 8 (tested 8.3)

@mficzel mficzel merged commit f32229d into main Jan 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Package does not work with Neos 9 cache flusher

2 participants