Problem/Motivation
As discovered in #3456128: Dynamic Page Cache uncachable on Drupal 10.3 with Admin Toolbar Tools and Update modules the UpdateManagerAccessCheck makes some pages uncacheable. In that issue catch suggested we could dynamically create the routes if the allow_authorize_operations setting is TRUE. Unfortunately this causes issues with local tasks and local actions if the routes don't exist, so they too need to be dynamically created. Instead it may be simpler to just disable access to the routes if the setting is FALSE.
Steps to reproduce
Proposed resolution
When no access is configured in settings, set all the routes to _access: FALSE, this means we don't need to do any other access checks at all on runtime - they're just denied. When access is configured, we only have to do a permissions check, not the uncacheable check based on settings. Deprecate the custom access check for removal in 12.x
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
Issue fork drupal-3458403
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
- 3458403-conditionally-disable-access
changes, plain diff MR !8620
Comments
Comment #3
mstrelan commentedComment #4
mstrelan commentedThis is effectively the same as removing the
setCacheMaxAge(0)call inUpdateManagerAccessCheck::access. Should we just remove that instead?Comment #5
catchOne comment on the MR but looks great otherwise.
Comment #6
mstrelan commentedDeprecated the access check and added a CR. Not sure if versions are correct.
Comment #7
smustgrave commentedHey sorry but can the issue summary be updated also.
Just reading the summary I wouldn't of known we are deprecating UpdateManagerAccessCheck for example
Comment #8
catchUpdated the issue summary.
Comment #9
smustgrave commentedThanks! CR also reads well.
Comment #10
quietone commentedAll questions here are answered, I read the MR and CR and they are clear. The only thing left would be changes for a 10.4 backport, but that doesn't need to be done now.
Comment #15
catchCommitted/pushed to 11.x, thanks!
I was about to ask for a backport MR, then I realised it would just be the same change with the first hunk of the diff removed, so went ahead and made that change locally for both 11.0.x and 10.4.x. Reclassifying as a bug report since this fixes performance issues.
Comment #16
mstrelan commentedAny chance of a back port to 10.3 where the regression was introduced?
Comment #18
catchWas a bit hesitant because of the new class, but I think it's fine and better than shipping the bug for 11 months - backported to 10.3.x too.