Problem/Motivation

drupal_add_http_header() is marked as @deprecated for removal before 8.0.0 release.

drupal_process_attached() uses drupal_add_http_header() in order to process attached headers.

The only use of drupal_process_attached() is by the views module, specifically to change the status code of the HTTP response.

'Attached' status codes need to have the following properties, for both anonymous and logged-in users:

  • Views needs to be able to cache both the query and the content, meaning the response code and whatever headers accompany it.
  • Drupal needs to be able to cache headers and response codes.

Comment #32 shows us a test for the Views needs.

Proposed resolution

The render system should store #attach html_header headers and response codes in an event subscriber, which would then modify the output Response object accordingly.

Alternately, FAPI should be changed to disallow #attached for html_header, and all code which wants to change headers or response code should create their own event subscriber.

Remaining tasks

Decide what to do.

User interface changes

API changes

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because the code works as-is, just isn't properly encapsulated.
Issue priority Normal because nothing is broken.
Prioritized changes Prioritized because this issue will unblock the removal of @deprecated code.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Mile23’s picture

Berdir’s picture

It's not going to be very pretty, but it shouldn't be too hard.

1. The headers are set in FinishResponseSubscriber (finish_response_subscriber). Add a addHeader() method on that, call it from here. Then use that information in there instead of calling drupal_get_http_header().
2. Remove drupal_get_http_header() and _drupal_add_http_header().
3. Party.

Berdir’s picture

Issue tags: +WSCCI, +Novice

Should be doable for a novice if you know about about services. (hint \Drupal::service('finish_response_subscriber')).

Mile23’s picture

Assigned: Unassigned » Mile23
Mile23’s picture

Assigned: Mile23 » Unassigned
Status: Active » Needs review
Issue tags: -Novice
FileSize
8.23 KB

It's only ugly because the names are wrong. There should be an 'attached_headers' service that's a ResponseHeaderBag.

But here we add a ResponseHeaderBag property to the finish_response_subscriber service and then use that to attach headers.

Includes tests. onRespond() has a gnarly set of dependencies.

Also, only a cruel, cruel person would say this is a novice issue. :-)

Status: Needs review » Needs work

The last submitted patch, 5: 2467759_5.patch, failed testing.

Crell’s picture

Um, wow. FinishResponseSubscriber is something I expected to get killed eventually, not turned into a stealth global AND event listener. I don't like this approach even a little bit.

Mile23’s picture

@crell: Have an alternate plan? Everyone seems to agree it's a bit stinky.

Berdir’s picture

Yeah, thanks for the helpful suggestion there, that sounds great, let's just implement that instead...

We need #attached to be able to add header tags. Full stop. Which means it's either in the existing finish subscriber or we add a separate one that does just that.

I've never heard of any plans to remove the finish subscriber and if we do, we can always move this somewhere else, I don't see why it's so bad for now.

@Mile23: On novice, well, I didn't expect to use fancy things like header bags (which does make sense) or writing complex unit tests for it to be fair :p

* You're calling it wrong, you need to use call_user_func_array() for it as well, because you get an array of function arguments.
* Instead of just adding it, let's replace the existing call to drupal_get_http_headers() in there. This is the only call to it anyway in core, and if we want to remove this, we need to remove the counter-part and helper functions as well.

Mile23’s picture

Status: Needs work » Needs review
FileSize
8.32 KB

Let's see what the testbot thinks of this.

I added a service/event subscriber called attached_headers_subscriber. drupal_process_attached() now uses it to attach headers, thus making this patch in scope. :-)

Includes a test, but more will be needed eventually.

Barking up the right tree here?

Status: Needs review » Needs work

The last submitted patch, 10: 2467759_10.patch, failed testing.

Berdir’s picture

+++ b/core/lib/Drupal/Core/Render/Element/Table.php
--- /dev/null
+++ b/core/tests/Drupal/Tests/Core/EventSubscriber/AttachedHeadersSubscriberTest.php

