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

[Improvement]: Dispatch event to manipulate the FullPageCache tags #16925

Open
mlumia opened this issue Apr 10, 2024 · 3 comments
Open

[Improvement]: Dispatch event to manipulate the FullPageCache tags #16925

mlumia opened this issue Apr 10, 2024 · 3 comments

Comments

@mlumia
Copy link

mlumia commented Apr 10, 2024

Improvement description

We cannot purge CDN content by cache tag (varnish based CDN e.g. Fastly) when FullPageCache is enabled, unless we flush the entire FullPageCache on purge. And disabling the FullPageCache then forces an un-cacheable Cache-Control header.

We want FullPageCache enabled and handling the Cache-Control header, because this keeps all cache responsibility in the same location.

It would be a huge improvement to allow the manipulation of the FullPageCache tags, because we can then append the response Surrogate-Key/Cache-Tag header tags to the tags used for Cache::save.

https://github.com/pimcore/pimcore/blob/11.x/bundles/CoreBundle/src/EventListener/Frontend/FullPageCacheListener.php#L338-L346)
Screenshot from 2024-04-10 11-37-11

There are 2 possible solutions:

  1. Move the dispatch of the PrepareResponseEvent event after the $tags, and pass the $tags to the event for manipulation.
  2. Dispatch a new event to manipulate the Cache parameters (if you want to keep the responsibility of the PrepareResponseEvent to only handle preparing the response for cache.

In my opinion, option 1 makes the most sense and also a good improvement on the FullPageCache.

@fashxp
Copy link
Member

fashxp commented Apr 12, 2024

Would you also need to remove the default tags outputand output_lifetime. Because I'm not too sure if we should allow manipulating these too as they are crucial for internal Pimcore cache handling and fiddling around with them might have unwanted side effects.
When it is just about adding additional events it might be enough to allow to add additional tags via the prepare response event, wdyt?

@mlumia
Copy link
Author

mlumia commented Apr 15, 2024

@fashxp for varnish tags we don't need to manipulate existing/system tags. We just need to add additional tags.

You can use the prepare response event (as it currently is) to pass the additional tags to the Cache:save $tags. But depending on the setup, the tags header can be X-Cache-Tags, Surrogate-Key or Cache-Tag. And you would need to also consider that sometimes there will be multiple cache tags to allow more tags than the 4kb limit. So it's not so easy to get the tags header value to set the $tags.

The best solution would be to allow the prepare response event to set some tags, and then set the $tags = array_merge($event->getTags(), $tags) to merge them with the Pimcore system tags.

Copy link

github-actions bot commented May 6, 2024

Thanks a lot for reporting the issue. We did not consider the issue as "Pimcore:Priority", "Pimcore:ToDo" or "Pimcore:Backlog", so we're not going to work on that anytime soon. Please create a pull request to fix the issue if this is a bug report. We'll then review it as quickly as possible. If you're interested in contributing a feature, please contact us first here before creating a pull request. We'll then decide whether we'd accept it or not. Thanks for your understanding.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants