Remove uses of drupal_add_http_header and related functions

i see no patch

Some work with an @FIXME

I like the patch so far :)

Regarding [#attached']['drupal_add_http_header']: do we even need to support that? It's used extremely rarely and it complicates the design of #attached (that ability means it's not solely declarative, but could execute any callback). I'd support removing that ability altogether and just have headers be set by the corresponding controllers.

(The most common uses seem drupal_add_feed() and drupal_add_html_head_link(), both of which seem to be prime candidates to move into the controllers anyway, to have all related logic in a single place, with headers being set explicitly by the responsible controller.)

+++ b/core/authorize.php
@@ -164,5 +165,7 @@ function authorize_access_allowed() {
+  $response->setContent(drupal_render($maintenance_page));

Are you sure we want to do this, and not use Drupal's HtmlPageController and all that?
(IDK, just asking/pointing out.)

"Move into controllers" solves nothing, since the Controllers run before we have a response object. (99% of controllers don't need to and shouldn't have to create their own response object, even though they can.)

This is a problem that we need a coherent strategy for. There's multiple kinds of out-of-band data that we need to handle, and need to be added in different places. HTTP headers, HTML headers, and assets are the main ones. We've talked a lot about assets, but less about the others.

If we want controllers to be able to add HTML headers and HTTP headers, then we need to give them an API to do so. Even if that API is declarative render arrays, they need something.

If we don't, then we MUST ensure that there's enough context that a module can write a view listener (for HTML headers) or response listener (for HTTP headers) and apply the logic it needs. Not doing one or both of those is not an option.

Related: #2218117: Bring back metatag support for the HtmlPage object

#12: Thanks for chiming in! Obviously you're in a better position to provide guidance on this. Could you provide a rough yet more concrete example of how you think this should work? Preferably with pseudocode?

Well, in fairness I will lead off by saying that not everyone agrees with me about how it should work. :-) That said...

My intent in the HtmlPage issue (although the code has drifted somewhat from there), to which the issue linked in #12 is a follow-up, was that above the controller/block level we don't have render arrays, just domain objects. HtmlFragment, HtmlPage, etc. Those contain a body (string or, I suppose, internal render array but only internal) and sideband data relevant to their domain object. See the patch in the linked issue for the kind of metadata I was thinking of. The view layer via view listeners is responsible for "upgrading" one domain object to another: HtmlFragment->HtmlPage->Response. That allows a controller to return an object at any level and have it upgraded the rest of the way, as well as allows modules to intercept that upgrading process anywhere along the way.

Below the Controller/Block level, we still have render arrays because, well, I don't have the energy to OOPify render API and/or develop something better in this cycle. However, those render arrays are, in a sense, simply a more degenerate form of the same domain objects listed above. They would still need to be able to carry the same information in degenerate form. So, eg, just as we have #attached to carry JS/CSS sideband data we would need an #html_header to carry HTML head data (perhaps using the HeadElement objects defined in the linked issue) and, possibly, #http_header data, although the latter is worrisome to me.

The challenge is that sometimes you want to add out-of-band data on a per-route basis (in a controller/block), and sometimes you want to do so globally (in a view or response listener), and sometimes you want to do so over a large scope (eg, all node html pages or all nodes of a certain type). For global operation, that's what HtmlFragment/HtmlPage are for. For per-route operation, we need to expand render API to transmit that data. For large scope operation... I don't know. We don't have sufficient metadata in the right place at present to do things like "add all links defined on this entity to the entity's page response", since we have no good way to say what the "primary" entity of a given page is, or if there is one. (That's the only reason nodes even have their own view controller right now rather than just using the entity-based automated view handlers.)

I was going to try and alter drupal_render() to return an HtmlFragment, but ran into challenges with caching. Discussion with Mark and Sam showed that the vision for HtmlFragment was not completely shared, and I didn't have a solution yet for how to make that all work and they wanted to go a different direction. We've not gotten back to that discussion as they seemed to indicate they felt it was settled, but I did not. I also have been way too busy to dig back into experimental code on this point. :-)

As relates to this issue specifically, I believe the solution involves:

1) Adding appropriate support to render API to allow render arrays to define HTTP headers.
2) Converting what code we can to proper response listeners.
3) Ensuring that code that uses the first option still works, probably at this point using the same techniques that we're using for internalizing _drupal_add_css() et al until/unless render API gets fully revamped.

