Problem/Motivation

#2559011: Ensure form tokens are marked max-age=0 is the last blocker for #2429617: Make D8 2x as fast: Dynamic Page Cache: context-dependent page caching (for *all* users!). It was split off from #2526472: Ensure forms are marked max-age=0, but have a way to opt-in to being cacheable, and uses a more granular, more precise approach to ensure SmartCache doesn't cache forms that should not be cached.

So, I tested #2429617-339: Make D8 2x as fast: Dynamic Page Cache: context-dependent page caching (for *all* users!) + #2559011-13: Ensure form tokens are marked max-age=0 (the RTBC patch there), to see if SmartCache would still be green. But, unfortunately, the answer is no: #2560959-4: Testing issue for #2429617 is red.

The reason for the failures is:

  public function prepareForm($form_id, &$form, FormStateInterface &$form_state) {
    …

    // Only update the action if it is not already set.
    if (!isset($form['#action'])) {
      $form['#action'] = $this->buildFormAction();
    }

    …
}

  protected function buildFormAction() {
    …

    return $parsed['path'] . ($parsed['query'] ? ('?' . UrlHelper::buildQuery($parsed['query'])) : '');
  }

In other words: any form that doesn't have $form['#action'] set, gets that generated automatically, based on the URL's path and all query arguments.

Proposed resolution

When FormBuilder automatically generates $form['#action'], also associate the 'url.path' and 'url.query_args' cache contexts.

Remaining tasks

Review.

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#6 interdiff.txt2.08 KBwim leers
#6 2561775-5.patch16.86 KBwim leers
#4 2561775-4.patch15.64 KBwim leers
#2 2561775-2.patch14.07 KBwim leers

Comments

Wim Leers created an issue. See original summary.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: wim leers » Unassigned
Status: Active » Needs review
Related issues: +#2561757: Follow-up for #2443457: improve \Drupal\block\Tests\Views\DisplayBlockTest::testBlockEmptyRendering()
StatusFileSize
new 2561775-2.patch14.07 KB

This implements the proposed solution. #2560959-10: Testing issue for #2429617 should prove that this patch together with SmartCache will be green.

A bunch of the changes here have been split off to #2561757: Follow-up for #2443457: improve \Drupal\block\Tests\Views\DisplayBlockTest::testBlockEmptyRendering(). That should be able to land very easily (it makes that test insensitive to cacheability changes in forms) and make this patch a lot smaller.

Status: Needs review » Needs work

The last submitted patch, 2: 2561775-2.patch, failed testing.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Needs review
StatusFileSize
new 2561775-4.patch15.64 KB

Rebased.

catch’s picture

catch
he/him
Primary language English
commented

Isn't the bug itself a duplicate of #2504139: Blocks containing a form include the form action in the cache, so they always submit to the first URL the form was viewed at?

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
StatusFileSize
new 2561775-5.patch16.86 KB
new interdiff.txt2.08 KB

Feedback from @Moshe and @tim.plunkett in chat:

Moshe Weitzman [16:15] @wimleers: its a little weird for buildFormAction to have the code for for building the action but not supply the contexts
Moshe Weitzman [16:16] i suggest that buildFormAction return a 2 item array.  an action (string), and a list of contexts
Moshe Weitzman [16:16] nitpicking a bit there
Tim Plunkett [16:28]  @wimleers: @moshe.weitzman in my original iteration, it was $form = $this->buildFormAction($form), and it did whatever it needed to do
Tim Plunkett [16:29] or maybe it was $this->buildFormAction(&$form), something like that

Done. Went with Tim's concrete implementation suggestion because he's a Form system maintainer.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

#5: interesting, yes, it is! But this one goes for a much simpler solution: it just adds the missing cache contexts.

That other issue goes further, it ensures that blocks with forms are cacheable across pages.

So IMO, this is a better first step: correctness. The other issue is a next step: improved cache hit ratio.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Closed (duplicate)

From IRC:

16:42:02 <WimLeers> catch: replied to your comment
16:43:14 <catch> WimLeers: I'm not sure 'just correctness' is a good argument if it means potentially very bad cache hit ratios.
16:43:58 <catch> WimLeers: also the other issue has about the same or less changes.
16:44:01 <WimLeers> catch: Well, in HEAD forms break
16:44:13 <WimLeers> catch: this issue makes it so that they don't break
16:44:24 <catch> WimLeers: do they actually break or just submit to the wrong url?
16:45:04 <WimLeers> catch: isn't that "breaking"?
16:45:46 <catch> WimLeers: if the form submission goes through and it does what it's supposed to though.
16:45:46 <WimLeers> catch: but, I totally see your point
16:45:57 <catch> WimLeers: compared to say, 100,000 extra entries in cache.render

Hence I think this issue is effectively superseded by #2504139. I've worked on getting that to green, starting at #2504139-81: Blocks containing a form include the form action in the cache, so they always submit to the first URL the form was viewed at, and if all is well, #2504139-84: Blocks containing a form include the form action in the cache, so they always submit to the first URL the form was viewed at should be green.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Related issues: +#2504139: Blocks containing a form include the form action in the cache, so they always submit to the first URL the form was viewed at