Problem/Motivation
When the http.response.debug_cacheability_headers container parameter is set to true, Drupal will add X-Drupal-Cache-Tags and X-Drupal-Cache-Contexts headers to the HTTP response, so that you can see/debug what tags and contexts have bubbled to the response. It's possible that the number of tags (or contexts, though less likely) on a page can be so numerous that the length of the header lines exceed the ~8k line length limit enforced by Apache. If this happens, Apache will show a blank page with a 5xx status code. It can be difficult for developers to debug what has happened, because there will be no entries in the Drupal log nor any exceptions or errors at the PHP level. The only evidence usually is an opaque error message in the Apache logs.
Generally this issue is limited to local or dev environments where http.response.debug_cacheability_headers is purposefully set to TRUE for development, but it can be possible for this setting to make it to production, either mistakenly or intentionally.
Note: a previous version of this IS mentioned CDNs and reverse proxies not being able to handle large cache tag headers. Issues with those should be addressed in the contrib or custom modules that provide the integrations, since every provider has different methods, whether through other custom response headers or through API requests, to do cache tag invalidation. This issue only addresses the X-Drupal-Cache-* headers used for debugging and development.
Steps to reproduce
Enable http.response.debug_cacheability_headers: true and look for a page with many related cache tags so the X-Drupal-Cache-Tags header is larger than 8K. On servers running Apache, the response will be a WSOD with a 5xx status code.
Proposed resolution
- Limit X-Drupal-Cache-Tags and X-Drupal-Cache-Contexts headers to 8K.
- If any header is bigger than that split the value into multiple instances of the header. Apache will usually be configured to merge the headers back together on output
- Apply this to any headers that potentially exceed the maximum length.
Update documentation.
Remaining tasks
Review and comment on what was done to review this issue and the MR.
Resolve issues on MR
Issue summary update. Does this need an release note snippet
Release note and/or change record seem unnecessary. The fix does not affect anything functionally or visually.
What Documentation needs to be updated? Is it the list in #130?
See #169:
If anything:
one or both of the following would suffice https://www.drupal.org/docs/drupal-apis/render-api/cacheability-of-rende...
and https://www.drupal.org/docs/8/api/responses/cacheableresponseinterface#d....Something like "Note that on some servers, such as those running nginx, the headers could be split across multiple lines with the same name if there are a lot of tags or contexts. On those running Apache, the tags or contexts will be on a single line, but may be separated by commas at intervals of around 8000 characters in length."
User interface changes
None.
Introduced terminology
None.
API changes
None.
Data model changes
None.
Release notes snippet
| Comment | File | Size | Author |
|---|---|---|---|
| #161 | 2844620-161-11.2.8.patch | 7.32 KB | artematem |
| #156 | 2844620-156-11.2.5.patch | 7.29 KB | tostinni |
| #149 | 2844620-149-10.5.x.patch | 6.71 KB | jurgenhaas |
| #142 | 2844620-nr-bot.txt | 90 bytes | needs-review-queue-bot |
| #139 | 2844620-nr-bot.txt | 161 bytes | needs-review-queue-bot |
Issue fork drupal-2844620
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #3
wim leersComment #4
dawehnerIt is a setting, right? I wonder whether the status page, similar to clean URL detection could maybe send out a couple of HTTP requests to test the size we support?
Hardcoding to some limit feels a little bit weird to be honest.
Comment #5
adam.weingarten commentedSo partners from Fastly, Akamai, and CloudFlare are asking for a standard so that they can ensure that their systems will reliably work with drupal. Tons of people also use varnish.
If we leave it variable then this could get very messy. We need to come up with a number that people can expect to work. We could make it overridable via settings.php, to address the concerns that you are raising, but that should be exception.
Also if the headers exceed a number like 16k something is really wrong with your content model!
Comment #6
wim leersI like that!
This.
Comment #7
adam.weingarten commentedUltimately we need to define interoperability standards with respect to headers so external systems can be built to work with Drupal.
Comment #8
wim leershttps://www.drupal.org/docs/8/api/cache-api/cache-tags#reverse-proxies already documented this; and I made it more explicit/clear just now: https://www.drupal.org/node/1884800/revisions/view/10334962/10480147
Comment #9
dawehnerThank you @Wim Leers for clarifying the docs!
Comment #11
fgmJust got bit by this today, while working on a site without even a reverse proxy: a page had 9kB of the single X-Drupal-Cache-Tags header (enabled for debug), which will probably translate to a comparable header length once converted for Varnish xkey.
The problem in this case happens in Apache, before touching the RP itself, as can be demonstrated by this short fragment outside Drupal itself to avoid side effects:
The problem here is that one can debug what's going on and step to the last execution line without any error, and still get a 500 in the client. Worse, there is no information in the PHP error log because there is no PHP error involved, and no information in Drupal logs, because it is not strictly Drupal-related either, so it is very hard to pinpoint indeed.
So that double limit should probably also be enabled and documented. Being a per-environment information, it should probably be a setting, with reasonable defaults like 8000/line, 16k for the whole set. And when caught, should probably truncate or drop/replace the offending header AND log and error, not even a warning.
Comment #12
fgmFWIW, here is an example of a quick workaround I added to solve the issue. It is not really good since it means the header will be wrong, but at least it prevents the 500 and logs an error.
Also, converting to a bug against current version as it is poised to occur more and more often as sites get more complex every day.
Comment #13
regilero commentedThe HTTP RFC has nevers contained an official limitation on the size of one header, just this note:
And in 2.5 we have this very generic sentance:
But for decades HTTP servers have hard coded security limits on length of headers. And 8k has been the hard coded are-you-crazy-or-what limit in Apache httpd.
Trying to reach 16k seems strange, I'm pretty sure a lot of WAF would silently remove http messages using such headers.
Comment #14
fgmThere are apparently two different limits involved:
I'm not sure what your point is about "trying to reach 16kB" and WAFs: in most cases, the configurable limitations are on request headers, not response, and the problem we have with cache tags is about responses. Complex pages can quite easily go above either (or both) limits.
Comment #15
regilero commentedLimits are usually shared also by responses. I once reached nginx buffers limits by settings a big CSP header on responses. On this case it was fixed by altering the buffer limit, but as stated earlier you cannot always alter settings on cloud hosting.
Comment #16
fgmIn Apache at least, they are separate: the request limit is configurable, while the response limit appears not to be.
Comment #19
mr.baileysMarked #2954339: X-Drupal-Cache-Tags / X-Drupal-Cache-Context header tags can exceed HTTP Server limits and cause WSOD as a duplicate of this one.
Comment #20
vaza18 commentedThis patch is taken from X-Drupal-Cache-Tags / X-Drupal-Cache-Context header tags can exceed HTTP Server limits and cause WSOD issue which was closed as a duplicate of this one.
The patch is actually doesn't give a warning but removes an issue at all by splitting too long headers into pieces. Considering those headers are used only for development purposes this looks working solution for me.
Comment #21
vaza18 commentedComment #22
fgmPatch is interesting. However, it has two issues:
Comment #23
mr.baileysAside from the concerns in #22, I'm not convinced that the suggested approach of splitting the headers will actually work, as some proxies (like nginx) have a single limit for the request line and all headers combined, not for each header line individually.
Comment #24
trevorbradley commentedJust a note that the patch in #20 just fixed an absolutely infuriating issue for me that's been plaguing me for months.
I'm working in a docker environment, where a rather large search_api_solr page request was being passed to proxy_fcgi. On smaller pages, the page would render just fine, but on larger ones, the web server would throw an error 500 with a "Premature end of script headers". This was only on my local docker dev environment - on a full apache deploy, there were no issues.
I finally watched the debug output, and noted that the fcgi proxy server was generating the page perfectly. It would attempt to send headers then die when trying to send the response. I dug deeper into the headers and found a monstrosity in Response.php: there was an 'x-drupal-cache-tags' header that was 12581 characters long!
I droped patch #20 into place to see if the issue would go away, and the page loaded perfectly.
Now, there's likely an issue independent of this one - should this particular header be so big? My page is assembled from hundreds of paragraphs, taxonomy terms and nodes, but should that generate such a large header?
Comment #25
jimmynash commentedI was also experiencing this issue in a docker environment on mac (Docksal).
Applying the patch in #20 did the trick for me.
Comment #26
berliner commentedSame situation as #25 and I can confirm that the patch fixes the issue. Didn't look at all at what the patch does though.
Comment #28
sittard commentedFor others experiencing this issue we noticed our development.servives.yml file had the following configuration:
http.response.debug_cacheability_headers: trueChanging this to false sorted the issue.
Comment #29
anybodyI can also confirm this issue in a Plesk driven environment. #28 fixed it for us!
The patch in #20 but I'm not experienced enough to prove the concerns for a logical / technical RTBC. My RTBC+1 is just empirically. At least I can surely confirm we have a bad issue / bug here for sure. ;)
Comment #31
wells+1 for the patch in #20 -- resolved the same issue for me of excessive cache tags in the header. What can done to move this issue forward wrt to the concerns raised in #22 and #23?
Comment #32
hoporr commented#28 fixed this in our server environment (Plesk).
In this case it is a geomap view, that pulled a lot of points (runs fine on local box, but had this error on server).
Comment #33
司南 commentedThis patch should be merge or we can not use the dev site mode.
Comment #34
bappa.sarkar commentedPatch for Drupal 9.1
Comment #35
psf_ commentedThe patch in #20 works for me. My problem is described in the old issue #2954339.
Comment #36
wellsPatch in #34 applies and resolves the issue in 9.x as well. Thanks, @bappa.sarkar!
The concerns in #22 still seem valid and should be considered and addressed. It seems that changing the approach to use repeated instead of enumerated headers would resolve both concerns, correct? I'm going to retarget this to 9.3.x-dev as this feels more like a new feature/disruptive change than a bug fix at this point.
Comment #37
wellsOops -- forgot to also set back to needs work pending follow up on #22.
Comment #38
oheller commentedI'm using php7.4, Drupal 9.2.1 with docksal. None of the above seem to work. I've set all `http.response.debug_cacheability_headers: false` cleared the caches, applied patch 34 and the latest patch from 2954339, restarted docksal, restarted docker.
I've also added .docksal/etc/apache/httpd-vhost-overrides.conf with
And I continue to get the proxy_fcgi:error.
Any further suggestions?
Comment #39
naveenvalechaI'm using https://www.drupal.org/project/remove_http_headers module to strip out the headers on 9.2.3 and its working fine for me.
Comment #40
oheller commentedThanks. It is an issue with the pantheon_advanced_page_cache module, enabled locally. I'm not sure why on this site when we have it enabled on many others. But hopefully this will aid in anyone else having this specific issue.
Comment #41
ronchica commentedThank you @oheller. I just experienced this issue and it was being caused by pantheon_advanced_page_cache. The issue seemed to only occur on pages with lots of content/paragraphs.
Comment #42
niles38 commentedNone of the above patches, modules, or other suggestions worked for my site. It's 500ing on an edit node page with a lot of complex paragraphs.
Comment #43
sittard commented@niles38 - For others experiencing this issue we noticed our development.services.yml file had the following configuration:
http.response.debug_cacheability_headers: true
Changing this to false sorted the issue.
I think this removes the extra debug information from the HTTP headers allowing it to be < 16kb.
Comment #44
niles38 commentedThanks for your reply @sittard. Unfortunately, that didn't work for me. I'm going to continue to work on this today (hopefully using x-debug). It only happens on edit node with a few types of paragraphs.
Comment #45
niles38 commentedI finally figured out our issue. It turns out it was the Purge module. The patch on this ticket works for us: https://www.drupal.org/project/purge/issues/2976480 . Hope this can help someone else :)
Comment #47
bappa.sarkar commentedPatch for drupal 9.3.0
Comment #48
Steven McCoy commentedComment 47 had a typo. Missing a $ on $cache_contexts
Comment #49
charginghawk commentedComment #50
stephane888 commentedPatch #48 works fine for me, thanks.
Using Drupal 9.3.0
Comment #51
abrar_arshad commentedPatch attached for Drupal 9.3.3
Comment #52
michaellenahan commentedI notice that drupal 9.3 introduced some caching improvements ... see https://www.drupal.org/node/3225328
Since upgrading to 9.3, I am not seeing the problem with the 5XX errors any more, so I am finding I don't need this patch any more ...
Interested to know if anyone else out there has the same experience ...
UPDATE ... sorry, it seems we are still getting the 5XX errors and need the patch still ...
We get these errors on our local dev environments and on our ci-runner (not on dev/stage/live environments) ...
Comment #53
mxr576Okay, it looks like I was trying to solve the same problem in #3268305.
@michaellenahan out of curiosity, can you test the latest MR from #3268305? Although, how #51 splits header values should be better than I did.
Comment #55
mxr576Patch #51 was added to my MR. Added some code quality improvements. Needs test coverage?
Comment #56
mxr576Comment #57
mxr576Comment #59
anybodyCame here as I had the same issue with
http.response.debug_cacheability_headers: trueenabled.@mxr576 what is merge request !1943 based on? Which is the latest patch to review to get this committed?
Comment #60
mxr576@Anybody, according my first commit in the MR, it is based on #51? (I would not remember without that commit :) )
https://git.drupalcode.org/project/drupal/-/merge_requests/1943/diffs?co...
Comment #62
duneblmr !143 apply and solve my problem on D9.5
Many thanks!!!
Comment #63
simobm commentedmr! 1943 Worked for me too on 9.3.22. Thanks!
Comment #64
smustgrave commentedMoving to NW for the test cases before reviewing.
Comment #65
aiphesHello
I'm facing off this on D9.5.3, do I need to patch ? if yes, which files ?
Thanks
Comment #66
dunebl@aiphes : you must look at the mr !1943 (see #57 for the link). From there, you can download the patch (in the upper right part of the screen see Code > download email patches)
Comment #67
aiphes@Dunebl, so this is the patch https://www.drupal.org/files/issues/2021-12-09/2844620x93.patch , and in which folder do I need to apply it ?
Comment #68
dunebl@aiphes this is not the last/full patch. Download it from the explanations I provide in #66 then apply it from within your Drupal root.
Comment #69
charginghawk commentedHere's a link to the MR:
https://git.drupalcode.org/project/drupal/-/merge_requests/1943
And here's a direct link to the patch:
https://git.drupalcode.org/project/drupal/-/merge_requests/1943.diff
If you manage patches using composer and the cweagans/composer-patches library, it's best practice to download the diff and use it as a local patch file, rather than reference this link. That's because attackers could conceivably update this MR to introduce vulnerabilities, and then you'd just automatically pull them in during composer install.
Comment #70
aiphes@DuneBL, ok, can I rollback if something goes wrong ? because I need to patch a production website... if yes, what the command to roolback ?
@charginghawk coudl I use your patch link to apply it ?
Comment #71
duneblYou can unpatch with this command:
Comment #72
aiphes@DuneBL & @charginghawk
Thanks for your help, patch seem to fix my random 500 error on editing certain content types. Hope this is durable. I will do the same on my others D9 websites.
Will I need to repatch after core updating in future ?
Comment #73
dunebl@aiphes: yes, you will need to repatch after each core update. (you can use composer and add the patch in composer.json)
Comment #74
aiphes@DuneBL for sure, but how can I do that ?
Comment #75
duneblA quick search leads me to this article: https://duvien.com/blog/how-apply-patch-file-composer-based-drupal-89
Comment #76
aiphesok, but I get errors of syntax:
Comment #77
rgpublicUm, sorry to interrupt the party, but I'd like to note that usually the patch is not needed for live websites. Make sure the development mode is not active (i.e. the line "include $app_root . '/' . $site_path . '/settings.local.php';" is commented/not enabled in sites/default/settings.php). Development mode should not be enabled for live websites! If you run into this problem with development mode enabled, make sure "http.response.debug_cacheability_headers" is set to false in sites/development.services.yml. Make sure you clear the cache after any changes in these files. These things should usually avoid this error - no need to patch anything.
Comment #78
aiphes@rgpublic So you tell that we doesn't need to use settings.local.php on production websites ?
Because I do this every time, whithout issues except this one after migrating from D6 or D8 to D9.
Comment #79
rgpublic@aiphes: Exactly. You can open and read that file if you don't believe me. You will find that contains only things that are needed for *development*. It will significantly slow down your live website. It disables caches, enables debugging headers and so on. It will also leave your site vulnerable to some attacks. You *can* run with this enabled. But it's not recommended.
Comment #80
aiphes@rgpublic ok, I will plan to use the settings.php and see after the future update ( that will drop the patch) , my 500 errors still goes out. Finger cross.
Comment #81
aiphes@DuneBL Trying to rollback and it doesn't work
/sited9$ patch -p0 -R < "https://git.drupalcode.org/project/drupal/-/merge_requests/1943.diff"orgive
-bash: https://git.drupalcode.org/project/drupal/-/merge_requests/1943.diff: No such file or directoryComment #82
dunebl@aiphes I don't think that the patch command works with https. You should use the real patch file in your local hard drive
Comment #83
aiphes@DuneBL But I don't know where are stored patches. I've nothing in the ~/patchs folder :/ I go to ask to my web host.
Comment #84
duneblThere is a misunderstanding: you must download by yourself the patch file and put it in the HD of your web host. After that you can run the command to apply or un-apply this patch
I am a little bit annoyed because I think we are polluting this thread with other concerns
Comment #85
aiphesTotally right :) ;)
Comment #86
aiphes@rgpublic after done an update
- Upgrading drupal/core (9.5.3 => 9.5.7)and disable settings.local before, it seem to work without 500 errors on known pages. Your tips seem to be the real fix :)Comment #87
marios anagnostopoulos commentedYou should have used it like so
https://git.drupalcode.org/project/drupal/-/merge_requests/1943/diffs.patchPreferably download it and use it as a local patch in your composer.json since, if the code in the pr changes, the patch will change as well with your next composer install/update whatever, and that might be something you don't want to happen uncontrollably.
I uploaded a patch you can use, and it will also be useful (probably) for allowing drupal.org test suite to run.
Comment #88
aiphesThanks Marios, but I don't understand anymore. Some says patch isn't a good way, prefer using settings.php only, others say patch is needed. :/
Comment #89
charginghawk commentedGreat discussion about patches, but let's please, out of respect for the 87 people following this issue, stop talking about patches here, and do not even reply to say "I agree" or "I understand" or something. We're done talking about patches! Thank you! Direct any further patch discussion here:
https://www.drupal.org/docs/develop/git/using-git-to-contribute-to-drupa...
https://www.drupal.org/node/1399218/discuss
Regarding settings.local.php and development.services.yml (which, rgpublic is correct, should not be enabled on production environments!), please refer to the files themselves and you can discuss it on the "Disable caching" page:
https://git.drupalcode.org/project/drupal/-/blob/10.1.x/sites/example.se...
https://git.drupalcode.org/project/drupal/-/blob/10.1.x/sites/developmen...
https://www.drupal.org/docs/develop/development-tools/disable-caching
https://www.drupal.org/node/3340861/discuss
You can also always go to Forums, drupal.stackexchange.com, Slack to get answers to your questions:
https://www.drupal.org/community/contributor-guide/reference-information...
My next comment here is going to summarize the state of this ticket and hopefully get us back on track.
Comment #90
charginghawk commentedThe most recent MR/patch is still mxr576's, !1943, from 8 March 2022.
https://git.drupalcode.org/project/drupal/-/merge_requests/1943
It is reviewed by the community (#62, #63) but it needs tests (#64).
Comment #91
dynamdilshan commentedHi, I recently upgraded my site to D9.5 and I'm getting this error on some of my pages. So far I tried the following:
1) Created a patch from 1943 MR and applied as a local patch
2) Updated the `http.response.debug_cacheability_headers` to `FALSE`
None of these above works for me. The errors I see apart from the WSOD can be found below:
Any idea what else I can do ? Thanks in advance.
Comment #92
charginghawk commented@dynamdilshan The current MR deals with limiting the X-Drupal-Cache-Tags and X-Drupal-Cache-Contexts headers. With http.response.debug_cacheability_headers set to false, these should not be an issue anyway. You can use xdebug or other debugging tools to step through the code in the patch and confirm whether or not those headers are limited or even need limiting.
I will note that other headers (including from the fastly and pantheon_advanced_page_cache modules) can get out of control and the current MR doesn't account for them (though you could maybe argue it should). Please refer to the recent comments on this related Docksal issue:
https://github.com/docksal/docksal/issues/1110#issuecomment-1216488250
Comment #93
dynamdilshan commentedThanks @charginghawk for your reply. Due to some reason this patch and the http.response.debug_cacheability_headers set to false not working for me. I put this below code on index.php to debug and get the size of my headers
And currently nuking all the cache tags on local is the only way for me to get the page working. I found found out some of my pages giving more than 8k size headers.
Comment #95
tlilleberg commentedI was having a similar issue but it turned out not to be an issue with core. My enormous header was edge-cache-tags from the akamai module. Disabling the purge and akamai modules on local took care of it.
Comment #96
herved commentedHere's a patch version of MR 1943 (latest commit bbcb4ac7), if anyone else needs it.
PS: our setup enforces static patches and MR diffs are not allowed (as they can change unexpectedly).
Comment #97
samlerner commentedI was able to reduce the number of tags dramatically by updating the cache tag blacklist with all the options listed here: https://www.drupal.org/docs/contributed-modules/akamai/cache-tag-purging
On some pages it reduced the number of tags from ~600 to ~100. Still a lot, but definitely an improvement.
Comment #98
flyke commentedThank you, this fixed a problem in a project that was recently updated to Drupal 10 where I was suddenly unable to edit the homepage.
I could revert the revision of the homepage and then edit it. But after a while editing would again not work and again the /node/xxxx/edit page would return an error 500. This patch (the diff from the issue fork) fixes it in my D10 project.
Comment #99
umac_de commentedSame here after update of project to 10.1.7
Updated the `http.response.debug_cacheability_headers` to `FALSE`
+ patch #87 fixed it
Comment #100
tunicI tried to clarify current state in the issue body: we need test to move this to RTBC.
The MR should be used, ignore patches.
The patch is working for me in a real project.
Comment #101
aiphes#87 works on D9.5.11 core .
Comment #102
damienmckennaRan into this recently loading the fields settings page for a content type after http.response.debug_cacheability_headers was set to "true" via the development.services.yml file; the site runs on ddev, Safari showed the site giving a 500 error and nothing I could do would give me an explanation of what was going on.
Comment #105
b_sharpe commentedConsidering this is still an issue in D11, I figured we should move it forward. Added tests and new MR for 11.x, back to review
Comment #106
b_sharpe commentedComment #108
smustgrave commentedAdded some nitpicky change for the tests.
Leaving in review for additional eyes.
Comment #109
tunicChanges loosk good to me. However, it would be great if we can run the tests only without the patch to confirm the test works ok (fails when no fix has been applied, passes when it is applied). I tried to run the "Test-only changes" stage at https://git.drupalcode.org/issue/drupal-2844620/-/jobs/1342592 but I says:
Comment #110
b_sharpe commented@tunic this was previously run, but I guess retriggered on rebase. For access I think you just need to request access to the fork and then sign in to gitlab.
Either way, here's the pass on main, and fail on test-only run: https://git.drupalcode.org/issue/drupal-2844620/-/pipelines/148275
Comment #111
tunicAh, of course, I forgot to ask for access rights.
Issue is addressed, code issues have been solved, test-only patch fails as it should and tests passes with patch. I think this can be moved to RTBC but I'll wait for more reviewers.
Comment #112
tunicUpdating solution.
BTW, do we need a change record? I would say no because it is not a big change, but the fact that headers have a number added if they are too big bugs me (X-Drupal-Cache-Contexts -> X-Drupal-Cache-Contexts-1, X-Drupal-Cache-Contexts-2).
Change record or not, the following documentation pages would need a quick update to reflect headers can be split:
Comment #113
b_sharpe commentedGiven this is a debug-specific change, I don't think a change record here is necessary; however, updating any docs referring to debugging headers is definitely a good idea.
Comment #114
o'briatOther doc page need also to be update https://www.drupal.org/docs/drupal-apis/cache-api/cache-tags-varnish, https://www.drupal.org/project/purge, https://www.drupal.org/project/varnish_purge
Comment #115
tunic#114, I think those are related but different headers. In this issue we are handling the headers X-Drupal-Cache-Tags and X-Drupal-Cache-Contexts that are sent by Drupal core when the cacheability debug functionality is on (http.response.debug_cacheability_headers: true). However, the links you provide refer to the Cache-Tags and Purge-Cache-Tags headers (each purger module uses one or the other).
While the information they carry is similar (the cache tag information) and all are affected by the same problem (header too large), the purger headers are not affected by the fix proposed here because that later headers are added by each purger module itself.
It can be interesting to propose a similar fix on the purger modules; however, it will probably be harder for Varnish to process a single header or multiple headers with an incremental sequence (Cache-Tags-1, Cache-Tags-2, etc) so I guess this is something that must be discussed.
Comment #116
o'briatok, sorry for the confusion (TL; DR 😬).
I think purge should just split tags by 16k chunks and make multiple calls to Varnish/CDN.
Comment #117
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #118
b_sharpe commentedComment #119
smustgrave commentedRan the test-only feature https://git.drupalcode.org/issue/drupal-2844620/-/jobs/1943211
And see there are 2 tests but testDebugHeaders isn't failing without the change. So does it need to be tweaked or removed?
Comment #120
b_sharpe commented@smustgrave Sorry I should've been more clear in my description. I added two tests here:
I was going to split this to two issues at first for the initial test, but figured given the longevity of this issue it made sense to keep it here. Back to NR.
Comment #121
smustgrave commented@b_sharpe thanks for the explanation
Left 1 nitipicky comment but overall changes look good. +1 from me.
Going to leave in review for additional eyes.
Comment #122
smustgrave commentedSelf applied 1 nitpicky item.
Comment #123
catchThe title no longer matches the issue summary and MR, can it be updated? Also one comment on the MR.
Comment #125
scott_euser commentedComment #126
scott_euser commentedUpdated to standard issue summary template
Comment #127
prudloff commentedComment #128
scott_euser commentedHa thanks! Clearly my morning coffee hadn't kicked in yet!
Comment #129
smustgrave commentedAppears feedback has been addressed.
Comment #130
quietone commentedI read the IS, comments and the MR. I don't see any unanswered questions. Thank you to @charginghawk for working on summarizing and keeping this issue on track.
There is a large number of documentation links in the comment for places this needs to be updated. Does this need to be documented in so many places? I have not looked at all the pages but this should be documented in one place and the others refer to that one place.
Here are the links I gathered from the comments
I left some suggestions and comments in the MR which need to be looked at so setting to NW.
I also updated credit.
Comment #131
erickbj commentedI just stumbled upon an issue with Cloudflare Purger and created https://www.drupal.org/project/cloudflare/issues/3482279 for that. I then found this issue and I think both are related to some extent. I was hoping the solution of this issue would apply for the Cloudflare Purger
Cache-Tagscase, but I don't think it would work if we're adding numerical suffixes such as "-1", "-2" etc when we have multiple "split" response headers.For Cloudflare specifically their documentation seems to indicate the solution is to have multiple response headers with the same name but different values.
That seems to be applicable to that header as it might for
X-Drupal-Cache-TagsandX-Drupal-Cache-Contextsas those adhere to the comma-separated specification for response header values (although the "X-" ones are purely for debugging purposes to my knowledge, while the Cloudflare PurgerCache-Tagsis functional). See:So the question is, if this solution is applicable for any headers, should we allow some to be added multiple times without adding numerical suffixes?
Comment #132
boazpoolman commentedJust created a patch from the MR so I can apply it in my composer.json.
Comment #134
mr.baileysAs @erikbj also mentions in #131, the official proper way to handle headers with too many values is indeed to split them up into separate lines but with the same header name (see https://www.rfc-editor.org/rfc/rfc9110.html#section-5.2). This would also ensure it remains easy to implement downstream logic basic on the header ("if header contains xyz" is a lot easier than "if header-1 contains xyz or header-2 contains xyz or header-3 contains xyz ...")
So I think splitting the headers but keeping the name is preferable over the current solution appending '-{delta}'. One caveat however: the different header line values are combined with ",", which means the current implementation would have to be converted to generate a comma-separated list instead of a space-separated list (which in turn is not backwards-compatible). On the other hand, this is meant to be a debugging tool, so I don't think this is that big a deal.
Comment #135
godotislateSplitting the header into separate lines with the same header name is going to be difficult, given that the Symfony Response object's headers object doesn't allow for multiple headers with the same key.
Also, if the goal here is only to fix headers for the debug cache tags, having the -1, -2 suffixes probably works just fine. If this issue also needs to address headers generally that are too long (such as the ones used by Purge), the current MR does not address that, and the question of whether and how to split headers into multiple lines with the same name is relevant.
Comment #136
znak commentedHi, everyone
I've tried to apply patch #132 to 11.x version but it does not apply. I updated it and created a new patch for version 10.2.x
Please, check if it's working.
Comment #137
znak commentedComment #138
jwineichen commented#132 is working for me on 10.4.3. Was getting a WSOD on pages with larger views with images results, facets, and having admin toolbar extras installed. Pages load successfully now, without having to limit views result size.
Comment #139
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #140
jason.ullstam commentedPatch from #132 worked for me on a site that was getting 500s. Core 10.4.6.
Comment #141
carolpettirossi commentedMR !7432 applies successfully to Drupal 11 and solves the WSOD on pages with many content/nodes/taxonomies displayed.
Comment #142
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #144
prudloff commentedMerged the latest 11.x.
Comment #145
akz commentedMy two cents on this:
Having said that I strongly believe that it's the servers' responsibility to provide header size configuration options for the developers to use and adjust their envs appropriately. This is not Drupal core's responsibility and definitely not a Drupal core bug. For the cases where server configuration is not an option (yet), yes, a solution should be provided, but not as a core fix, rather than as a contrib module that should be enabled in dev envs only.
Towards this direction I have composed the contrib module Debug Cacheability Headers Split that uses the idea and code provided in this issue. I think this issue should be closed with status "Closed (won't fix)" and the MR should be cancelled.
Comment #146
rgpublicI don't think I'd really agree here, I'm afraid. I came to this issue back then when I was investigating why a Drupal site suddenly showed a WSOD. There was no error message. Nothing. This proved to be extremely(!) difficult to debug. Almost impossible for novice users. That's why I think solving this issue is important. It doesn't necessarily mean automatically breaking the headers into multiple lines. But at the very least a proper error message for the common case (>16 KB) should appear with information on how to solve this (install the module or switch of dev settings).
If you installed such a module, you'd already be aware of such an issue. But as described the problem is more severe if you run into this unintentionally. Of course, people should rather switch off the development settings for production servers - I've already mentioned that in a previous comment. Nevertheless, for some reason, many people have this accidentally switched on. They won't notice any problem but when a page suddenly exceeds the limit (it could even be the start page), the website all of a sudden shows a WSOD with not good option to debug this. This limit can be reached just by doing editorial changes (inserting a view etc) so this can happen to a site pretty unexpectedly.
Of course the limit can vary so I think a relatively low limit (4 KB?) should already trigger a warning. Automatically breaking the lines in addition to the warning would be even better IMHO, because it would keep the site from breaking suddenly.
Comment #147
akz commented@rgpublic I am totally in favor of Drupal core raising a warning when a certain size limit is exceeded. I just do not agree that this is a bug, let alone a core bug. A softer approach perhaps would be to re-categorize this issue as a "Feature request" and let the more experienced Drupal core devs to decide if/how such a warning would be useful in order to be included into core code.
Comment #148
rgpublic@akz: Personally, I'm absolutely fine with reclassifying this as a feature request. It's not really a "bug", right. I just think it's important that we are a bit more helpful to developers and not leave them in this unpleasant situation (white screen) without any hint on what's going on. They can then install the module or do whatever to resolve this, but at the very least they should be informed about this situation.
Comment #149
jurgenhaasUpdated the MR to apply on 10.5.2
Comment #150
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #151
luke.leberI'm not exactly following why the
-1,-2, etc header name enumeration exists.The HTTP spec allows for multiple headers with the same name. IMO changing the name of headers like that might be a new drupalism?
Comment #152
godotislateI'm of two minds about this: by spec, multiple headers with the same name should be implemented as comma-separated values. But these headers are for humans to read in order to debug cache metadata, not for machine ingest, and changing the tags to be separated by commas instead of spaces would make them more difficult to read.
Anyway, the point is that human-readability is more important here than adhering strictly to spec, and I'd say whichever way, numbered headers or no, is easier for more people to understand at a glance is the way to go.
Update issue title, since this is strictly about the core cache debug headers.
Comment #153
nicxvan commentedWhat if we add a special cache tag to the beginning for human debugging.
Something like header_split:1
Then we can follow spec and aid human debugging.
These tags can be used by reverse proxies for clearing right?
Comment #154
godotislateNot really? Reverse proxy services typically have a documented API for clearing, whether that's a REST endpoint or their own HTTP header and expected values, etc.
If you control your own reverse proxy, you could possibly customize it to use these headers for clearing tags, but I think that's an atypical use case. They're called
http.response.debug_cacheability_headersbecause they're specifically for debugging.Comment #156
tostinni commentedRebasing the MR and adding a patch for Drupal core 11.2.5
Comment #157
artematem commentedComment #158
artematem commentedComment #159
artematem commentedComment #160
artematem commentedComment #161
artematem commentedComment #165
godotislateI hit this several times on my work project, which wouldn't be that big an issue, except that for some reason there are a few different development.services.yml files in the project, and every time I have to flip the values of
http.response.debug_cacheability_headersin multiple files before I figure out which one is authoritative.So I did some investigation and some testing changing this line on
mainin FinishReponseSubscriber:to
Using the standard DDEV configuration of Apache:
$line_lengthup to roughly 8000 resulted in responses with status code 200.$line_lengthclearly above 8190 resulted in responses with status code 5xx$line_countcould get quite high, I think I got up to 10 (or 20, don't quite remember), without issue. So at least the total header size in Apache is set highLimitRequestFieldSizehas no effect on the response header line limit, because it is set at16380by default in DDEV. This is unsurprising, but some sources online suggested/guessed that the setting would apply to both requests and responses', 'Using the standard DDEV configuration of nginx:
$line_countat 1, values of$line_lengthup to roughly 32000 resulted in responses with status code 200, and 5xx above that$line_countat 4, values of$line_lengthbelow around 8000 resulted in responses with status code 200, and 5xx above thatThe findings here are consistent with a lot of previous comments that the concern should be about splitting the header lines below the Apache 8K line limit, and not worrying about the total header size on Apache or nginx, since running into that would probably be a result of significant content architectural issues.
Anyway, I have rebased the MR and made the following changes:
-Nsuffixes on the header names, so there will be multiple lines with the same header name. As mentioned, in Apache, this most likely will result in only one X-Drupal-Cache-xxx line output anyway, just with a couple of', 'separators sprinkled inComment #166
benstallings commentedLooks good!
Comment #167
quietone commented@benstallings, what did you do to review this issue and the MR? And there are over 10 unresolved comments on the merge request. Have they been addressed?
There are several steps, or gates, that an issue must pass before it is marked RTBC. For most issues following step 10 in the Review a patch or merge request task of the Contributor guide is sufficient. The complete list of core gates has more topics. Also, check the tags on the issue and make sure they are complete.
Setting to Needs review. I updated the remaining tasks with what I see.
Comment #168
quietone commentedAnd I updated credit
Comment #169
godotislateI thought I had resolved the MR comments, but either I forgot to hit submit back then or I just plain forgot. Updated the MR and addressed comments.
I removed the release note snippet, because I don't think it needs one, but I can add an updated one if needed.
What Documentation needs to be updated? Is it the list in #130?
I'm not sure documentation needs to be updated for what is ending up to be a subtle change, if it does, I'd say one or both of the following would suffice https://www.drupal.org/docs/drupal-apis/render-api/cacheability-of-rende...
and https://www.drupal.org/docs/8/api/responses/cacheableresponseinterface#d....
Something like "Note that on some servers, such as those running nginx, the headers could be split across multiple lines with the same name if there are a lot of tags or contexts. On those running Apache, the tags or contexts will be on a single line, but may be separated by commas at intervals of around 8000 characters in length."
Comment #170
godotislateInvestigated the behavior of Apache appending (or merging?, not sure which since the header line values are different) headers with the same name: I'm not sure what I read previously that gave me the idea that this is default behavior. Looking again, I haven't found anything to substantiate or refute that it is the default setting, with or without mod_headers. Anyway, Apache in my local ddev is appending or merging the debug headers if there are more than one with the same name, but it is not happening on Drupal CI, and I'm not sure what the configuration difference is. I'll revisit whether some comments should be rewritten.
Comment #171
o'briatIs HTTP headers are really the place for these debug info?
Could these info just simply be printed as comments at the top of the body (as other cache and twig debug info)?
Do I miss a real use case where they have to be placed into HTTP headers?
I do understand that other headers containing cache-tags are used for proxy cache invalidation (with the same issue) but that's should be fixed in these contrib modules (ex: https://www.drupal.org/project/purge/issues/2952277)
Comment #172
rgpublic@o'briat: Well, I think it's actually a pretty nice solution. Printing stuff right in the web page has frequently numerous problems. Depending on what styles exist, stuff is cut off, in a font size too small, in the wrong font altogether, in the wrong color and more. What's more it isnt really a solution for non-HTML requests like Ajax Requests etc. Printing stuff there will obviously destroy e.g. a proper JSON response.
This functionality, btw - you can't stress this enough - is opt-in and should only be used in development mode. I think just automatically splitting them across multiple headers if they exceed a certain length is still a solution that makes sense, although an alternative would be to just cut them off and add "overflow" at the end or to the error log, because such a long list is hardly usable anymore, anyway. The main point here is that a WSOD is prevented which is very confusing for developers to understand.
@godotislate: Your comment gave me the idea to checked whether this is actually covered by the HTTP standard:
https://greenbytes.de/tech/webdav/rfc2616.html#message.headers
It seems this is actually allowed (and as far as I could figure out also used e.g. with Set-Cookie or CSP headers), BUT: The list must be a) separated by commas and b) must have the same meaning regardless of whether it is one single entry or split over multiple entries. And c) the order of those entries is relevant. We fulfill b) and c) but I just tested this and our list is separated by spaces currently. Of course there is no standard for our custom header, so we can change this anyway we want. In order to fulfill the demands of the HTTP standard it seems we need to switch the list to comma separated it seems.
Some software along the response path (proxies etc) might merge or mangle this stuff anyway. But it's their responsibility then IMHO to make sure they don't merge stuff to a line longer than allowed etc. As this is an opt-in debugging feature, I don't think we need to take care of that. In the worst case, the multiple lines would still cause a WSOD, we can't do anything about it. But it will solve the WSOD for most developers who would otherweise get bitten by this, so I think it's still worthwhile solving this.
Okay, these are just my thought's on this FWIW.
Comment #173
godotislateAmong other things, it's difficult to do this because Drupal calculates and sets a content-length header by the time the subscriber runs, so adding markup at this point could cut off the output of the response sent by Apache or nginx.
I mentioned this in #165, but since these are debug headers meant to be read by humans on a non-production environtment, and space separation is more legible than comma separation, I think this is fine. (And comma + space separation just uses more bytes). Arguably we are still meeting HTTP standards because each line of multiple contexts or tags could be considered a singular value.
Comment #174
o'briatI meant to display then in HTML comments, but you go a point with json
Comment #175
damienmckennaJust to mention it, but on a site I'm building I have a busy node edit form that was WSOD'ing even though $response->contents had HTML, it turned out to be this bug and the MR fixed it for me.
Comment #176
godotislateOK, did some clean up with comments, and moved the Functional test to a Kernel test since we have #3390193: Add a drupalGet() method to KernelTestBase now. This made the header lines repeatedly separate in my local testing as well.
Updated the IS. I don't think there needs to be a release note or change record, and I'm not quite sure what it would say. Happy to provide one if there are suggestions. Removed IS update tag.
Comment #177
geek-merlinReviewed the code.
Code looks straightforward and good, tests are in place and look reasonably.
Bot is green.
+1 for no release not for such a simple bugfix.
and YAY! for leveraging the new kernel tests!
Comment #178
alexpottWow - it is lovely to see this get fixed - such a gotcha.
Committed and pushed d38a4f4ea13 to main and 47289f37d4d to 11.x and f0cd52cd632 to 11.3.x. Thanks!
Comment #183
alexpottCan't go in 11.3.x not all the kernel test drupalGet stuff has landed there.
Comment #184
alexpottAlso had to do a quick follow-up because
Only works on PHP 8.4 and up... breaks on PHP 8.3. It needs to be
Comment #189
godotislateI was wrong about this. The content length is calculated afterward, so debug information could be added to the response content somehow.
Opened #3589816: Look into alternate way to display debug cache information to look into it.