HtmlResponseSubscriber does not call HtmlResponseAttachmentsProcessor on subrequests

Reproduced the failure. And it was a quite tricky one to debug.

Turns out this actually uncovers a bug in HEAD. 4xx pages are rendered using subrequests. This is once again related to #2230121: Remove exit() from FormBuilder and indicates how #2206909: Regression: Form submit redirects from 403/404 pages are no longer possible didn't actually fix everything: it fixed the ?destination= case, but not the "redirect caused by a form" case. Which is what we're seeing here.

In HEAD, the 4xx page's placeholders are rendered in the master request. When placeholders are replaced, this triggers \Drupal\Core\Form\FormSubmitter::redirectForm() which does return new RedirectResponse($url, Response::HTTP_SEE_OTHER);. This goes up through the layers of building a form until it gets to throw new EnforcedResponseException($response);. This is then the response that is returned.

However, with the patch, the 4xx page's placeholders are already rendered in the sub request. When placeholders are replaced, this triggers the same process as above. But then after the sub request's response is returned, \Drupal\Core\EventSubscriber\DefaultExceptionHtmlSubscriber::makeSubrequest() indiscriminately sets the 4xx status code. This makes sense for a 200 response. But not for a redirect.

Schematically:

HEAD

1. makeSubrequest() gets back a 200 response
2. it overrides the status code to 404
3. it then invokes the RESPONSE event
4. which causes the placeholders to be replaced, and eventually results in a 303
5. a 303 is returned, and SimpleTest follows the redirect

HEAD

1. makeSubrequest() gets back a 303 response, because placeholders are already replaced
2. it overrides the status code to 404
3. it then invokes the RESPONSE event
4. placeholders are not replaced again of course
5. a 404 is returned, and SimpleTest does NOT follow the redirect

AFAICT the failed test here is exactly the test coverage that we wanted.

Discussed with @dawehner in IRC:

13:13:43 <dawehner> WimLeers: do you mind adding a comment in DefaultExceptionHtmlSubscriber, maybe just link to your comment
13:14:01 <WimLeers> dawehner: Sure, happy to do so
13:14:05 <WimLeers> dawehner: did that make sense?
13:27:54 <dawehner> WimLeers: if (!$response->isRedirection()) { ... yeah I'm wondering whether some other check is the right thing to do
13:28:11 <WimLeers> dawehner: I was wondering the same
13:28:19 <WimLeers> dawehner: feels like that still doesn't cover everything
13:28:25 <WimLeers> dawehner: e.g. a 500 should also not be overridden
13:28:40 <WimLeers> dawehner: so perhaps only in case of 2xx statuses we should override it to 404?
13:28:43 <dawehner> WimLeers: right, so just 200 should be?
13:33:53 <dawehner> WimLeers: that sounds sensible for me

Done.

Wim--

Test looks great to me. RTBC'ing the test. @dawehner has implicitly RTBC'd the few silly/simple LOC I wrote.

Thanks, @dawehner.

WTF? #8 definitely used to be red for the fail patch and green for the pass patch. It seems testbot is having problems. Re-testing them.

Okay, so #8's fail patch *passes* because it doesn't include the actual test!

The reason it was first failing, but now not anymore, is because there was a DrupalCI bug/problem. They fixed it, and OVERWROTE the existing test results. Which is why it now shows up green.

In other words: an intricate set of PRECISELY the right bad conditions!

Kudos to @Mixologic for trying to figure out wtf happened on the testbot side. Without him I'd still be scratching my head.

Rerolling #8 with proper fail/pass patches now. Identical to #8, now just with a proper FAIL patch.

For completeness:

16:59:16 <Mixologic_> WimLeers: so, Im not convinced that that test (https://www.drupal.org/node/2603788#comment-10506080 ) ever legitimately failed.  It had actually failed as a result of drupalci being broken, but even when we re-ran it on the previous deployment, it still passes.
16:59:20 <Druplicon> https://www.drupal.org/node/2603788 => HtmlResponseSubscriber does not call HtmlResponseAttachmentsProcessor on subrequests #2603788: HtmlResponseSubscriber does not call HtmlResponseAttachmentsProcessor on subrequests => 14 comments, 6 IRC mentions
17:00:08 <WimLeers> Mixologic_: I'm sorry if I put you on the wrong track :( But this is a dawehner fail patch. It's very rare that a fail patch of him doesn't actually fail.
17:00:44 <WimLeers> oh lol
17:00:49 <WimLeers> his fail patch doesn't include the test
17:00:56 <WimLeers> so it was really only red by accident the first time
17:01:04 <WimLeers> … because DrupalCI was having a problem, presumably
17:01:05 <WimLeers> my god
17:01:16 <WimLeers> what an intricate set of PRECISELY the right bad conditions!
17:01:29 <WimLeers> 1) Daniel made a mistake, which is rare
17:01:31 <Mixologic_> WimLeers: no problemo, we definitely wanted to dig deeper into anything that might be masking failures - we had deployed some major refactorings (ones that we had been putting off from *before* barcelona), and some of that refactoring included how we handle error conditions, so it was definitely possible that something was awry.
17:01:33 <WimLeers> 2) the patch failed
17:01:39 <WimLeers> 3) the patch only failed because DrupalCI was misbehaving
17:01:51 <WimLeers> 4) I'd seen the failed patch, but it was then re-tested/overwritten when DrupalCI was fixed
17:01:54 <WimLeers> geez!!!
17:02:08 <WimLeers> Mixologic_: Do those points together make sense?
17:02:37 <Mixologic_> 5) DrupalCI was misbehaving because we made a major deployment that we thought might cause misbehavior, so we saved it till after RC1/BCN... ;)
17:02:45 <Mixologic_> WimLeers: it totes makes sense.
17:02:56 <Mixologic_> Closed (cannot reproduce)
17:03:10 <WimLeers> +1
17:03:13 <WimLeers> Mixologic_++

