Problem/Motivation

  1. #2559011: Ensure form tokens are marked max-age=0 made sure the form token (a CSRF token) was only set for authenticated users, and ensured it specified max-age=0.
  2. #2571995: GET forms shouldn't have CSRF tokens by default refined this to make sure that GET forms don't get a form token by default, i.e. only POST forms get this.
  3. #2463567: Push CSRF tokens for forms to placeholders + #lazy_builder then moves the rendering of the form token into a #lazy_builder callback, which means the rendered form can actually be cached, because the form token is rendered later, and therefore the rendered form is not always by definition bound to the current user/session, which is what made it uncacheable. But it keeps the max-age=0 that point 1 introduced, because removing that merits further discussion.
  4. This issue is about removing the max-age=0 that point 1 introduced, and having that further discussion.

#2552873-18: node/1 flamegraphs also points out how #2571909: CommentForm selects using the user formatted name caused a very big performance regression. #2571909 made the comment form no longer personalized per user, so we thought we made the form cacheable. But we forgot about the form token setting max-age=0, which then makes the full node display uncacheable!

Proposed resolution

Remove max-age=0.

Remaining tasks

TBD.

User interface changes

None.

API changes

None.

Data model changes

None.

Why this should (maybe) be an RC target

This is a significant performance improvement, but also a significant change in behavior. See #27 through #31.

CommentFileSizeAuthor
#45 interdiff.txt2.68 KBWim Leers
#45 cacheable_forms-2578855-45.patch32.11 KBWim Leers
#43 interdiff.txt2.68 KBWim Leers
#43 cacheable_forms-2578855-42.patch30.14 KBWim Leers
#40 interdiff.txt2.08 KBWim Leers
#40 cacheable_forms-2578855-40.patch29.34 KBWim Leers
#3 cacheable_forms-2578855-3.patch681 bytesWim Leers
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,009 pass(es), 110 fail(s), and 7 exception(s). View
#4 cacheable_forms-2578855-4.patch3.61 KBWim Leers
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,028 pass(es), 108 fail(s), and 7 exception(s). View
#4 interdiff.txt2.96 KBWim Leers
#9 cacheable_forms-2578855-9.patch7.74 KBWim Leers
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,377 pass(es), 107 fail(s), and 7 exception(s). View
#9 interdiff.txt4.22 KBWim Leers
#12 cacheable_forms-2578855-12.patch13.49 KBWim Leers
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,441 pass(es), 92 fail(s), and 7 exception(s). View
#12 interdiff.txt5.9 KBWim Leers
#15 cacheable_forms-2578855-15.patch25.11 KBWim Leers
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,117 pass(es), 95 fail(s), and 7 exception(s). View
#15 interdiff.txt11.69 KBWim Leers
#17 cacheable_forms-2578855-17.patch26.5 KBWim Leers
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,203 pass(es), 47 fail(s), and 5 exception(s). View
#17 interdiff.txt1.44 KBWim Leers
#22 cacheable_forms-2578855-22.patch28.82 KBWim Leers
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,220 pass(es), 38 fail(s), and 5 exception(s). View
#22 interdiff.txt2.4 KBWim Leers
#35 cacheable_forms-2578855-35.patch28.68 KBWim Leers
#36 cacheable_forms-2578855-36.patch27.89 KBWim Leers
#36 interdiff.txt849 bytesWim Leers
#39 cacheable_forms-2578855-39.patch29.34 KBWim Leers
#39 interdiff.txt5.52 KBWim Leers
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

Wim Leers created an issue. See original summary.

Wim Leers’s picture

#2552873-18: node/1 flamegraphs also points out how #2571909: CommentForm selects using the user formatted name caused a very big performance regression. #2571909 made the comment form no longer personalized per user, so we thought we made the form cacheable. But we forgot about the form token setting max-age=0, which then makes the full node display uncacheable!

Wim Leers’s picture

Status: Active » Needs review
FileSize
681 bytes
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,009 pass(es), 110 fail(s), and 7 exception(s). View

So this is basically what we want to do. I know some unit tests break. Let's see what else breaks.

Wim Leers’s picture

FileSize
3.61 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,028 pass(es), 108 fail(s), and 7 exception(s). View
2.96 KB

The "contact" forms end up being cached incorrectly due to missing cacheability metadata. Here's a fix for that.

In fixing that, discovered that ContactAuthenticatedUserTest is only passing because it only does assertions that verify certain things are absent. But it's a 404 response, so everything is absent.

