Change notice: Start relying on Request/Response objects for cache handling

This doesn't really do anything other than remove some global functions in favor of their new object equivalents. That is, it's a Good Thing(tm) on its face.

If the bot's happy, I am.

Per #1855260-39: Page caching broken by accept header-based routing, would this be the appropriate issue in which to discuss HalSubscriber and AjaxSubscriber calling $request->setFormat() within a KernelEvents::REQUEST event? Since all setFormat() does is set a static mimetype map, would it make sense to do it pre-kernel, so that the cache layer can benefit from it?

use_http_objects_for_cache.patch queued for re-testing.

@effulgentsia - this doesn't feel quite like the right place for that - thus far what we've been discussion is really just cleaning ways in which we dodge around request/response by busting encapsulation in one way or another.

Crell, msonnabaum and i chatted about this a fair bit today. i'm gonna take a stab at cleaning up & consolidating the crazy shit we do with headers, and msonnabaum will have a swing at some of our other global violations - asking $_SERVER, and such.

http://drupal.org/node/1427826 contains instructions for updating the issue summary with the summary template.

The summary may need to be updated with information from comments.

this still has quite a few failures locally, but the important thing is that i've castrated drupal_add_http_header() so that it no longer calls drupal_send_headers(); it simply builds up a bunch of headers, which later get attached to the response object. so we're finally relying on symfony to actually send our headers. woot.

i would have completely removed drupal_send_headers(), but it's still being used in install.core.inc. that's on the short list for removal.

generally, this is progress, but the separation of drupal_set_page_cache() and drupal_serve_page_from_cache() is pretty awkward, and merits some refactoring in the context of a situation where we're entirely deferring the sending of headers, and simply queueing them up on the response. but, once we get this green, we could easily deal with that in a followup.

This looks great so far!

