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

Command icon 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:

Comments

mstrelan created an issue. See original summary.

mstrelan’s picture

Status: Active » Needs work
mstrelan’s picture

This is effectively the same as removing the setCacheMaxAge(0) call in UpdateManagerAccessCheck::access. Should we just remove that instead?

catch’s picture

One comment on the MR but looks great otherwise.

mstrelan’s picture

Status: Needs work » Needs review

Deprecated the access check and added a CR. Not sure if versions are correct.

smustgrave’s picture

Status: Needs review » Needs work
Issue tags: +Needs issue summary update

Hey sorry but can the issue summary be updated also.

Just reading the summary I wouldn't of known we are deprecating UpdateManagerAccessCheck for example

catch’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs issue summary update

Updated the issue summary.

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Thanks! CR also reads well.

quietone’s picture

All 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.

  • catch committed 4692fca5 on 11.x
    Issue #3458403 by mstrelan: Conditionally disable access to update...

  • catch committed 764b6cb7 on 10.4.x
    Issue #3458403 by mstrelan: Conditionally disable access to update...

  • catch committed 7f878e8e on 11.0.x
    Issue #3458403 by mstrelan: Conditionally disable access to update...
catch’s picture

Version: 11.x-dev » 10.4.x-dev
Category: Feature request » Task
Status: Reviewed & tested by the community » Fixed

Committed/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.

mstrelan’s picture

Any chance of a back port to 10.3 where the regression was introduced?

  • catch committed 206a3ac2 on 10.3.x
    Issue #3458403 by mstrelan: Conditionally disable access to update...
catch’s picture

Version: 10.4.x-dev » 10.3.x-dev

Was 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.

Status: Fixed » Closed (fixed)

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