Status: Needs review » Needs work

The last submitted patch, 4: cacheable_forms-2578855-4.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
7.74 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,377 pass(es), 107 fail(s), and 7 exception(s). View
4.22 KB

Here we go, unit tests updated. The unit test became approximately two orders of magnitude easier to understand.

Status: Needs review » Needs work

The last submitted patch, 9: cacheable_forms-2578855-9.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
13.49 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,441 pass(es), 92 fail(s), and 7 exception(s). View
5.9 KB

A bunch of fixes, still working on the rest.

Status: Needs review » Needs work

The last submitted patch, 12: cacheable_forms-2578855-12.patch, failed testing.

The last submitted patch, 12: cacheable_forms-2578855-12.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
Related issues: +#2487600: #access should support AccessResultInterface objects or better has to always use it
FileSize
25.11 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,117 pass(es), 95 fail(s), and 7 exception(s). View
11.69 KB

By fixing the missing language_select-related cacheability metadata, I uncovered a few bits of unpleasantness. The biggest one of those is also the first real surprise: turns out #2487600: #access should support AccessResultInterface objects or better has to always use it was not 100% correct/complete; the cacheability metadata for an access result on #access is lost, but only when A) access is denied (i.e. an early return occurs) and B) it is a child of an element being rendered through a template.
This reroll fixes that and adds missing test coverage.

