Problem/Motivation

LoginFormBlock currently adds the route cache context because it adds the #action explicitly including destination.
The problem with that is that this makes this particular block much less cacheable (once per page rather than global)

Proposed resolution

Use a #lazy_builder to to set the #action. This makes it possible to cache the main part of the block.
I see there's #2505339: Stop using getMainRequest() to build $form['#action'] which is clearly related, so I'm expecting some test fails from that, so possibly we need to postpone on that, or merge it into that.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir created an issue. See original summary.

Berdir’s picture

Status: Active » Needs review
FileSize
1.43 KB

Go testbot, tell me I'm crazy, I dare you.

Status: Needs review » Needs work

The last submitted patch, 2: user-login-form-action-magic-2867703-2.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review

Ok, so it looks indeed like this is now failing on an access denied page, which sounds related to #2505339: Stop using getMainRequest() to build $form['#action'].

Berdir’s picture

Status: Needs review » Needs work

Did not want to change status.

Berdir’s picture

Status: Needs work » Needs review
FileSize
2.47 KB

Turns out it is unrelated, this does need a lazy builder.

The previous patch was bogus anyway, but this is passing for me for that test at least.

Berdir’s picture

Title: Remove custom action in LoginFormBlock » Change #action in UserLoginBlock to a #lazy_builder to avoid the route cache context
Berdir’s picture

The last submitted patch, 8: user-login-form-action-magic-2867703-8-tests-only.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 8: user-login-form-action-magic-2867703-8.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review

The last submitted patch, 8: user-login-form-action-magic-2867703-8-tests-only.patch, failed testing.