Back to RTBC per #9.

That's weird; the green patch of #15 came back red this time. It looks like a testbot problem, so re-testing. Let's see what happens.

There we go, a red #15 fail-patch, a green #15 regular patch.

Back to RTBC per #9.

#14:

Adding #2595695: 4xx handling using subrequests: no longer able to vary by URL to related issues.

Not sure if we actually want to support using subrequests like this at all?

If by this you mean we shouldn't be making these changes to the exception subscriber, nor be adding these tests, because we shouldn't use subrequests at all for 4xx responses, then I fully agree.

If you mean something else, could you please clarify?

IOW: I think @catch means: Let's get #2595695: 4xx handling using subrequests: no longer able to vary by URL done first, so that this patch becomes simpler, I agree that'd be great.

@catch confirmed in IRC that #28/#29 is what he meant.

Note that #2595695-23: 4xx handling using subrequests: no longer able to vary by URL as of five seconds ago now has a working patch, but it's AFAICT utterly impossible to get rid of subrequests. See that comment.

Therefore, moving back to RTBC, since this patch will hence still be necessary.

To answer the overall question:

Could that issue / contrib project instead implement an additional subscriber that runs processAttachments() for the subrequests that that code creates? That way, we don't introduce side effects for code that uses subrequests for different purposes, such as the comment permalinks.

Yes, it could. But I don't see why it should.

To simplify future archeology: #2469431-181: BigPipe for auth users: first send+render the cheap parts of the page, then the expensive parts is the first iteration of BigPipe that updated HtmlResponseSubscriber also apply to subrequests.

To answer the concrete points put forward:

First of all, I find it hard to reason about whether it makes sense theoretically to process attachments during subrequests or not, because I find the entire concept of subrequests hard to reason about.

I can somewhat see that. But, really, it's very simple IMO: subrequests are just the same like any other request. They should get responses just like any other request. It's just that for some practical reason, it makes sense for a certain (master) request to execute some subrequest(s). In the case of the comment permalink, we essentially do an internal redirect, and use the subrequest's response to answer the master request. In the case of BigPipe, we do something somewhat less orthodox: we take the subrequest's response and parse out the parts we want for the purpose of having an advanced render pipeline ($response->getContent() for the rendered HTML and $response->getAttachments() to get its final (processed) attachments).

Also, HtmlResponseAttachmentsProcessor::processAttachments() does a few different things, including placeholders, assets, and headers. I suspect that some of those things make sense to do for all subrequests, but I'm not convinced that all of those things are.

Which then do not make sense to you?

#37.1: Interesting observation! Interestingly, you uncovered a BigPipe bug in the process: in its current form (#2469431-191: BigPipe for auth users: first send+render the cheap parts of the page, then the expensive parts), it does not work for placeholders attached to the response for a subrequest! So the comment form for /comment/1 is not delivered via BigPipe.

More detailed research:

If we don't make HtmlResponsePlaceholderStrategySubscriber also run for subrequests, that means that BigPipeStrategy doesn't run for the subrequest, which means it gets the default strategy, which means any placeholders in the subrequest actually are not converted to BigPipe placeholders.
So, for the entity.comment.canonical route (CommentController::commentPermalink()), the subrequest is rendered, and the SingleFlushStrategy is applied, which means that by the time commentPermalink() gets a response for the subrequest back, all HTML is rendered in its final form.
However, if we update HtmlResponsePlaceholderStrategySubscriber to also run on subrequests, then the placeholders (for e.g. the comment form) are turned into BigPipe placeholders. Those placeholders then are ignored by HtmlResponseBigPipeSubscriber, because it's a subrequest. So the subrequest's response that is returned as the answer to the master request then is processed once again by HtmlResponseBigPipeSubscriber, but this time it is the master request, and therefore this time it does get processed.

IOW: with the current BigPipe patch, /comment/1 gets the comment form delivered as already-rendered, uncacheable HTML; with the change you propose (make HtmlResponsePlaceholderStrategySubscriber also operate on all requests), the comment form is delivered via BigPipe.

Done in this reroll, as requested.

#37.2: Totally agreed! Follow-up created: #2613550: Rationalize & document use of isMasterRequest()-checks in event subscribers.

YAY!!!!!!!!