Disable placeholdering (and BigPipe) on unsafe requests to help find forms as fast as possible (to allow EnforcedResponses to work)

Oh, wow! Great edge case discovery! I can't believe nobody encountered that yet in the past six months! I'll need to step through this with a debugger to verify that it is indeed essential to disable BigPipe on POST requests. I'm not sure about that yet. But it definitely would be the simplest solution.

However, this is definitely not a problem for all forms. For example, for the comment form (e.g. at /node/1), it works fine. So, what makes Poll module's form handling special?

Reply to #3 from IRC:

14:06:24 <WimLeers> berdir: follow-up Q: https://www.drupal.org/node/2702609#comment-11051647
14:12:16 <berdir> WimLeers: that's a good question, I don't know
14:13:47 <berdir> WimLeers: ah, might have an idea. comment posts to a special route, where it doesn't use a lazy_builder
14:14:00 <bojanz> berdir: reviewed, looks good. That was a tough one :)
14:14:21 <berdir> WimLeers: see CommentForm, where it sets #action. I'd guess that if for some reason that condition isn't triggered, then it's broken too

That seems very plausible.

I still need/want to investigate this further before settling on the approach in #2. But if we end up going with the approach in #2, we should add:

// @todo Remove the isMethodSafe() condition as soon as Drupal gets rid of \Drupal\Core\Form\EnforcedResponseException.

I know there is an issue for that somewhere (to make sure all forms have dedicated controllers), but I can't seem to find it right now. I think @catch created it.

catch just pointed me to the issue I was looking for: #2503429: [PP-*] Allow both AJAX and non-AJAX forms to POST to dedicated URLs. And #2699489: FormBuilder $ajax_form_request check does not check which AJAX form is being requested is related too.

Train of thought

Yeah, while BigPipe is affected and leads to a bug here, the bigger issue is with lazy builders auto-placeholdering in general here.

A block should just not be auto-placeholdered - if it contains a form without a dedicated URL to POST to.

While the normal processing for placeholders is still early enough to catch things like EnforcedFormException (?) or what that thing is called and specifically also does so (IIRC), it is just something that should in theory never be placeholdered at all.

However the block can perfectly be BigPipe'd and such Performance saved and as such just disabling auto-placeholdering also is wrong.

On the other hand, we disable most caching for non-safe methods for that exact reason (so that blocks and other things can be processed), so the patch is fine.

Once we have tackled the larger problem of dedicated FORM submissions urls, we can both activate caching and BigPipe again.

Therefore:

TLDR

Because all render caching is disabled for non-safe methods, the same should be true for BiPipe => RTBC.

This still needs tests, at minimum. And the comment in #4.

CNW for tests and from IRC:

10:02 < Fabianx-screen> WimLeers: It is the right thing to do, we also disable all render caching for that reason.
10:03 < Fabianx-screen> WimLeers: Maybe we could also disable placeholdering, then BigPipe would not need a special case though.

I think it is better to disable (auto-)placeholdering in the Renderer directly, the same as we don't retrieve anything from caches for POST requests.

Performance gains come back automatically with dedicated URLs.

BigPipe could then still special case this or not ...

On the other hand, we disable most caching for non-safe methods for that exact reason (so that blocks and other things can be processed), so the patch is fine.

Render cache can indeed prevent a form from being rendered.

However, lazy builders do not prevent a form from being rendered. Lazy builders don't cause problems. In fact, that's what we are witnessing here: the form is being processed just fine!

The problem is that — just like Berdir already said in the IS — BigPipe calls lazy builders after the response has already begun being sent. And so if Form API throws this exception, it is by definition impossible to send this enforced response (contained in/passed by \Drupal\Core\Form\EnforcedResponseException), because the response is already being sent.

Therefore I disagree with your analysis.

But I agree — like I already said in #3 and #4, that this is probably the right solution.

That said, I find the idea to not do placeholdering on post requests quite interesting. render caching is off anyway, and doing it immediately might actually improve performance because we find and process the form earlier in the request.

Oh!!! Now that actually makes a ton of sense.

@Berdir++
@Berdir++
@Berdir++