+++ b/core/includes/bootstrap.inc
@@ -1303,7 +1312,7 @@ function drupal_page_header() {
  */
-function drupal_serve_page_from_cache(stdClass $cache) {
+function drupal_serve_page_from_cache(stdClass $cache, \Symfony\Component\HttpFoundation\Response $response) {
   $config = config('system.performance');

Do not use the fully qualified namespace here, use use statements instead

+++ b/core/includes/bootstrap.inc
@@ -2156,24 +2162,28 @@ function _drupal_bootstrap_page_cache() {
   if (!isset($_COOKIE[session_name()]) && $cache_enabled) {
+    $response = new \Symfony\Component\HttpFoundation\Response();

same here and in a couple of other places.

So the session and page caching tests are failing, which worries me a bit whether this works at all.

this patch:

• adds use statements, per #12.
• reduces complexity of the Etag (removes compression info from it). this may or may not be right.
• chases after 8.x to accommodate changes to the syntax for the language header.
• switches (back) to what i GUESS is "common form" for http headers - capitalizing the first letter of the header field name, and if there's a dash in it then capitalizing the first letter after the dash, too. most of the time. (UGH)

this patch fixes all the PageCacheTest test failures. however, there are still outstanding issues.

• generally speaking, header "overriding" is a mess. the truth is that some headers can be overridden, and others can't. this inherently became the case as soon as drupal_add_http_header() was castrated and turned into a passive header store. imo, as with pretty much all of our other drupal_add*() globals, this was a shit API in the first place and should not be considered a regression. however, it should be addressed clearly in the followup that takes care of refactoring the caching process to be sane and make sense (i.e., #1597696: Consider whether HttpCache offers any significant benefit over the existing page cache).
• there are a number of assertions in those tests in which we're checking to ensure that Vary: Accept-Encoding got sent. these tests do NOT set system.performance.response.gzip = 1, and as that's the *only* time we set the Accept-Encoding header, i have no idea how this logic even worked before. we don't seem to be seeing the gzinflate exceptions with testbot, though, so i'm imagining that it's being taken care of transparently by the testbot's webserver configuration, the tests are wrong, and we're not actually testing the right thing there. in any case, i've made it explicit.
• we previously were generating an etag based on the request time concatenated with the value of the internal variable for gzip inflation. in my novitiate state, this seems completely wrong, as the presence of Vary: Accept-Encoding ought to already be expressing this information effectively to the client, and it need not be explicitly included in the etag. though of course, #1573064: Remove unique ETag hack as it is no longer necessary hopefully indicates we can remove all this etag nonsense and save it for when it's actually appropriate to use...
• a few of the assertions changed to accommodate different capitalization that results from finally relying on Symfony for sending headers. every instance of such a change represents a test that was inherently broken, as it was performing case sensitive comparison on data that is specified to be case insensitive by RFC 2616. the appropriate solution would be to create a WebTestBase::assertHeader() method that does the somewhat grunty work of a) allowing partial assertions (e.g., will be true if one does something like $this->assertHeader('Vary', 'Cookie')), and b) keeps a correct catalogue of what header field names and values are specified to be case insensitive/sensitive according to RFC 2616, and performs string comparisons accordingly. at least some of this would, i suspect, be eased by using Guzzle...but eh.
• i've converted the bizarro one-off page caching implementation in toolbar module, though i fervently hope it will die later. i have no idea what test failures (or not) it will cause.

the other major step involved here, apart from fixing some of these other test failures, is passing a Request into drupal_serve_page_from_cache() and using it, instead of superglobals, to get request header information.

the patch also includes #2004908: Allow tests to specify that response headers should be dumped in verbose mode, without which it is utterly fucking impossible to work on this, so i've just left it in for now as a convenience to others :) that should go in presently, and we can remove it from here when it does.

Given that this is just step one, and we're planning to change/refactor a lot of the code anyway as soon as this lands, I am totally OK if we address "weird and hard to debug tests that don't make sense" by removing them outright rather than rewriting them. I suspect a lot will become redundant anyway once we start using a wrapping kernel for caching, or be so different that they might as well, so I'm all for making our lives easier.

well i'd try that...cept i don't know which tests are failing, since testbot is choking and not telling us (jthorson said we're hitting #1931574: Update PIFR to use config to set simpletest verbose for D8 tests.). and it was all green locally :( i'm not really sure how to resolve it.

meanwhile, i'll see if we can nail the other two failures. and, we still need to refactor out the $_SERVER calls, swapping in the request object. i wouldn't cry if someone else wanted to take that on :)

+++ b/core/includes/bootstrap.inc
@@ -1115,7 +1119,9 @@ function drupal_get_http_header($name = NULL) {
+ * @deprecated

@deprecated should include a statement of, at minimum, what to use instead.

+++ b/core/includes/bootstrap.inc
@@ -1229,57 +1241,54 @@ function drupal_serve_page_from_cache(stdClass $cache) {
+  // If the client sent a session cookie, a cached copy will only be served
+  // to that one particular client due to Vary: Cookie. Thus, do not set
+  // max-age > 0, allowing the page to be cached by external proxies, when a
+  // session cookie is present unless the Vary header has been replaced.
+  $max_age = !isset($_COOKIE[session_name()]) || isset($boot_headers['vary']) ? $config->get('cache.page.max_age') : 0;
+  $response->headers->set('Cache-Control', 'public, max-age=' . $max_age);

The comment is a bit unclear, as it almost implies at a glance that not setting max-age > 0 is what allows it to be cached by external proxies, which is the opposite of what's intended. I'd suggest:

"Thus, do not set a max-age > 0 (which would imply that the page may be cached by external proxies), when a session cookie is..."

+++ b/core/includes/common.inc
@@ -4667,16 +4669,18 @@ function drupal_page_set_cache($body) {
+      $date = new DrupalDateTime($date);

Why DrupalDateTime here but DateTime elsewhere? And shouldn't it be \DrupalDateTime?

updated patch. i'm incredibly dumb, and missed the fact that i was trying to fix install.core.inc, and the actual problem was in the UPDATE system. god dammit - that's two days burned. now with any luck, we'll actually get testbot to tell us why the page cache test is failing.

@Crell - re: @deprecated - there is no substitute for drupal_get_http_header(), because we haven't designed it yet. but i've added a note saying that all headers are set directly on the response object.

I agree, the comment is unclear. It's also what's already in core :) it's just moved from above in the function. i'm not touching it, and we should not care about it as it will likely go away with a more proper refactor.

the DrupalDateTime is from msonnabaum's original patch. i don't really know how to use it, so i just used DateTime elsewhere because it's what Symfony's Response object explicitly type-hints for. if we just want to get this in with the expectation that we're (immediately) going to be shifting the responsibility outside of the kernel, this seems like a nit not worth mucking with.

i've also included a version of the patch that excludes the header-dumping logic that's over in #2004908: Allow tests to specify that response headers should be dumped in verbose mode.

With regards to test failures this one should do it: Normalize casing of Vary headers.

(Or at least if I would upload the complete patch.)

ok, that's weird. i can't imagine why header casing would vary between my environment and testbot's, especially since that's under symfony's control. and we should maybe consider changing that test to conform to RFC2616, which specifies that both those field names and values should be case-insensitive.

but for now...who cares, it's green! thanks!

so now, just need to do a bit more tweaking to only use a Request, instead of directly accessing the $_SERVER globals in drupal_serve_page_from_cache(). then this should be ready for proper review, and committing. i'll do that now...

ok, refactored to use request, instead.

this was somewhat awkward in _drupal_bootstrap_page_cache(), as it occurs before the request has been put into the container, thus making it impossible to retrieve via any of our global methods. this shouldn't be surprising to anyone, as this it's the problem we've been discussing at length about the degree of encapsulation and ordering of operations in our bootstrap process.

to solve it, i simply dropped a Request::createFromGlobals() into _drupal_bootstrap_page_cache(). this is the same thing we do just a little later to create the request object that's actually used, and is a read-only operation, so should be good enough.

also, i changed PageCacheTest to perform case-insensitive tests in appropriate places on header content. while the fix from #25 did make testbot happy, it broke on my local, and the fact that the RFC dictates that *either* casing approach is valid means that we should not be calling failure in either case. i had previously left it case-sensitive after an internal debate where i landed on the notion that "it's OK for Drupal to perform case-sensitive tests here as what we're testing is not RFC compliance, but 'common form' compliance" (see RFC 2616, section 14.44, "Vary"). however, if we're seeing variations in the actual output across environments, i take that as a strong indicator that we should be testing nothing more than RFC compliance.

i'd say this is now ready for PROPER review, and committing.

+++ b/core/includes/bootstrap.inc
@@ -1220,67 +1230,66 @@ function drupal_page_header() {
   if ($if_modified_since && $if_none_match
-      && $if_none_match == $etag // etag must match
+      && $if_none_match == $response->headers->get('etag') // etag must match
       && $if_modified_since == $cache->created) {  // if-modified-since must match
-    header($_SERVER['SERVER_PROTOCOL'] . ' 304 Not Modified');
-    drupal_send_headers($default_headers);
+    $response->setStatusCode(304);
     return;
   }

What happens with the empty return? We're setting stuff on a response object and then not returning it? That doesn't seem right.

yes, the response object on which the 304 is being set is the same one passed in as a parameter to that way-too-long function. as @msonnabaum points out, the 304 tests would be failing if that didn't work. changing the structure of drupal_serve_page_from_cache() to have it return a response would, imo, constitute a change in the API that is beyond the scope of what we're trying to do here.

@beejeebus pointed out that, with the current logic flow, we can't set cache-control headers on a 304. that's wrong. i now can't remember why i originally moved that logic into the second half of the function (though i have a sinking feeling it was after observing oddities in flow owing to what headers we throw into the 'page' cache bin), but it certainly doesn't seem to belong there.

so if there are immediately obvious issues, hopefully our tests will catch them. if not, then it'll be better to solve those issues with a broader refactor.

note that this is kinda dependent on #2004908: Allow tests to specify that response headers should be dumped in verbose mode, at least for full, proper debugging output. it'd be great if we could get that in (incidentally, it's also RTBC!) as well...

This has a +1 from me as well.

+++ b/core/includes/bootstrap.incundefined
@@ -2066,27 +2074,34 @@ function _drupal_bootstrap_page_cache() {
+  // @todo this is *criminal*. but, necessary, until we fix bootstrap ordering.

Is there an issue for this?

+++ b/core/includes/bootstrap.incundefined
@@ -2066,27 +2074,34 @@ function _drupal_bootstrap_page_cache() {
-      header('X-Drupal-Cache: MISS');
+      drupal_add_http_header('X-Drupal-Cache', 'MISS');

This just got marked deprecated, although I guess it's a step less deprecated than calling header() directly..

+++ b/core/includes/common.incundefined
@@ -4405,15 +4407,15 @@ function _drupal_bootstrap_full($skip = FALSE) {
       'data' => array(
-        'path' => current_path(),
-        'body' => $body,

current_path() and getQueryString() aren't remotely equivalent - why the change? If the change was on purpose then this should be called something other than path. The system path is available here so not sure why it's not using that logic.

+++ b/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.phpundefined
@@ -60,6 +61,7 @@ public function onRespond(FilterResponseEvent $event) {
+    // @todo use $response->setLastModified()
     $response->headers->set('Last-Modified', gmdate(DATE_RFC1123, REQUEST_TIME));

Why not do it here? Would prefer an issue to the @todo if there's some reason not to change it now.

@catch - follow-up re: criminality is, i'd say, #1597696: Consider whether HttpCache offers any significant benefit over the existing page cache. at least, that's how we've all been talking about it - this patch paves the way for us separating out caching into something that's situated outside(-ish) of the kernel. that's the best way to address the ordering problem.

deprecation of drupal_add_http_header() - right, this is the patch that marks it deprecated. while it sucks to have to keep using it, and we want to refactor those things out, fully removing it falls largely under the heading of the first issue - write a saner system that wraps-ish the kernel, and we remove the need for those deprecated functions. in the meantime, however, we're relying on drupal_add_http_header() as the way for "anything" to add headers at anytime without actually sending them - actually making it behave a lot more like the other global drupal_add*() functions. at the end of request processing (in FinishResponseSubscriber), we then deque them onto the response object. this is all because Response is designed to not send any headers at all if they've already been sent, so we have to use this deferring method to avoid calls to header() from our own code.

current_path() and getQueryString() - that's from @msonnabaum's original patch, i'm gonna defer to him on that. though i will anecdotally say that, from what i've observed being dumped into the page cache table with and without this patch...the two look pretty similar.

re: setLastModified() - ahh yes, thanks for catching that, i think i just forgot to finish it off in one of my earlier patches when i was getting frustrated by DateTime. new patch fixes that.

fix the error, it was the relic of a poor rebase after #2004908: Allow tests to specify that response headers should be dumped in verbose mode went in. incorporates #47, and the interdiff is against that.

+++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.phpundefined
@@ -54,16 +54,6 @@
-   * Indicates that headers should be dumped if verbose output is enabled.
-   *
-   * Headers are dumped to verbose by drupalGet(), drupalHead(), and
-   * drupalPost().
-   *
-   * @var bool
-   */
-  protected $dumpHeaders = FALSE;
-

@@ -1201,15 +1191,9 @@ protected function drupalGet($path, array $options = array(), array $headers = a
-
-    $verbose = 'GET request to: ' . $path .
-               '<hr />Ending URL: ' . $this->getUrl();
-    if ($this->dumpHeaders) {
-      $verbose .= '<hr />Headers: <pre>' . check_plain(var_export(array_map('trim', $this->headers), TRUE)) . '</pre>';
-    }
-    $verbose .= '<hr />' . $out;
-
-    $this->verbose($verbose);
+    $this->verbose('GET request to: ' . $path .
+                   '<hr />Ending URL: ' . $this->getUrl() .
+                   '<hr />' . $out);

@@ -1389,16 +1373,10 @@ protected function drupalPost($path, $edit, $submit, array $options = array(), a
-
-          $verbose = 'POST request to: ' . $path;
-          $verbose .= '<hr />Ending URL: ' . $this->getUrl();
-          if ($this->dumpHeaders) {
-            $verbose .= '<hr />Headers: <pre>' . check_plain(var_export(array_map('trim', $this->headers), TRUE)) . '</pre>';
-          }
-          $verbose .= '<hr />Fields: ' . highlight_string('<?php ' . var_export($post_array, TRUE), TRUE);
-          $verbose .= '<hr />' . $out;
-
-          $this->verbose($verbose);
+          $this->verbose('POST request to: ' . $path .
+                         '<hr />Ending URL: ' . $this->getUrl() .
+                         '<hr />Fields: ' . highlight_string('<?php ' . var_export($post_array, TRUE), TRUE) .
+                         '<hr />' . $out);

@@ -1655,13 +1633,6 @@ protected function drupalHead($path, array $options = array(), array $headers =
-    if ($this->dumpHeaders) {
-      $this->verbose('GET request to: ' . $path .
-                     '<hr />Ending URL: ' . $this->getUrl() .
-                     '<hr />Headers: <pre>' . check_plain(var_export(array_map('trim', $this->headers), TRUE)) . '</pre>');
-    }

These changes in #50 look unintended to me... perhaps we still have some "relics from a poor rebase"

ugh...yes. thank you, sorry. should be taken care of now.

One more time. :-)

Committed 6941c5b and pushed to 8.x. Thanks!

Just moving to task so I don't think there's been a regression.

Going to write API change notification.

Here is the draft of change notice:

As discussed over IRC, we might need to wait for other issues to get in to right change notice:

Drupal 7

function drupal_serve_page_from_cache(stdClass $cache) {
...
...
  // Send the remaining headers.
  foreach ($cache->data['headers'] as $name => $value) {
    drupal_add_http_header($name, $value);
  }

}

Drupal 8

use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpFoundation\Response;

function drupal_serve_page_from_cache(stdClass $cache, Response $response, Request $request) {
...
...
  // Send the remaining headers.
    $response->headers->set($name, $value);
 }

}

Drupal 7

drupal_add_http_header();

Drupal 8

Drupal 7

drupal_send_headers();

Drupal 8

Drupal 7

drupal_page_header();

Drupal 8

Most of #58 isn't really API-facing. The API-facing change here is "if you want to mess with the headers, use kernel.response. Don't use the functions, because they're gone/will be gone."

#2083415: [META] Write up all outstanding change notices before release

Added summary of patches made and results also list of cleanup tasks using issue summary template.

Added change record as per #59

What is the future of drupal_*_header()? They are marked deprecated, but I can't find any issues to remove them.

Header manipulation needs to happen on the Response object directly, in the RESPONSE event. Setting headers arbitrarily is no longer supported.

I have filed #2184907: Remove uses of drupal_add_http_header and related functions to remove these functions.

53: reqresp-1984766-53.patch queued for re-testing.

This doesn't need reroll - it doesn't apply because it's already been committed! Please read the comments beyond a patch before queuing it for retesting or marking the patch as needs work.

This is still open because the change record needs to be completed. The holdup there is that it's not exactly clear what the change record should say. If you were previously calling drupal_add_http_header(), how should you change your code now? There's a relevant discussion on #2184907-10: Remove uses of drupal_add_http_header and related functions onwards. Leaving as needs work, because we still need a change record.

We no longer have those globals.