See \Drupal\search_api\Controller\TaskController::executeTasksAccess(). Due to #2722237: "Local actions" block doesn't take cache data of route access information into account we had to set the cache max-age to 0 previously, but once we can rely on that fix in Drupal 8.7 (or 8.6.2, really – but I don’t think we want to start depending on patch versions), we should be able to change that to make the access check properly cacheable.
| Comment | File | Size | Author |
|---|---|---|---|
| #10 | 3004270-10--proper_task_cache_tag.patch | 4.41 KB | drunken monkey |
| #10 | 3004270-10--proper_task_cache_tag--tests_only.patch | 4.41 KB | drunken monkey |
Comments
Comment #2
drunken monkeyComment #3
drunken monkeyOK, the fix would be in 8.6.2 (or 8.6.3? The next non-security release, in any case.) already, too – but I don’t think that will matter to us.
Comment #4
drunken monkeyComment #5
stefvanlooveren commentedComment #6
stefvanlooveren commentedComment #7
stefvanlooveren commentedLast file should have proper line endings (unix)
Comment #8
drunken monkeyThanks a lot for the patch! And also for re-posting with correct line endings – also something new learned, for both of us, I guess. ;)
Unfortunately, the patch is not quite complete like this: for the correct behavior we need to set the same cache metadata for both forbidden and allowed access results – see the attached patch.
I’m also uploading a debugging patch that just removes the metadata completely, to make sure that this correctly fails our test suite. (Otherwise, we would also need test coverage for this.)
Comment #9
drunken monkeyOK, the test bot likes this and a manual test just now also confirmed that this is working fine.
Setting back to “Postponed”, to be committed once we depend on 8.7.
Comment #10
drunken monkeyOK, “likes this” was wrong, of course, as I actually wanted the debug patch to fail.
Well, seems we need a new test for this after all. Let’s see …
Comment #11
drunken monkeyWould have been nice to get another test/review, but as this has proper test coverage now, I think we can be fairly confident it works as intended.
So: committed.
Thanks again for your help on this!