+++ b/core/tests/Drupal/Tests/Core/EventSubscriber/AttachedHeadersSubscriberTest.php
@@ -0,0 +1,66 @@
+class AttachedHeaderSubscriberTest extends UnitTestCase {

Header vs Headers, that's why.

Mile23’s picture

Assigned: Unassigned » Mile23
Mile23’s picture

Status: Needs work » Needs review
FileSize
8.85 KB
2.85 KB

Moving forward...

Noting a few things:

Anyone can now do this: \Drupal::services('attached_headers_subscriber')->add('any-header', 'any value'); without specifying it in the render array. This includes caching. Should we just have a general 'this is where you put extra headers' service or enforce attachments in some way?

__drupal_add_http_header() will unset a stored header if the value is FALSE. This isn't implemented here. Does it matter?

Berdir’s picture

Assigned: Mile23 » dawehner

Separate service is fine with me as well.

Anyone could have called the current function as well. I think the only way to get rid of that would be to refactor the #attached processing so that it is event based and extendable (which would be pretty cool but way off-topic for this issue), then the subscriber could just listen for that even and pick the headers out, and we wouldn't have a public API to add them.

Maybe @dawehner has some thoughts on this, but this looks acceptable to me to get rid of those deprecated function calls. We need to make small steps or it won't happen.

Crell’s picture

Miles23, Berdir: The root problem here is that Render API still relies on globals for storing its non-string data to pass forward to where it gets added to the HTML output or response. That's what needs fixing. Moving the global around doesn't really improve anything other than superficially. The design flaw is still there, just with more indirection. Maybe that indirection is an improvement, but it's still papering over a global design flaw in Render API.

If it's too late to fix that design flaw, well, so be it but it makes me very sad. The latest patch is at least an improvement from an SRP perspective. Although my next question is if it should also be the collector for the other attached data? Or do we need parallel services for that?

Miles23: Anyone other than Render API should be setting headers in their own Response events, not through this service. I wouldn't put any more work into its API than is necessary for that. In fact I'd go as far as saying that we should add documentation to the class saying "if you're not Render API, do not use this class, write your own response event".

+++ b/core/core.services.yml
@@ -962,6 +962,10 @@ services:
+  attached_headers_subscriber:
+    class: Drupal\Core\EventSubscriber\AttachedHeadersSubscriber
+    tags:
+      - { name: event_subscriber }

This shouldn't be named "subscriber". Devs should never have reason to access a _subscriber service directly; we'd make them private if we could. (EventDispatcher's setup doesn't support that unfortunately.)

And therefore the class should not be in the EventSubscriber namespace. Probably it should go in whatever namespace Render API is in?

Mile23’s picture

FileSize
6.05 KB

Something like this?

The only place in core we're using ['#attached']['http_header'] is views' HTTPStatusCode, and it's only using it to change the response code, which I guess counts as a header. Maybe.

This hack-around has HTTPStatusCode adding the subscriber during the render phase of a form. There is probably a better place to have this happen, but I don't know where it is, because my views-fu... is no good! But it does allow us to put off deciding whether we need it until the very last minute; we should probably have an event for that.

The upshot of this strategy is that handling of headers and response codes and such becomes a race condition, assuming anyone does it. There should probably also be some guidelines about setting priorities for these things.

The attached patch passes AreaHTTPStatusCodeTest. Let's see what it breaks.

Mile23’s picture

FileSize
6.55 KB
2.75 KB

Coding standards update.

Fabianx’s picture

#16:

The problem is not Render API, every Controller that returns a render array is fine as HtmlRenderer could easily take the data out of #attached and put it on the Response.

The problem is that we allow arbitrary responses, but have not defined what you need to do to get that data from the Renderer in my opinion.

e.g. a Controller doing:

$markup = $renderer->renderRoot($foo);

return new JsonResponse(['content' => $markup], 200);

--

That is where the GAP is, how to get the data from the renderer to the response and how to "encourage" people to don't forget it.

And how to store data on the response too, see also #2463009: Introduce CacheableResponseInterface: consolidate ways of setting X-Drupal-Cache-Tags/Contexts headers.

At the moment we already have special code for ajax responses to add the attached script assets, so maybe a generic:

$markup = $renderer->renderRoot($elements);

$response = new Response($markup, 200);
$response->addRenderData($elements);

could work?

I am not sure. I just know that responses not having a way to store cacheable data and attached data is a problem for Drupal.
( and a missing ParameterBag for arbitrary data makes the problem more problematic, too).

Mile23’s picture

Status: Needs review » Needs work

So restating the problem:

HTTP headers and response codes need to be cacheable, and the cache system does this through the render API.

This means that whatever system replaces drupal_add_http_header() has to honor the render array, and not give an API to bypass it.

The issue of whether someone just bypasses the whole thing and does return new Response(); is a mistake which no core dev should make (and no review should allow), and so therefore the problem is contrib.

An API exists for changing headers and response codes, and that is #attached. That's what we want contrib to do, for the caching reasons mentioned above.

About right?

Fabianx’s picture

I am tempted to postpone this for a moment on #2463009-12: Introduce CacheableResponseInterface: consolidate ways of setting X-Drupal-Cache-Tags/Contexts headers as that issue will make this issue easier, but not yet doing so, leaving that decision to Wim.

Mile23’s picture

Mile23’s picture

Status: Needs work » Needs review
Issue tags: +Needs change record
FileSize
6.53 KB

Re-roll.

Again: This only changes the http_status_code view because it's the only code in core that's using drupal_add_http_header()/drupal_process_attached().

It basically removes the capacity to add headers using ['#attached']['http_header'] in the render system, meaning that it might be an API change for contrib. The way for contrib to handle changing headers is the solution proposed earlier here: Create a listener and have it add the headers you want during onResponse().

If this API change is too much to bear in beta times, we can add a follow-up to re-add the #attached functionality and test for it. For now we want to work on @deprecate-ing drupal_add_http_header().

#2463009: Introduce CacheableResponseInterface: consolidate ways of setting X-Drupal-Cache-Tags/Contexts headers in fact uses the listener model to solve some important caching and header problems, so it should be a guide for contrib.

Berdir’s picture

Status: Needs review » Needs work
+++ b/core/modules/views/src/Plugin/views/area/HTTPStatusCode.php
@@ -62,10 +104,11 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
    */
-  function render($empty = FALSE) {
+  public function render($empty = FALSE) {
     if (!$empty || !empty($this->options['empty'])) {
-      $build['#attached']['http_header'][] = ['Status', $this->options['status_code']];
-      return $build;
+      // Add an event subscriber so it will change the status code when the
+      // response object is ready.
+      $this->eventDispatcher->addSubscriber(new HTTPStatusCodeListener($this->options['status_code']));
     }

This was using #attached for a reason, you can't just replace it like this.

If the view is using render/output caching, then the status code needs to be set again every time the view is displayed.

Mile23’s picture

Status: Needs work » Needs review
FileSize
14.4 KB
7.87 KB

This test says otherwise. :-)

This patch expands AreaHTTPStatusCodeTest to include testing the cache status of cached views.

(The test_only patch is the interdiff.)

Wim Leers’s picture

Status: Needs review » Needs work
  1. +++ b/core/includes/common.inc
    @@ -723,9 +723,6 @@ function drupal_process_attached(array $elements) {
    -        case 'http_header':
    -          call_user_func_array('_drupal_add_http_header', $args);
    -          break;
    
    +++ b/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php
    @@ -114,23 +114,6 @@ public function onRespond(FilterResponseEvent $event) {
    -    // Attach globally-declared headers to the response object so that Symfony
    -    // can send them for us correctly.
    -    // @todo Remove this once drupal_process_attached() no longer calls
    -    //    _drupal_add_http_header(), which has its own static. Instead,
    -    //    _drupal_process_attached() should use
    -    //    \Symfony\Component\HttpFoundation\Response->headers->set(), which is
    -    //    already documented on the (deprecated) _drupal_process_attached() to
    -    //    become the final, intended mechanism.
    -    $headers = drupal_get_http_header();
    -    foreach ($headers as $name => $value) {
    -      // Symfony special-cases the 'Status' header.
    -      if ($name === 'status') {
    -        $response->setStatusCode($value);
    -      }
    -      $response->headers->set($name, $value, FALSE);
    -    }
    

    No question that this was a bad implementation. But it was completely from the D7 days — it just never got fixed/improved by WSCCI. That's why we're only fixing it now.

    Note that there is no actual problem with ['#attached']['http_header']; the actual problem is that instead of relying the bubbling and thus not relying on global state, we choose to call a procedural function that *does* store it in global state.

    If we'd have something like CacheableResponse(Interface|Trait), but for #attached, then we could let FinishResponeSubscriber do something very similar to this, which it already does:

        // Expose the cache contexts and cache tags associated with this page in a
        // X-Drupal-Cache-Contexts and X-Drupal-Cache-Tags header respectively.
        if ($response instanceof CacheableResponseInterface) {
          $response_cacheability = $response->getCacheableMetadata();
          $response->headers->set('X-Drupal-Cache-Tags', implode(' ', $response_cacheability->getCacheTags()));
          $response->headers->set('X-Drupal-Cache-Contexts', implode(' ', $this->cacheContextsManager->optimizeTokens($response_cacheability->getCacheContexts())));
        }
    
  2. +++ b/core/modules/views/src/EventSubscriber/HTTPStatusCodeListener.php
    @@ -0,0 +1,56 @@
    + * Contains \Drupal\views\Plugin\views\area\HTTPStatusCode.
    ...
    +class HTTPStatusCodeListener implements EventSubscriberInterface {
    

    Mismatch + "listener" instead of "subscriber".

  3. +++ b/core/modules/views/src/EventSubscriber/HTTPStatusCodeListener.php
    @@ -0,0 +1,56 @@
    + * Used by the 'http_status_code' view.
    

    s/view/Views area handler/

  4. +++ b/core/modules/views/src/Plugin/views/area/HTTPStatusCode.php
    @@ -20,6 +23,45 @@
    +   * @param ContainerAwareEventDispatcher $event_disaptcher
    

    Needs FQCN.

  5. +++ b/core/modules/views/src/Tests/Handler/AreaHTTPStatusCodeTest.php
    @@ -47,6 +51,46 @@ public function testHTTPStatusCodeHandler() {
    +    // Test that the response is not from the cache.
    +    $this->drupalGet('test-http-status-code-cache-time');
    +    $this->assertResponse(418);
    +    $this->assertEqual('MISS', $this->drupalGetHeader('x-drupal-cache'));
    +
    +    // Make the same request so it should come from the cache.
    +    $this->drupalGet('test-http-status-code-cache-time');
    +    $this->assertResponse(418);
    +    $this->assertEqual('HIT', $this->drupalGetHeader('x-drupal-cache'));
    

    The reason these tests pass, is that they're relying on the internal page cache (for anonymous users). For an authenticated user (or if the internal page cache were disabled), they wouldn't work.

    In other words: a View whose output cache is being used, should also have the http_status_code Views area handler work as expected. But it won't.

Fabianx’s picture

Yes, I think a ResponseAttachmentsInterface would be nice, especially as AJAX already has setAttachments.

And because placeholders will soon live in #attached as well, it would all make nice sense.

Train of thought

To go all out, we could even just put a placeholder-string for JS and CSS in html.html.twig and just replace them in FinishResponseSubscriber ...

Then attachments could be changed even later ...

Wim Leers’s picture

Yes, I think a ResponseAttachmentsInterface would be nice, especially as AJAX already has setAttachments.

Exactly, see \Drupal\Core\Ajax\AjaxResponse::setAttachments(). Related: #2368797: Optimize ajaxPageState to keep Drupal 8 sites fast on high-latency networks, prevent CSS/JS aggregation from taking down sites and use HTTP GET for AJAX requests and #2407201: Consider introducing AjaxResponse::addAttachments().

Mile23’s picture

Thanks for the review.

The reason these tests pass, is that they're relying on the internal page cache (for anonymous users). For an authenticated user (or if the internal page cache were disabled), they wouldn't work.

In other words: a View whose output cache is being used, should also have the http_status_code Views area handler work as expected. But it won't.

Any suggestions on a way forward?

Fabianx’s picture

Yes, we need to either set attachments on responses using interfaces or creating a response generator that can be used to create a response from a render array.

Mile23’s picture

Status: Needs work » Needs review
FileSize
8.68 KB

So here's another test along the same lines, against HEAD.

This is the same test, except we call it twice, once with an anonymous user and once with a logged-in user.

It will fail, because the logged-in user has a different behavior.

I'm not up on the Views internal stuff to know whether that's expected or not, but from what you're saying it seems like it should be the same either way.

So should it?

Status: Needs review » Needs work

The last submitted patch, 32: 2467759_32.patch, failed testing.

Mile23’s picture

Yes, I think a ResponseAttachmentsInterface would be nice, especially as AJAX already has setAttachments.

Disagree. Attachments are just html_head, a special case of HTML not present in all responses. (Also: AJAX. How 20th century. :-))

But that's out of scope here.

For a logged-in user, what is the expected behavior of cached views responses with altered response codes?

Mile23’s picture

Component: render system » views.module
Mile23’s picture

Issue summary: View changes
Wim Leers’s picture

Attachments are just html_head, a special case of HTML not present in all responses.

They're present in all of the Response formats that render arrays can be rendered into: HTML, dialog, modal, AJAX. So I'm not sure what you mean?

Mile23’s picture

So when all these new response interfaces are done, how will you cache them according to the scope of this issue? :-)

'Attached' status codes need to have the following properties, for both anonymous and logged-in users:

  • Views needs to be able to cache both the query and the content, meaning the response code and whatever headers accompany it.
  • Drupal needs to be able to cache headers and response codes.
Mile23’s picture

So where are we on this?

Fabianx’s picture

#39: Ready to be worked on.

All crap is now isolated to one Response subscriber :).

Mile23’s picture

Status: Needs work » Needs review
FileSize
13.66 KB

Here's just a plain ol' reroll.

Mile23’s picture

All crap is now isolated to one Response subscriber :).

Right, but WHO CACHES WHAT PART? :-)

The problem here: We're altering the response code, which must be cached, at any of the following levels: Controller, views, rendering, kernel.

Status: Needs review » Needs work

The last submitted patch, 41: 2467759_40.patch, failed testing.

Fabianx’s picture

Old patches won't help.

You can leave all the render array things in.

The main thing is now In HtmlResponseSubscriber, there you can refactor.

Mile23’s picture

Priority: Normal » Major
Status: Needs work » Postponed
Related issues: +#2529500: Views HTTP Status Area caching has inconsistent behavior for anonymous vs. logged-in users

Old patches won't help.

You can see from #41 that Views' behavior is still inconsistent, as it was in #32, despite the objections to earlier solutions in this issue.

Let's figure out the desired caching behavior before proceeding here: #2529500: Views HTTP Status Area caching has inconsistent behavior for anonymous vs. logged-in users

Mile23’s picture

Status: Postponed » Needs review
FileSize
8.34 KB

OK. Here's a somewhat work-in-progress I want to run against the testbot and also reviewers. :-)

This patch gives AttachmentsTrait the ability to parse out headers and status code from the render array.

Then it's a matter of having the response listener do this, and add the result to the response.

Given this, we can, in fact, refactor drupal_process_attached() so it doesn't use drupal_add_http_header(). In fact, this strategy allows us to bypass drupal_process_attached() and the others. But not in this patch, or this issue.

Will fill in more docs and so forth if the patch is met with happiness.

Status: Needs review » Needs work

The last submitted patch, 46: 2467759_46.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
5.19 KB

Suppose easy way to do that

Status: Needs review » Needs work

The last submitted patch, 48: 2467759-drupal_get_http_header-46.patch, failed testing.

Mile23’s picture

Status: Needs work » Needs review
FileSize
5.23 KB
706 bytes

Much better. Here's one straggler.

Mile23’s picture

Mile23’s picture

Issue tags: -Needs change record
dawehner’s picture

Assigned: dawehner » Unassigned

Nice, this looks really great!

andypost’s picture

Component: views.module » render system
Assigned: Unassigned » Wim Leers

Would be great to get sign off from Wim on the change

andypost’s picture

Wim Leers’s picture

Assigned: Wim Leers » Unassigned
Status: Needs review » Needs work

This looks great! :)

