Support from Acquia helps fund testing for Drupal Acquia logo

Comments

effulgentsia’s picture

Title: Remove the '_maintenance' request attribute » Remove the '_http_statuscode' request attribute
Component: base system » views.module
dawehner’s picture

You know, I kind of still think that attributes are kind of a good communication layer for basically decoupled code paths
for things which aren't used often. _http_statuscode is just one single small feature in views, so not many people would ever have to deal with it.

dawehner’s picture

Status: Active » Needs review
FileSize
2.25 KB

Trying out some simple idea.

Status: Needs review » Needs work

The last submitted patch, 3: vdc-2239003-3.patch, failed testing.

pwolanin’s picture

How does this differ from _exception_statuscode ?

dawehner’s picture

An alternative way could be to add support for http status codes into #attached

Wim Leers’s picture

#6: But how would that resolve conflicting status codes? Currently, it's "last caller wins" (I think), but in case of bubbling a status code, there's no clear winner?

dawehner’s picture

Well, currently you cannot set it as part of the render array at all, so default is 200, of course. If we add support for it in the render level,
a custom one could overrule the 200.

Wim Leers’s picture

#8: Well, yes, of course, but I mean a situation like this:

parent
  |- A 404
  |- B 500
  |- C 200

Which wins?

dawehner’s picture

From a high level perspective I would argue the highest, at least seen from the first digit. In general I agree that this feature is really specific for viewss, so we can workaround it, as we currently do.

Wim Leers’s picture

I think that the "main content" block should be the one to determine the page's status code. And I think that you're right: highest number wins.

Crell’s picture

We've discussed on and off having HtmlFragment have a sub-interface/class that's specific to the content region, to give it extra special powers. Putting a status code on that (which is the only one that then gets propagated up to the HtmlPage later) makes sense to me.

dawehner’s picture

@crell
The problem is that this doesn't help. Views either returns a HTML/render array (for html pages) or a response object based upon serializer.
This happens in two different codes, obviously.

For both of them, though we want to make the generic functionality inside views to work. Atm we use a subscriber for that and the attributes as temporary storage.

Crell’s picture

It seems like "Views" shouldn't be able to set a response code in the first place. The Page Display Plugin should. That would allow the Page Display Plugin to be responsible for setting a response code on an HtmlMainContentFragment (or whatever). Or am I still completely missing the point?

dawehner’s picture

It seems like "Views" shouldn't be able to set a response code in the first place. The Page Display Plugin should.

Yeah, I like this constructive idea. If you can explain me how the page display plugin would be able to know that a view result is empty, you would be great.

catch’s picture

catch’s picture

Priority: Normal » Critical

Bumping to critical.

#2352155: Remove HtmlFragment/HtmlPage may result in fixing this, but as it is, Views' use of this is incompatible with caching - if the code that sets the attribute on request doesn't run every time, then the status code won't be changed - same as with the static drupal_add_*() methods.

Wim Leers’s picture

Status: Needs work » Needs review
Issue tags: +page rendering
FileSize
3.47 KB

Split off my fix for this from the patch in #2352155: Remove HtmlFragment/HtmlPage.


In all of Drupal core, there's only a single case where the main content (_content) wants to set the HTTP Status header (Status: 200 by default).

In HEAD, Views' \Drupal\views\EventSubscriber\RouteSubscriber has an event listener for KernelEvents::VIEW that runs very late, to modify the HtmlPage object's HTTP status code just before it's converted into a response. This is a global in disguise.
But to determine which status to set, some other Views code sets a request attribute (another global in disguise).

So not only does HEAD actually use two globals, this issue of course aims to remove HtmlPage, so this pattern cannot continue.

Drupal already uses ['#attached']['http_header'], which maps to _drupal_add_http_header(), which is retrieved via drupal_get_http_header() (and yes, this is also a global, but it's not a new one), and \Drupal\Core\EventSubscriber\FinishResponseSubscriber applies those headers to the Symfony Response object. The only tricky bit is that Symfony apparently special-cases the Status header, so a tiny addition was made:

	$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);
  }

Hence now the main content can simply do e.g.:

$build['#attached']['http_header'][] =  ['Status', 404];

to render content, but mark it as a 404 to prevent search engines from indexing the response.

Fabianx’s picture

Looks great to me! And would be RTBC from my POV.

Two questions:

1. If several things set the http_status code via #attached, who will win?
2. Is there test coverage for the case that views sets the correct status code?

e.g. Is it possible to upload a 'fail' patch to show that this has enough test coverage?

Thanks!

catch’s picture

My view on #1 is that we should more or less do max($status_codes) and the worst one wins. That's not perfect but should catch most cases.

catch’s picture

In answer to #6 we've had support for http status codes in #attached since Drupal 7 via #attached['drupal_add_http_header'].

dawehner’s picture

FileSize
4.86 KB
1.39 KB

Sadly the time is a bit limited here at the moment ...
I doubt that this works, let me write a test for it.

Drupal is more than just render arrays and this is the reason why we went with such a functionality in the first place.
On the other hand I doubt that this is a critical issue, it is a really limited feature, just used internally by views.

Status: Needs review » Needs work

The last submitted patch, 22: 2239003-22.patch, failed testing.

dawehner’s picture

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
4.32 KB
2.8 KB

So the test case added in #22 shows that my patch doesn't work for non-HTML responses. So I investigated, trying to find a way to make it work after all. To my surprise, FinishResponseSubscriber::onRespond() was also called, which then should set the headers all the same. So, what's going on here?
Turns out Views doesn't execute area handlers for Feed displays. Hence the status code-setting code is never called, hence the test cannot ever pass, also not in HEAD.

Discussed with dawehner in IRC. He's fine with #18 as-is for now, but is not happy with the fact that we're going back to another global, one we've been working to get rid of. To make sure we still move in the right direction, we decided that we want a @todo in FinishResponseSubscriber, where it calls drupal_get_http_header(), to make sure we update to the right pattern.

dawehner’s picture

Opened a follow up for that problem itself ...

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

It's kinda sad to know that other stuff is broken ...

alexpott’s picture

+++ b/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php
@@ -98,10 +98,18 @@ public function onRespond(FilterResponseEvent $event) {
+    // @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.

Is there an issue for this @todo?

Wim Leers’s picture

#28: No, just like there wasn't for the pre-existing @todo, which I've merely clarified.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Removing the _drupal_add_*() functions is enabled once all of #2352155: Remove HtmlFragment/HtmlPage is complete, but I don't think we have a specific plan for what the replacements look like yet (possibly tagged services, possibly something else).

Since it's just expanding the existing @todo I'm going to go ahead and commit this to keep the parent issue moving.

Committed/pushed to 8.0.x, thanks!

Fabianx’s picture

Do we need a followup to use the max() of status codes or is it too much of an edge case?

  • alexpott committed 61f3fce on 8.0.x
    Issue #2239003 by Wim Leers, dawehner | effulgentsia: Remove the '...
catch’s picture

I think that's worth a follow-up.

Status: Fixed » Closed (fixed)

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