Generate own Purge-Cache-Tags header

Yes this was in fact already on my radar for a while (Wim mentioned it before). Highly likely to follow FosCache's implementation.

To be continued...

[cacheability-headers 6bc4a80] Issue #2692523: Replace http.response.debug_cacheability_headers dependency by a proper EventSubscriber, the header now is X-Cache-Tags (in line and compatible with FOSHttpCache and compatible with other modules).
 4 files changed, 132 insertions(+), 54 deletions(-)
 create mode 100644 src/EventSubscriber/CacheTagsHeaderSubscriber.php
 create mode 100644 src/Tests/CacheTagsHeaderSubscriberTest.php
 delete mode 100644 src/Tests/CacheabilityHeadersTest.php

Pushed upstream, expect this for 8.x-3.0-beta4.

Some notes on the implementation decisions I've made:

1. Parameter http.response.debug_cacheability_headers dropped, please drop it from your service.yml's.
2. Header will always be set on the master request. The ability to disable it only leads to confusion and whenever possible external systems are encouraged to go by tag invalidation.
3. The header name isn't configurable either, to foster standardization and reduce confusion risk.
4. It checks if the header is already set, so other modules/eventsubscriber can come before.

More changes, new header named Purge-Cache-Tagsto avoid conflicts with potential future API changes within Drupal core. See: https://github.com/nielsvm/purge/commit/3e9dafeec03fea9bda7c1a54ba2ddabb...

Also now working on 'header plugins', which will allow downstream modules to provide a plugin which defines the header name AND potentially override the formatting of the header value. This way, we reduce (necessary) code for Cloudflare and Fastly.

To be continued.

I mentioned it earlier today, I worked on the concept of headers-via-plugins, Wim's idea!

Just merged the branch I worked on a good chunk of today:

nvmourik ~/src/purge (8.x-3.x)$ git merge header-plugins
Updating b052c21..6188a33
Fast-forward
MFIED README.md
MFIED purge.services.yml
MFIED src/Annotation/PurgeTagsHeader.php
MFIED src/EventSubscriber/CacheableResponseSubscriber.php
MFIED src/Plugin/Purge/Purger/RuntimeMeasurementTracker.php
MFIED src/Plugin/Purge/TagsHeader/PluginManager.php
MFIED src/Plugin/Purge/TagsHeader/PurgeCacheTagsHeader.php
MFIED src/Plugin/Purge/TagsHeader/TagsHeaderBase.php
MFIED src/Plugin/Purge/TagsHeader/TagsHeaderInterface.php
MFIED src/Plugin/Purge/TagsHeader/TagsHeadersService.php
MFIED src/Plugin/Purge/TagsHeader/TagsHeadersServiceInterface.php
MFIED src/Tests/{ => TagsHeader}/CacheableResponseSubscriberTest.php
MFIED src/Tests/TagsHeader/PluginManagerTest.php
MFIED src/Tests/TagsHeader/PurgeCacheTagsHeaderTest.php
MFIED src/Tests/TagsHeader/ServiceTest.php
MFIED src/Tests/TestTrait.php
MFIED tests/modules/purge_tagsheader_test/purge_tagsheader_test.info.yml
MFIED tests/modules/purge_tagsheader_test/src/Plugin/Purge/TagsHeader/ATagsHeader.php
MFIED tests/modules/purge_tagsheader_test/src/Plugin/Purge/TagsHeader/BTagsHeader.php
MFIED tests/modules/purge_tagsheader_test/src/Plugin/Purge/TagsHeader/CTagsHeader.php
NEW src/Annotation/PurgeTagsHeader.php
NEW src/Plugin/Purge/TagsHeader/PluginManager.php
NEW src/Plugin/Purge/TagsHeader/PurgeCacheTagsHeader.php
NEW src/Plugin/Purge/TagsHeader/TagsHeaderBase.php
NEW src/Plugin/Purge/TagsHeader/TagsHeaderInterface.php
NEW src/Plugin/Purge/TagsHeader/TagsHeadersService.php
NEW src/Plugin/Purge/TagsHeader/TagsHeadersServiceInterface.php
RENAME src/Tests/{ => TagsHeader}/CacheableResponseSubscriberTest.php (51%)
NEW src/Tests/TagsHeader/PluginManagerTest.php
NEW src/Tests/TagsHeader/PurgeCacheTagsHeaderTest.php
NEW src/Tests/TagsHeader/ServiceTest.php
NEW tests/modules/purge_tagsheader_test/purge_tagsheader_test.info.yml
NEW tests/modules/purge_tagsheader_test/src/Plugin/Purge/TagsHeader/ATagsHeader.php
NEW tests/modules/purge_tagsheader_test/src/Plugin/Purge/TagsHeader/BTagsHeader.php
NEW tests/modules/purge_tagsheader_test/src/Plugin/Purge/TagsHeader/CTagsHeader.php