The way I discovered this, is thanks to the language_select field depending on the ContentLanguageSettings config entity, of which one exists per entity bundle. The settings stored in that config entity determine if the widget to select a language shows up at all, and which values are available. And due to missing cacheability metadata, that is exactly the information that was missing.
Turns out that there is both a field access hook (language_entity_field_access()) and a plain form alter (language_form_alter()) cause #access to be set to forbid access. I think we only need to keep one of them, but fixing that is out of scope for this issue. What did need to be fixed here, was their cacheability metadata: they both depended on the corresponding contentLanguageSettings config entity, but they omitted this cacheability metadata.
Finally, to make matters worse, ContentLanguageSettings::loadByEntityTypeBundle() creates ephemeral instances of this config entity (i.e. ones that aren't saved), which has some default values. This is fine until you deal with cacheability metadata: then you need to be notified of changes, and in this case, we aren't, because only when the language settings are first customized, the config entity is actually saved, which means only changes after that point will cause the proper invalidations. I've added a work-around for this. It can be made obsolete later, but this is a perfectly fine, perfectly acceptable work-around.
For all of this, I've also added test coverage.

Status: Needs review » Needs work

The last submitted patch, 15: cacheable_forms-2578855-15.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
26.5 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,203 pass(es), 47 fail(s), and 5 exception(s). View
1.44 KB

This fixes the node form related test failures. This was quite simple: max-age=0 is the appropriate choice, because the node form tightly integrates with TempStore. For analogous reasons, it never makes sense to cache the node preview (nor does that make sense at all) — this already was the case, this interdiff just slightly refines that.

moshe weitzman’s picture

because the node form tightly integrates with TempStore.

Does the mere viewing of the form interact with tempstore? Or does that happen once you submit the form?

Wim Leers’s picture

Does the mere viewing of the form interact with tempstore? Or does that happen once you submit the form?

Yes, the mere viewing: it retrieves data from TempStore, and for TempStore we don't have cache tags, so we can't know when it has changed, hence the need for max-age=0.

moshe weitzman’s picture

Thanks for clarifying. This pattern is pretty bad for any form aspires to show up on a non-admin page. I wouldnt know where to document such a thing.

Status: Needs review » Needs work

The last submitted patch, 17: cacheable_forms-2578855-17.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
Related issues: +#2464409: Search results should bubble rendered entity cache tags and set list cache tags
FileSize
28.82 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,220 pass(es), 38 fail(s), and 5 exception(s). View
2.4 KB

This fixes the Search form test failures, but that will really only be fixable once #2464409: Search results should bubble rendered entity cache tags and set list cache tags lands, so I've added a @todo for that.

Status: Needs review » Needs work

The last submitted patch, 22: cacheable_forms-2578855-22.patch, failed testing.

The last submitted patch, 15: cacheable_forms-2578855-15.patch, failed testing.

The last submitted patch, 17: cacheable_forms-2578855-17.patch, failed testing.

The last submitted patch, 22: cacheable_forms-2578855-22.patch, failed testing.

Wim Leers’s picture

Issue tags: +rc target triage, +Performance

I suspect this should now be done in Drupal 8.1, but given the fact that this can yield an enormous speedup on /node/X pages (i.e. pages where the comment form is visible), it probably should at least be triaged to determine whether it should be an RC target or not.

moshe weitzman’s picture

Yeah, this meets the Performance Critical criteria listed at #1744302: [meta] Resolve known performance regressions in Drupal 8. See recent node/1 flame graphs. Furthermore, popular contrib/custom forms can appear on almost all pages (e.g. fivestar widget).

Berdir’s picture

It is a big performance improvement but it's also a rather big behavior change. I don't really see how we'd do this exactly without some sort of BC break? Unless we make it configurable, but then we'd essentially require a module that displays a form in e.g. a block to deal with both possible values of that option?

Maybe we can allow forms to opt-in, possibly by setting max-age to a non-zero value themself?

I know we had exactly this kind of discussion before and I know that Wim really doesn't like opt-in performance improvements, but in the previous discussions, we weren't in the RC phase yet.

catch’s picture

Maybe we can allow forms to opt-in, possibly by setting max-age to a non-zero value themself?

That makes sense to me, I don't think we can change the form behaviour across the board at the moment.

Even with making the behaviour opt-in there's a bit of a change for form_alters/widgets that they'll be expected to provide correct cacheability metadata whereas at the moment they might not and could still work by accident, but that seems OK to me considering the performance gain.

moshe weitzman’s picture

I'll argue that we worked on criteria for Performance Critical for a reason. We are allowed to break BC for Criticals. If we can get a (tentative) green light here, I'm hoping that @wimleers will clear up the final fails.

xjm’s picture

Issue summary: View changes

Linking the discussion of the rc-target-ness of this in the summary.

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/Entity/Entity/EntityFormDisplay.php
    @@ -171,7 +171,7 @@ public function buildForm(FieldableEntityInterface $entity, array &$form, FormSt
    -        $form[$name]['#access'] = $items->access('edit');
    +        $form[$name]['#access'] = $items->access('edit', NULL, TRUE);
    

    I'm confused, didn't we threw an exception when #access was not an object?

  2. +++ b/core/modules/editor/src/Element.php
    @@ -53,6 +53,7 @@ function preRenderTextFormat(array $element) {
    +    $element['#cache']['tags'][] = 'config:editor_list';
    
    +++ b/core/modules/filter/src/Element/TextFormat.php
    @@ -210,6 +210,8 @@ public static function processFormat(&$element, FormStateInterface $form_state,
     
    +    $element['#cache']['tags'][] = 'config:filter_format_list';
    +
    
    +++ b/core/modules/language/language.module
    @@ -163,6 +163,7 @@ function language_process_language_select($element) {
    +    $element['#cache']['tags'][] = 'config:language_content_settings_list';
    

    Don't we want to use \Drupal::entityManager()->getDefinition('editor')->getListCacheTags()

  3. +++ b/core/modules/node/src/Controller/NodePreviewController.php
    @@ -26,6 +26,7 @@ public function view(EntityInterface $node_preview, $view_mode_id = 'full', $lan
         unset($build['#cache']);
    +    $build['#cache']['max-age'] = 0;
    

    Do we actually need both lines, it feels a bit redundant. At least it could be merged into one line, if needed

  4. +++ b/core/modules/node/src/Plugin/Search/NodeSearch.php
    @@ -518,6 +518,11 @@ public function searchFormAlter(array &$form, FormStateInterface $form_state) {
    +        'contexts' => [
    +          'url.query_args',
    +        ],
    

    I guess its not worth to specify the exact gazillion possible query parameters here ...

  5. +++ b/core/tests/Drupal/Tests/Core/Form/FormBuilderTest.php
    @@ -845,19 +845,32 @@ public function testFormTokenCacheability($token, $is_authenticated, $expected_f
    +    // impossible (and unnecessary) to set a form token if the user is not
    +    // logged in, because there is no session, and hence no CSRF token.
    

    Just a general question, you could have an anonymous user session, at least Drupal supports that, right? Do we need to take that into account somewhere?

catch’s picture

@Moshe is #30 not a tentative green light?

Wim Leers’s picture

FileSize
28.68 KB

First, a straight reroll of #22.

(Exact same changes, the context has just become slightly smaller, hence slightly different patch size.)

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
27.89 KB
849 bytes

#22 said

This fixes the Search form test failures, but that will really only be fixable once #2464409: Search results should bubble rendered entity cache tags and set list cache tags lands, so I've added a @todo for that

That issue has since landed. So, removed that work-around.

This change in #22 fixed 5 fails that existed in #17. Let's see how many return. (I tested the first two, and neither returned.)

Status: Needs review » Needs work

The last submitted patch, 36: cacheable_forms-2578855-36.patch, failed testing.

Wim Leers’s picture

So, comparing #36 to #22 (sadly #35 didn't run tests because I stupidly forgot to mark it NR):

  • Still 5 fails in Aggregator
  • Only 1 instead of 2 fails in Comment
  • Still 3 fails in Field
  • 2 fails that have reappeared because of #36's revert.
  • Still 1 fail in Taxonomy

So, Comment's cacheability metadata was improved in the last weeks. And Search will still need some fixing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
5.52 KB
29.34 KB

#29:

I know we had exactly this kind of discussion before and I know that Wim really doesn't like opt-in performance improvements, but in the previous discussions, we weren't in the RC phase yet.

Indeed, at this time, it must be opt-in. Even a year ago, it would have to have been opt-in, because forms are everywhere.

#33:

  1. No, #2487600: #access should support AccessResultInterface objects or better has to always use it made it possible to use AccessResultInterface objects. Requiring them to always be AccessResultInterface objects would have caught this problem months ago… :(
  2. That'd be better, yes. Done.
  3. Agreed, done.
  4. Future optimization :)
  5. Just a general question, you could have an anonymous user session, at least Drupal supports that, right?

    Yes, of course.

    But … Drupal has never set a form token for anonymous users! This is copied from FormBuilder when it was created in #2112807: Move the form builder functions in form.inc to a form service, where it was just copied from form.inc, >2 years ago (revision 82eb2924cfcc40dcedd981e4462176a6d9256f0f):

        // Add a token, based on either #token or form_id, to any form displayed to
        // authenticated users. This ensures that any submitted form was actually
        // requested previously by the user and protects against cross site request
        // forgeries.
        // This does not apply to programmatically submitted forms. Furthermore,
        // since tokens are session-bound and forms displayed to anonymous users are
        // very likely cached, we cannot assign a token for them.
        // During installation, there is no $user yet.
        if ($user && $user->isAuthenticated() && !$form_state['programmed']) {
          // Form constructors may explicitly set #token to FALSE when cross site
          // request forgery is irrelevant to the form, such as search forms.
          if (isset($form['#token']) && $form['#token'] === FALSE) {
            unset($form['#token']);
          }
          // Otherwise, generate a public token based on the form id.
          else {
            $form['#token'] = $form_id;
            $form['form_token'] = array(
              '#id' => Html::getUniqueId('edit-' . $form_id . '-form-token'),
              '#type' => 'token',
              '#default_value' => $this->csrfToken->get($form['#token']),
              // Form processing and validation requires this value, so ensure the
              // submitted form value appears literally, regardless of custom #tree
              // and #parents being set elsewhere.
              '#parents' => array('form_token'),
            );
          }
        }
    

    And in Drupal 7's drupal_prepare_form():

    if (!empty($user->uid) && !$form_state['programmed']) {
    
Wim Leers’s picture

This fixes the last fail in Comment.

The last submitted patch, 39: cacheable_forms-2578855-39.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 40: cacheable_forms-2578855-40.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
30.14 KB
2.68 KB

This makes cacheable forms opt-in for the first time!

It opts in the following two forms:

  1. CommentForm
  2. MessageForm (for /contact)

Next: removing as many of the changes as possible, to make this patch as simple as possible. Now that we don't aim to make all forms cacheable, we can defer many of the bugfixes in this patch to follow-up issues against 8.1.

Wim Leers’s picture

Ugh, #40's interdiff was correct, but the patch didn't contain it, it was identical to #39… Sorry about that.

Wim Leers’s picture

Ugh, it's worse. #43 contains the patch matching the #40 interdiff. So at least we'll know the effect of just that.

The attachments to this comment are associated with the text written in #43 (even though this interdiff is identical to that in #43).

The last submitted patch, 43: cacheable_forms-2578855-42.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 45: cacheable_forms-2578855-45.patch, failed testing.

xjm’s picture

Issue tags: -rc target triage

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

nielsaers’s picture

Does anyone have a status update on this issue?

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.