Just one small problem:

+++ b/core/lib/Drupal/Core/Render/HtmlResponseAttachmentsProcessor.php
@@ -205,13 +205,17 @@ protected function renderPlaceholders(HtmlResponse $response, array $placeholder
+      $append = !empty($values[2]);
...
+      $response->headers->set($name, $value, $append);

So the original parameter was called $append. But HeaderBag::set()'s 3rd parameter is called $replace. So this should be renamed to $replace.

andypost’s picture

Status: Needs work » Needs review
FileSize
913 bytes
5.21 KB

Yep, makes sense

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
Mile23’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
7.79 KB
2.58 KB

Added a test.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Oh wow, I'd have sworn we had a test for this already, but apparently we did not. Many thanks, @Mile23!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/lib/Drupal/Core/Render/HtmlResponseAttachmentsProcessor.php
@@ -205,13 +205,17 @@ protected function renderPlaceholders(HtmlResponse $response, array $placeholder
+      $replace = !empty($values[2]);
...
+      $response->headers->set($name, $value, $replace);

Do we have test coverage of $replace being TRUE? Should it ever be TRUE?

Wim Leers’s picture

Issue tags: +Novice, +php-novice

Let's add a quick test for that, right here:

+++ b/core/modules/system/tests/modules/httpkernel_test/src/Controller/TestController.php
@@ -21,4 +21,17 @@ public function get() {
+    $render['#attached']['http_header'][] = ['X-Test-Teapot', 'Teapot Mode Active'];
+    $render['#attached']['http_header'][] = ['Status', "418 I'm a teapot."];

Add the same header twice, with different values, ensure that the last value wins, because $replace === TRUE.

Mile23’s picture

Status: Needs work » Needs review
FileSize
8.36 KB
1.79 KB

Behold, we have added such a test.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Behold, we have added such a test.

:D

Thank, you good Sir!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Thanks for the additional test. Committed 3416124 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

  • alexpott committed 3416124 on 8.0.x
    Issue #2467759 by Mile23, andypost, Wim Leers, Berdir, Fabianx, Crell:...
alexpott’s picture

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.