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.
Comment | File | Size | Author |
---|---|---|---|
#1 | block_visibility_first_for_perf-2272081-1.patch | 1.35 KB | Wim Leers |
Comments
Comment #1
Wim LeersComment #2
chx CreditAttribution: chx commentedComment #3
chx CreditAttribution: chx commentedThe 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.
Comment #4
tim.plunkettAdditional +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.
Comment #6
webchickAt 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!
Comment #7
Wim Leers