dawehner’s picture

  1. +++ b/core/modules/user/src/Plugin/Block/UserLoginBlock.php
    @@ -94,7 +92,21 @@ public function build() {
    +    // Crypt::hashBase64('\Drupal\user\Plugin\Block\UserLoginBlock::build');
    +    $placeholder = 'form_action_p_4r8ITd22yaUvXM6SzwrSe9rnQWe48hz9k1Sxto3pBvE';
    +
    +    $form['#attached']['placeholders'][$placeholder] = [
    +      '#lazy_builder' => ['\Drupal\user\Plugin\Block\UserLoginBlock::renderPlaceholderFormAction', []],
    +    ];
    +    $form['#action'] = $placeholder;
    

    This API is weird that hard to use IMHO. Of course this is not the fault of this issue.

  2. +++ b/core/modules/user/src/Plugin/Block/UserLoginBlock.php
    @@ -128,4 +140,18 @@ public function build() {
    +      '#markup' => Url::fromRoute('<current>', [], ['query' => \Drupal::destination()->getAsArray(), 'external' => FALSE])->toString(),
    +      '#cache' => ['contexts' => ['url.path', 'url.query_args']],
    

    Isn't this cache context basically 'url' together with the <current> route?

The last submitted patch, 8: user-login-form-action-magic-2867703-8-tests-only.patch, failed testing.

Berdir’s picture

It is I guess. Most of the code is almost 1:1 copy of what #action does by default, with the only real difference that it forces the destination parameter to always be there, which overrides the default login destination of going to the user profile page. And that this uses the current route and the default code does stuff by hand. Not sure why, possibly because it had to use the master request in the past: #2505339: Stop using getMainRequest() to build $form['#action']

dawehner’s picture

Issue summary: View changes
Wim Leers’s picture

This API is weird that hard to use IMHO. Of course this is not the fault of this issue.

I agree. The reason is of course our incredibly convoluted (because organically grown etc) Render system + Theme system. Nobody should even be aware of #lazy_builder. It should happen for everything that is rendered, automatically. I tried to work towards a solution in #2702061: Unify & simplify render & theme system: component-based rendering (enables pattern library, style guides, interface previews, client-side re-rendering), but it's incredibly difficult because we need to maintain BC… which means we need to keep supporting our existing crappy APIs.


Great find, great report, great patch! :)

This reminds me of #2582797: [Regression] login link has no destination=drupalSettings.path, so dumps you on the profile, which is tangentially related.


  1. +++ b/core/modules/user/src/Plugin/Block/UserLoginBlock.php
    @@ -94,7 +92,21 @@ public function build() {
    +    // Instead of setting an actual action URL, we set the placeholder, which
    +    // will be replaced at the very last moment. This ensures forms with
    +    // dynamically generated action URLs don't have poor cacheability.
    +    // Use the proper API to generate the placeholder, when we have one. See
    +    // https://www.drupal.org/node/2562341. The placholder uses a fixed string
    +    // that is
    +    // Crypt::hashBase64('\Drupal\user\Plugin\Block\UserLoginBlock::build');
    +    $placeholder = 'form_action_p_4r8ITd22yaUvXM6SzwrSe9rnQWe48hz9k1Sxto3pBvE';
    +
    
    @see \Drupal\Core\Form\FormBuilder::prepareForm()
    
  2. +++ b/core/modules/user/src/Plugin/Block/UserLoginBlock.php
    @@ -128,4 +140,18 @@ public function build() {
    +  public static function renderPlaceholderFormAction() {
    

    @see \Drupal\Core\Form\FormBuilder::renderPlaceholderFormAction()

  3. +++ b/core/modules/user/src/Plugin/Block/UserLoginBlock.php
    @@ -128,4 +140,18 @@ public function build() {
    +      '#cache' => ['contexts' => ['url.path', 'url.query_args']],
    

    Can be made more specific: url.query_args:destination.

  4. +++ b/core/modules/user/src/Tests/UserBlocksTest.php
    @@ -81,6 +81,20 @@ public function testUserLoginBlock() {
    +    // Log out again and repeat with a non-403 page including query arguments.
    +    $this->drupalLogout();
    +    $this->drupalPostForm('filter/tips', $edit, t('Log in'), ['query' => ['foo' => 'bar']]);
    +    $this->assertNoText(t('User login'), 'Logged in.');
    +    $this->assertPattern('!<title.*?' . t('Compose tips') . '.*?</title>!', 'Still on the same page after login for allowed page');
    +    $this->assertTrue(strpos($this->getUrl(),'/filter/tips?foo=bar') !== FALSE, 'Correct query arguments are displayed after login');
    +
    +    // Repeat with different query arguments.
    +    $this->drupalLogout();
    +    $this->drupalPostForm('filter/tips', $edit, t('Log in'), ['query' => ['foo' => 'baz']]);
    +    $this->assertNoText(t('User login'), 'Logged in.');
    +    $this->assertPattern('!<title.*?' . t('Compose tips') . '.*?</title>!', 'Still on the same page after login for allowed page');
    +    $this->assertTrue(strpos($this->getUrl(),'/filter/tips?foo=baz') !== FALSE, 'Correct query arguments are displayed after login');
    

    I'm missing X-Drupal-Dynamic-Cache response header validation: the first one should be MISS, the second should be HIT.

After that, this is RTBC.

Berdir’s picture

1. Added a comment referencing that and explain why we need to duplicate it.
2. Added.
3. As discussed, destination can also include other query arguments (currently at least partially broken but will eventually be fixed again). So, needed.
4. Added that to the tests. Since there is actually already a call to that page above, both are HIT, added 3 asserts.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Perfect.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.4.x, thanks! This is a nice find.

Fixed the following on commit:

FILE: /core/modules/user/src/Tests/UserBlocksTest.php
----------------------------------------------------------------------
FOUND 4 ERRORS AFFECTING 2 LINES
----------------------------------------------------------------------
  94 | ERROR | [x] No space found after comma in function call
  94 | ERROR | [x] Expected one space after the comma, 0 found
 103 | ERROR | [x] No space found after comma in function call
 103 | ERROR | [x] Expected one space after the comma, 0 found
----------------------------------------------------------------------
PHPCBF CAN FIX THE 4 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

  • catch committed dcec863 on 8.4.x
    Issue #2867703 by Berdir: Change #action in UserLoginBlock to a #...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.