@crell
I totally agree but we don't have a normal page on all the instances in the patch. This patch is covering: toolbar json response, authorize.php, update.php and the installer. At least the three other instances needs totally rewrite anyway, so we can't block the changes here just because we want to rewrite these 3 instances.

I am just rerolling the patch to keep the discussion going. Most of the conflicts were from #2218039: Render the maintenance/install page like any other HTML page.

I should add that in #16, I have used the return value from _toolbar_initialize_page_cache in ToolbarController::subtreesJsonp. Strangely, I couldn't find this in the previous patch but I don't see what is the point in returning a JsonResponse from _toolbar_initialize_page_cache unless we're using it somewhere. The only place that function was used was in ToolbarController::subtreesJsonp.

If there is a single place we can also set the header in this singe place.
Are you okay with adapting it?

Patch no longer applied. Created re-roll.

Uploaded wrong patch. This is the right one.

+++ b/core/includes/install.core.inc
@@ -919,7 +919,16 @@ function install_display_output($output, $install_state) {
+    'Expires' => 'Sun, 19 Nov 1978 05:00:00 GMT',

Is this random? Why not 1.1.1970?

These changes seem fine. My IDE tells me the only other call to drupal_add_http_header() is in system_test.module, which means it should be easy enough to remove as well. There's also a @see in common.inc we can remove. Let's go ahead and do those, then remove drupal_add_http_header() itself.

hass: That's always been Drupal's expiration time for "in the past". It's a specific value with importance to Drupal history. :-)

Aha - Dries birthday. No idea why we need this. It's new and it's not documented why.

Made changes as suggested by creel in #24. Patch attached.

hass: It's not new. That's been our expires time for at least a decade. It's the most reliable way to finger-print a Drupal site. :-) It's an easter egg.

That is a bad idea. We typically try to obfruscate software versions and software types to the public. Otherwise you are easier attackable and detectable from outside for trojan horses. I was not aware of this yet. Very bad.

i think the hunk in install.core.inc is not needed..but let see if this gets to green first

ah..i see. #20 missed the drupal_page_header() removal. should be go to go now

• There is a potential code path (no output) which results in sending nothing, not even an empty respond. Let's ensure to send at the end of the process.

+++ b/core/includes/install.core.inc
@@ -922,7 +920,16 @@ function install_display_output($output, $install_state) {
+  $response = new Response();
+  $default_headers = array(
+    'Expires' => 'Sun, 19 Nov 1978 05:00:00 GMT',
+    'Last-Modified' => gmdate(DATE_RFC1123, REQUEST_TIME),
+    'Cache-Control' => 'no-cache, must-revalidate, post-check=0, pre-check=0',
+    'ETag' => '"' . REQUEST_TIME . '"',
+  );
+  $response->headers->add($default_headers);
+  $response->setContent(DefaultHtmlPageRenderer::renderPage($output, $output['#title'], 'install', $regions));
+  $response->send();

Sure this is some progress, but there really should be just one place where we send things out, at least for now this is okay.

Can we remove drupal_add_http_header() while we're at it?

Hmm. I was working on this issue but in the function drupal_add_html_head_link the drupal_add_http_header is still called :

$element['#attached']['drupal_add_http_header'][] = array('Link',  $href . drupal_http_header_attributes($attributes), TRUE);

I'm not sure how to remove it here.

Yeah drupal_add_http_header can be later dropped.

The installer code is a bit sad, but so are lots of things in the installer. Removing the direct usages here is worth the duplication there.

This doesn't have any real conflicts with #2350877: Deprecate/rename drupal_add_feed(), drupal_add_html_head(), drupal_add_html_head_link(), drupal_add_http_header(), and allow to be set declaratively in #attached and that's not quite ready yet, so committed/pushed to 8.x, thanks!

related: #2549041: Remove deprecated _drupal_set_preferred_header_name()

It took me a while to figure it out, just in case anybody needs to set the http status response code for example in template_preprocess_node() or in template_preprocess_page() as it was possible with drupal_add_http_header(), it could be done there like:

$variables['#attached']['http_header'] = array(
  array('status', 401),
);