Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comments
Comment #2
BerdirGo testbot, tell me I'm crazy, I dare you.
Comment #4
BerdirOk, 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'].
Comment #5
BerdirDid not want to change status.
Comment #6
BerdirTurns 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.
Comment #7
BerdirComment #8
BerdirAlso added query argument tests now to cover the problem identified by #2847972: Missing context on UserLoginBlock causes render-cache poisoning of the "destination".
Comment #11
BerdirComment #13
dawehnerThis API is weird that hard to use IMHO. Of course this is not the fault of this issue.
Isn't this cache context basically
'url'
together with the<current>
route?Comment #15
BerdirIt 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']
Comment #16
dawehnerComment #17
Wim LeersI 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.
@see \Drupal\Core\Form\FormBuilder::renderPlaceholderFormAction()
Can be made more specific:
url.query_args:destination
.I'm missing
X-Drupal-Dynamic-Cache
response header validation: the first one should beMISS
, the second should beHIT
.After that, this is RTBC.
Comment #18
Berdir1. 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.
Comment #19
Wim LeersPerfect.
Comment #20
catchCommitted/pushed to 8.4.x, thanks! This is a nice find.
Fixed the following on commit: