Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Child of #2124749: [meta] Stop using $request->attributes->get(MAGIC_KEY) as a public API. Any ideas what to replace it with?
Comment | File | Size | Author |
---|---|---|---|
#25 | attached_status_header-2239003-25.patch | 4.32 KB | Wim Leers |
Comments
Comment #1
effulgentsia CreditAttribution: effulgentsia commentedComment #2
dawehnerYou 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.
Comment #3
dawehnerTrying out some simple idea.
Comment #5
pwolanin CreditAttribution: pwolanin commentedHow does this differ from _exception_statuscode ?
Comment #6
dawehnerAn alternative way could be to add support for http status codes into #attached
Comment #7
Wim Leers#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?
Comment #8
dawehnerWell, 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.
Comment #9
Wim Leers#8: Well, yes, of course, but I mean a situation like this:
Which wins?
Comment #10
dawehnerFrom 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.
Comment #11
Wim LeersI 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.
Comment #12
Crell CreditAttribution: Crell commentedWe'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.
Comment #13
dawehner@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.
Comment #14
Crell CreditAttribution: Crell commentedIt 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?
Comment #15
dawehnerYeah, 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.
Comment #16
catchComment #17
catchBumping 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.
Comment #18
Wim LeersSplit 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 HTTPStatus
header (Status: 200
by default).In HEAD, Views'
\Drupal\views\EventSubscriber\RouteSubscriber
has an event listener forKernelEvents::VIEW
that runs very late, to modify theHtmlPage
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 viadrupal_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 SymfonyResponse
object. The only tricky bit is that Symfony apparently special-cases theStatus
header, so a tiny addition was made:Hence now the main content can simply do e.g.:
to render content, but mark it as a 404 to prevent search engines from indexing the response.
Comment #19
Fabianx CreditAttribution: Fabianx commentedLooks 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!
Comment #20
catchMy 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.
Comment #21
catchIn answer to #6 we've had support for http status codes in #attached since Drupal 7 via
#attached['drupal_add_http_header']
.Comment #22
dawehnerSadly 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.
Comment #24
dawehnerComment #25
Wim LeersSo 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 forFeed
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
inFinishResponseSubscriber
, where it callsdrupal_get_http_header()
, to make sure we update to the right pattern.Comment #26
dawehnerOpened a follow up for that problem itself ...
Comment #27
dawehnerIt's kinda sad to know that other stuff is broken ...
Comment #28
alexpottIs there an issue for this @todo?
Comment #29
Wim Leers#28: No, just like there wasn't for the pre-existing @todo, which I've merely clarified.
Comment #30
catchRemoving 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!
Comment #31
Fabianx CreditAttribution: Fabianx commentedDo we need a followup to use the max() of status codes or is it too much of an edge case?
Comment #33
catchI think that's worth a follow-up.