Problem/Motivation

We're still relying on global headers instead of using the request/response objects in places like page cache handling. This needs to be fixed before release, so here's a start on it.

Working on cleaning up global variable calls such as $_SERVER in code #1998638: [Meta] Refactor raw PHP variables (e.g $_SERVER, $_REQUEST, $_GET, $_POST) with Symfony Request object.

Not the appropriate place to discuss #1855260-39: Make sure page caching works with accept header-based routing.

Proposed resolution

Update drupal_add_http_header() so relying on Symfony to send headers patch: reqrep-1984766-9.patch result: "FAILED: [[SimpleTest]]: [MySQL] 55,672 pass(es), 29 fail(s), and 0 exception(s)." details.

Remaining tasks

  • drupal_set_page_cache() and drupal_serve_page_from_cache() need clean-up.
  • drupal_add_http_header() still being used in install.core.inc and cannot be removed yet.
  • Update or add new patch to use symfony to send headers.

Comments

Status:Needs review» Reviewed & tested by the community

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.

Status:Reviewed & tested by the community» Needs work

The last submitted patch, use_http_objects_for_cache.patch, failed testing.

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?

Status:Needs work» Needs review

use_http_objects_for_cache.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, use_http_objects_for_cache.patch, failed 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.

Status:Needs work» Needs review
StatusFileSize
new12.66 KB
FAILED: [[SimpleTest]]: [MySQL] 55,672 pass(es), 29 fail(s), and 0 exception(s).
[ View ]

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.

Status:Needs review» Needs work
Issue tags:-Needs issue summary update

The last submitted patch, reqrep-1984766-9.patch, failed testing.

The last submitted patch, reqrep-1984766-9.patch, failed testing.

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.

StatusFileSize
new12.53 KB
FAILED: [[SimpleTest]]: [MySQL] 57,314 pass(es), 30 fail(s), and 0 exception(s).
[ View ]

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)

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, reqrep-1984766-13.patch, failed testing.

StatusFileSize
new22.09 KB
FAILED: [[SimpleTest]]: [MySQL] 56,076 pass(es), 5 fail(s), and 0 exception(s).
[ View ]
new15.91 KB

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.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, reqresp-1984766-16.patch, failed testing.

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?

StatusFileSize
new21.81 KB
FAILED: [[SimpleTest]]: [MySQL] 56,002 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
new4.93 KB
new24.8 KB
FAILED: [[SimpleTest]]: [MySQL] 56,041 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

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.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, reqresp-noheaders-1984766-22.patch, failed testing.

StatusFileSize
new3.13 KB
new2.94 KB
FAILED: [[SimpleTest]]: [MySQL] 56,526 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

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

Status:Needs work» Needs review
StatusFileSize
new20.87 KB
PASSED: [[SimpleTest]]: [MySQL] 55,910 pass(es).
[ View ]

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

Status:Needs review» Needs work

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...

StatusFileSize
new8.84 KB
new23.65 KB
PASSED: [[SimpleTest]]: [MySQL] 55,843 pass(es).
[ View ]

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.

Status:Needs work» Needs review

+++ 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.

Isn't the response a reference to the one that gets sent? If the 304 test is passing, I'm assuming that change works.

The rest of this looks like a nice step forward. Would RTBC if it wasn't my original patch.

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.

Status:Needs review» Needs work

this looks good to me.

one thing that needs fixing is returning a cache-control with max-age on a 304 response, so that clients don't keep asking. apart from that, i think this is ready.

Status:Needs work» Needs review
StatusFileSize
new1.68 KB
new23.35 KB
PASSED: [[SimpleTest]]: [MySQL] 55,994 pass(es).
[ View ]

@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.

Status:Needs review» Reviewed & tested by the community

yay, i think this is ready to fly.

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.

Status:Reviewed & tested by the community» Needs work

+++ 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.

StatusFileSize
new945 bytes
new23.46 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/lib/Drupal/system/Tests/Bootstrap/PageCacheTest.php.
[ View ]

@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.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, reqresp-1984766-39.patch, failed testing.

StatusFileSize
new23.36 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/lib/Drupal/system/Tests/Bootstrap/PageCacheTest.php.
[ View ]

Addressed 3 from #38. Good catch-catch.

Status:Needs work» Needs review

oh hai bot.

Status:Needs review» Needs work

The last submitted patch, reqresp-1984766-39.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new611 bytes
new26.21 KB
PASSED: [[SimpleTest]]: [MySQL] 55,831 pass(es).
[ View ]

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.

Status:Needs review» Reviewed & tested by the community

#50 looks good, RTBC time again.

Status:Reviewed & tested by the community» Needs work

+++ 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"

Status:Needs work» Needs review
StatusFileSize
new23.22 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch reqresp-1984766-53.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

Status:Needs review» Reviewed & tested by the community

One more time. :-)

Title:Start relying on Request/Response objects for cache handlingChange notice: Start relying on Request/Response objects for cache handling
Priority:Major» Critical
Status:Reviewed & tested by the community» Active
Issue tags:-Needs issue summary update+Needs change record

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

Category:bug» task

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

Assigned:Unassigned» vijaycs85

Going to write API change notification.

Assigned:vijaycs85» Unassigned

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

<?php
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

<?php
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

<?php
drupal_add_http_header
();
?>

Drupal 8

<?php

?>

Drupal 7

<?php
drupal_send_headers
();
?>

Drupal 8

<?php

?>

Drupal 7

<?php
drupal_page_header
();
?>

Drupal 8

<?php

?>

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."

Issue summary:View changes

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

Issue summary:View changes
Status:Active» Needs review
Issue tags:-Needs change record+LONDON_2014_JANUARY, +SprintWeekend2014

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.

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

Status:Needs review» Needs work

The last submitted patch, 53: reqresp-1984766-53.patch, failed testing.

Issue tags:+Needs reroll

Issue tags:-Needs reroll

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.