But that'd require a change in the render system too, not just in BigPipe. We should have a big fat @TODO to the code that disables placeholdering on POST requests then. And of course, if we don't have placeholdering, then BigPipe automatically is made ineffective… (though we should still disable BigPipe explicitly on POST, because it's possible somebody is doing manual placeholdering).

So then we can basically retitle this issue to Disable auto-placeholdering on POST requests to help find forms as fast as possible., because that will be the primary change.

Thoughts?

#11: Hey, I want some of Berdir's credit, too for this :D.

Yes, that was the reasoning behind my idea / analysis.

No placeholdering => We find the form faster => We do not profit from placeholdering anyway when all render caching is off.

It is just additional work to do.

And yes should disable in BigPipe, too for safety reasons.

Fabianx++
Fabianx++

:)

There!

And yes should disable in BigPipe, too for safety reasons.

What do you mean by safety reasons? IMO the only reason to disable it for BigPipe explicitly as well is: because manual placeholdering.

#13: Yes, exactly because of manual placeholders.

Ok, cool, then we're on the same page :)

Massive update to the issue summary.

Here's expanded test coverage for BigPipeStrategy.

#19 had a random failure in the functional JS test. After re-testing, all is well again.

And here's the thing that prevents all placeholdering, both manual and automatic (and within automatic: both the direct and bubbled). i.e. this now avoids as many placeholders as possible.

And now this is all done! :)

And now its RTBC - Great work!

+++ b/core/lib/Drupal/Core/Render/PlaceholderingRenderCache.php
@@ -92,6 +92,11 @@ public function __construct(RequestStack $request_stack, CacheFactoryInterface $
   public function get(array $elements) {
+    // @todo remove this check when https://www.drupal.org/node/2367555 lands.
+    if (!$this->requestStack->getCurrentRequest()->isMethodSafe()) {
+      return FALSE;
+    }
+
     // When rendering placeholders, special case auto-placeholdered elements:

Minor but the @todo links to #2367555: Deprecate EnforcedResponseException support which is marked duplicate of #2503429: [PP-*] Allow both AJAX and non-AJAX forms to POST to dedicated URLs.

Also we can't resolve this @todo until we actually remove EnforcedResponseException, which is going to have to be in Drupal 9. I really hope we can get to the point sooner than later where these conditions never evaluate to true, but they'll have to sit there until support is removed.

Minor but the @todo links to #2367555] which is marked duplicate of #2503429: [PP-*] Allow both AJAX and non-AJAX forms to POST to dedicated URLs.

That is intentional. Then at least all links point to the same URL, rather than two different URLs, which is more confusing.

I'd be happy to update all existing links to point to the same URL (2503429), if you'd prefer that though.

I think linking to the active issue is better, and would prefer lack of consistency to making things consistently worse.

Either way the comment needs updating since that issue cannot remove those hunks anyway per #27.

Then all existing comments are wrong too: they all state that upon that issue landing, the additional checks can be removed, which per #27/#29 is not true. Shall I update all of the others as well then?

Discussed with Wim in irc - I've re-opened the issue mark duplicate to be about deprecating the functionality, which makes the comment correct and we needed an issue for that anyway.

Committed/pushed to 8.2.x and cherry-picked to 8.1.x. Thanks!

Also committed & pushed to the contrib module for Drupal 8.0: http://drupalcode.org/project/big_pipe.git/commit/2331bf5

Turns out the changes we made to auto-placeholdering in fact also were necessary for AJAXy forms in lazy builders to work also without BigPipe: #2710939: AddToCartFormatter lazy builder causes erroneous Ajax submits, when a second form is present in Commerce for D8 was fixed by this :)

Retroactively tagging contributed project blocker: blocked both Poll & Commerce.

In case anyone gets a similar error but it's not fixed by this patch, there's a fair chance that it might be #2699489: FormBuilder $ajax_form_request check does not check which AJAX form is being requested instead which has similar symptoms but a different cause.

Oops, that was already linked apparently.

Same issue return in #2723407: Not compatible with 8.1.1 big_pipe but seems fixed with a patch on BigPipe.

#41: Great.

#42: That's … bizarre. Can you open a new issue and STR?

#42: see #2712935-7: Messages are not rendered last, meaning those messages set within placeholders only appear on the next request, I'm fairly certain that's what you're seeing.