The net behavior result should be identical, there's still a Purge-Cache-Tags header for you to leverage and its value will be exactly the same. However, this now allows modules such as fastly and cloudflare to set their own headers (both of them require this) and even override formatting for the header value, but without actually duplicating any more code. So this is a definite win, also because its testing coverage isn't bad (for beta code).

Pushed this just a minute ago, give it a spin!

Oh, and thanks for updating the article and everything, happy that we managed to ditch that container param :).

Full diff at this link btw:

https://github.com/nielsvm/purge/compare/3e9dafeec03fea9bda7c1a54ba2ddab...

review isn't fully accurate, more its 'needs testing' (although I'm confident).

Yes this is actually desired behavior, because many sites won't just have only one purger but can require different ones. For instance if you have a site with acquia_purge clearing Acquia Cloud and in front of that Fastly, you will need the fastly module which requires the header named Surrogate-Keys whereas Acquia Cloud uses the standard one.

Overriding should be possible (I think) by letting the plugin use the same name, but then you risk breaking other reverse proxies/CDNs.

Based on comment #5, I assume there is some reasoning behind the rename from X-Cache-Tags which seems to be based on a review by Wim, but it's not clear to me why you are not following the suggestion from the issue summary:

Just like the Fastly module does, the Purge module would need to generate its own cache tag header (we could use X-Cache-Tags following the style of FOSHttpCache [...]

I found no hints in the diff on Github either.

Although I understand that there is no inherent naming convention here, I don't understand why we are not following the lead from FosHttpCache, but instead inventing our own (Purge-Cache-Tags). I am asking because I work in an environment where we host 3 other OSS PHP CMS'es as well as Symfony webapps, and in this context, standardizing on X-Cache-Tags makes perfects sense. (I realize I can just reconfigure the purger in Drupal to emit this header.)

but the page still gets Purge-Cache-Tags added as well; is that the desired end result?

Yes this is actually desired behavior, because many sites won't just have only one purger but can require different ones.

No, this is very much undesired behavior.

Quoting https://www.drupal.org/node/2592471:

Many hosting environments (and proxies and reverse proxies and CDNs) impose limits on the header size, and for a sufficiently complex Drupal 8 page, the number of cache tags on that page (i.e. the number of things that page depends on) can cause the cache tags header to go beyond 4 KB, causing WSODs.

If Purge is always going to send a Purge-Cache-Tags header, then this problem will continue to exist. You could argue that Purge then simply requires higher limits. Fine. But remember that you will then be sending potentially kilobytes of unnecessary data for every response to all clients. That means a slower TTI.

The rule is very simple: an end user should (in production) never ever ever ever be able to see a cache tags header. Because that is always wasteful (worse performance) and sometimes harmful (WSOD). Therefore you should only send cache tags headers that are stripped by proxies under your control: Varnish, Fastly, whatever.

Based on comment #5, I assume there is some reasoning behind the rename from X-Cache-Tags which seems to be based on a review by Wim, but it's not clear to me why you are not following the suggestion from the issue summary:

Yes Wim and I had a epic long conversation about this and Purge-Cache-Tags is the final result which will be shipped with beta4 by default. There are several underpinning reasons why this became the outcome it is:

1. X-... named headers are deprecated since the issuance of RFC #6648. And I think its better to stick to (updated) standards.
2. Names like Drupal-Cache-Tags imply that Drupal core issues the header and that its the de facto header to use. Although I'd love this solution, Purge remains a contrib module that could theoretically compete with other modules doing similar things, so it has to include some level of namespacing.
3. Beyond the previous bullet, the header shouldn't conflict with core's debug header.
4. I agree that the lack of consistency hurts, but it is perfectly doable to set differently named headers and/or rely on stock defaults.

Yes this is actually desired behavior, because many sites won't just have only one purger but can require different ones.

No, this is very much undesired behavior.

Let's agree that 'desired behavior' isn't the right selection of words at all.

No it isn't desired, but it is unavoidable for a project aiming to support complex configurations with multiple downstream caching platforms. Therefore, I stand by the necessity of having multiple headers only if and when the situation requires this! For instance Fastly mandates a specific header, but homegrown Varnish installations can simply run a VCL configuration dictated by Drupal/Purge.

That said, I do totally agree that these headers should NOT leak through to webbrowsers and that each caching layer outside of Drupal should remove it. This is out of our control, but definitely something I will emphasize in documentation once I get to that.

This is out of our control, but definitely something I will emphasize in documentation once I get to that.

Except that it's partially within Purge's control… If Purge stubbornly always sends a particular header that no intermediary will remove, then it will be sent to the end user. Therefore it is essential that Purge does not send any header by default, but instead only sends a header for every intermediary it purges. That's right: the tag header should be configured at the same time as you configure a particular purger, because that's how you interact with said purger.

If multiple purgers end up using the same header (which of course requires the first intermediary to not strip the header), then that's even better. But that is going to be a rare case.

I agree with @nielsvm on this subject, Purge should send the headers by default, and they should be removed when needed within the cache solution. If only headers should be sent when it purges - I find it hard to get something like Varnish even working with drupal cache-tags.

If only headers should be sent when it purges

That's not what I said.

I said it should only send headers for those purgers that have been configured. It should not ALWAYS send headers. Furthermore, different purgers need different header names and values, so a single solution can't even work.

Ok, then I misunderstood, So your suggestion is to solve this inside the different purges (like the varnish purge we have started to work on as a fork on the generic http purger), and that you must enable the headers inside a custom service.yml ? So you have no headers outputted by default?

Yes, no headers by default. No, you wouldn't have to enable headers in a custom service.yml file; enabling a purger would cause the necessary header to be generated automatically.

Ok, now I am in the game, yes that should be the case. I will look into that and see if I could do a patch for this in the next few days.

Having thought about this a little more thoroughly, I can say that I agree with everyone here despite the fact that I remain of the option that this code should have a place somewhere. Therefore I decided to move this header to the purge_purger_http project, where it will even be a submodule so that users of that project can truly configure themselves a custom scenario without enforcing this onto other configured pipelines where the submodules take care of this already (as it should be).

In all my thinking, these were the pros/cons I considered:

Upsides:

• Keeps the purge core truly technology-agnostic (no HTTP assumptions at all)
• Moves to a submodule which really is designed for "custom setups".
• One less submodule to a already quite "submoduled" core project.

Downsides:

• Harder to find and less central

So as of purge 8.x-3.0-beta5, this code will disappear and create a regression (its still in beta however). Upcoming version 8.x-1.0-beta4 of the purge_purger_http subproject, will include it as submodule purge_purger_http_tagsheader which users can enable at will, or keep disabled. My change notice will clearly explain this and why it happened.

Unless this decision creates further debate, I'll leave this as fixed.