Views access plugins alter route's access requirements, altered routes must have associated cache tags, because site builders can modify the view (the config entity), causing different access alterations. Unless the responses for this route always have corresponding cache tags (i.e. the View config entity), it is possible for access to be denied once to the anonymous user, and then for the resulting page to be cached forever. The View config entity's cache tag does not get set because the route's controller is never executed, because access checking happens at the routing level, not at the controller level.
Quoting the patch in #606840: Enable internal page cache by default:
diff --git a/core/modules/views/src/Tests/Plugin/AccessTest.php b/core/modules/views/src/Tests/Plugin/AccessTest.php
index 4bc77db..c756706 100644
--- a/core/modules/views/src/Tests/Plugin/AccessTest.php
+++ b/core/modules/views/src/Tests/Plugin/AccessTest.php
@@ -7,6 +7,7 @@
namespace Drupal\views\Tests\Plugin;
+use Drupal\Core\Cache\Cache;
use Drupal\views\Tests\ViewTestData;
use Drupal\views\Views;
@@ -101,6 +102,13 @@ function testStaticAccessPlugin() {
// termination event fires. Simulate that here.
$this->container->get('router.builder')->rebuildIfNeeded();
+ // Clear the page cache.
+ // @todo Remove this. The root cause is that the access plugins alters the
+ // route's access requirements. That means that the 403 from above does
+ // not have any cache tags, so modifying the View entity does not cause
+ // the cached 403 page to be invalidated.
+ Cache::invalidateTags(['rendered']);
+
$this->assertTrue($access_plugin->access($this->normalUser));
$this->drupalGet('test_access_static');
Comments
Comment #1
amateescu commentedWorking on this.
Comment #2
amateescu commentedComment #3
amateescu commentedThis should do it.
Comment #5
amateescu commentedHopefully better this time :)
Comment #6
wim leersThis looks wonderfully simple so far :)
Can we add an
assertCacheTag($view->getCacheTags()[0])assertion?Also, I don't think the current code works for multiple cache tags when
cache_tags: …is set in a*.routing.ymlfile (EDIT: nope, that does look fine, but that is still worth having a tiny simple test for). We'll want a test asserting that that is supported. Look at996eb16/#2464659: Routes that are varied by the 'user.permissions' cache context for anonymous users must also get the anonymous Role's cache tag for a similar set of changes, where you can probably c/p a bunch to build a test.Comment #8
amateescu commentedThanks for reviewing :)
Fixed both points and the test fails from above.
Comment #9
amateescu commentedGo bot!
Comment #10
amateescu commentedTrying once more, same patch as #8.
Comment #12
amateescu commentedBetter fix for the failures in #5, it seems the test is the one that needs to be fixed.
Comment #13
wim leersLooks great. I had too much of a hand in this issue to be able to RTBC it. Assigning to Fabianx for review.
Related: #2472281: 404/403 responses for non-existing nodes are cached in Page Cache/reverse proxy, are not invalidated when the node is created.
Comment #14
fabianx commentedRTBC, I can't wait for #2463009: Introduce CacheableResponseInterface: consolidate ways of setting X-Drupal-Cache-Tags/Contexts headers to make this API simpler, so you can just call addCacheTags() like normally, but for now this should suffice.
Not sure assertTrue(isset()) is a common pattern, but don't know any better, so leaving for a core committer to take a look.
Just one thing, not sure it affects RTBC state:
Maybe we add a comment here, why we have to do that?
Comment #15
amateescu commentedIs this comment helpful or too "wordy"? :)
Comment #16
fabianx commentedNope, this is great!
Comment #17
wim leersGreat work! :)
Comment #18
amateescu commentedSo #2472281: 404/403 responses for non-existing nodes are cached in Page Cache/reverse proxy, are not invalidated when the node is created happened in the meantime which fixed our problem in a more generic way, the only thing we need to do here now is to remove the manual cache invalidation from the test.
Comment #19
wim leersLooks great!
Comment #20
fabianx commentedAre we sure the additional view tags are not helpful anymore?
What happens if something was 'okay', but is 404 now?
This patch takes care of that the, 40x patch does not - as far as I can see.
Comment #21
wim leersIf something "was okay" and now is 404 due to changes in the access plugin configuration for a view, then the response will have the View's cache tag already anyway, because it contains the rendered View, and therefore the bubbled cache tag.
Comment #22
fabianx commentedRight that is the same for nodes, therefore back to RTBC for #18.
Comment #23
catchYes the 40X patch was really nifty, and this issue shows why.