Problem/Motivation

I'm working on #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees, and this is unfortunately a hard blocker (hence this is marked critical, because #1805054 is marked critical).

To be able to efficiently determine the visibility of menu blocks, whose visibility may depend on the user viewing it (!), #1805054 must implement SystemMenuBlock::access().

Unfortunately, BlockAccessController::checkAccess() applies the block visibility checks after calling the block plugin's ::access() method. Whereas block visibility is about as cheap as it gets, it's likely that many blocks will have complex ::access() logic. StatisticsPopularBlock is the only example in current HEAD, but with #1805054, that will include SystemMenuBlock.

Proposed resolution

Simply apply block visibility before calling the block plugin's ::access().

Remaining tasks

None.

User interface changes

None.

API changes

None.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers’s picture

Status: Active » Needs review
FileSize
1.35 KB
chx’s picture

Status: Needs review » Reviewed & tested by the community

The linked issue is related but is not blocking because there the problem is that this function is not called at all. (Opsie)

With that said, everything in BlockAccessController::checkAccess (when they run at least; only on op view they do) returns FALSE so the order doesn't matter.

Good to go, as far as I can see.

Related issue: if ($visibility['path']['visibility'] < BLOCK_VISIBILITY_PHP) { now has no else {} counterpart and so this is entirely broken not to mention you can't even set it BLOCK_VISIBILITY_PHP any more... this needs a cleanup.

tim.plunkett’s picture

Additional +1.
This is not a problem because nothing in BlockAccessController::checkAccess returns TRUE, only FALSE. So we're not accidentally granting access without consulting the block plugin.

  • Commit e071723 on 8.x by webchick:
    Issue #2272081 by Wim Leers: BlockAccessController::checkAccess() should...
webchick’s picture

Status: Reviewed & tested by the community » Fixed

At first I was confused why we didn't add tests for this, but it's still doing the same thing, just faster.

Committed and pushed to 8.x. Thanks!

Wim Leers’s picture

Issue tags: -sprint

Status: Fixed » Closed (fixed)

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