From 3ce43cc2b110e110e4d77aff160c7ca012be92e6 Mon Sep 17 00:00:00 2001
From: Eric Smith <ericsmith@catalyst.net.nz>
Date: Tue, 4 Feb 2025 09:45:09 +1300
Subject: [PATCH 1/9] Issue #3504114: Add WIP test to show error
---
.../NodeAccessCacheRedirectWarning.php | 70 +++++++++++++++++++
1 file changed, 70 insertions(+)
create mode 100644 core/modules/node/tests/src/Functional/NodeAccessCacheRedirectWarning.php
diff --git a/core/modules/node/tests/src/Functional/NodeAccessCacheRedirectWarning.php b/core/modules/node/tests/src/Functional/NodeAccessCacheRedirectWarning.php
new file mode 100644
index 000000000000..461d96f2ba37
--- /dev/null
+++ b/core/modules/node/tests/src/Functional/NodeAccessCacheRedirectWarning.php
@@ -0,0 +1,70 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Drupal\Tests\node\Functional;
+
+use Drupal\Core\Cache\CacheableMetadata;
+
+/**
+ * Tests the node access grants cache context service.
+ *
+ * @group node
+ * @group Cache
+ */
+class NodeAccessCacheRedirectWarning extends NodeTestBase {
+
+ /**
+ * {@inheritdoc}
+ */
+ protected static $modules = ['block', 'node_access_test_empty'];
+
+ /**
+ * {@inheritdoc}
+ */
+ protected $defaultTheme = 'stark';
+
+ /**
+ * {@inheritdoc}
+ */
+ protected function setUp(): void {
+ parent::setUp();
+
+ node_access_rebuild();
+ }
+
+ /**
+ * Quick demonstration of the differences in cache contexts.
+ *
+ * The intent here was to visit the nodes to view the error but for whatever
+ * reason I can't seem to trigger the redirect warning this way. Needs work.
+ */
+ public function testNodeAccessCacheRedirectWarning(): void {
+ $this->drupalPlaceBlock('local_tasks_block');
+
+ $this->assertTrue(\Drupal::moduleHandler()->hasImplementations('node_grants'));
+
+ $author = $this->drupalCreateUser([
+ 'create page content',
+ 'edit own page content',
+ 'view own unpublished content',
+ ]);
+ $this->drupalLogin($author);
+
+ $node = $this->drupalCreateNode(['uid' => $author->id(), 'status' => 0]);
+
+ $access = $node->access('view', $author, TRUE);
+ $unpublished_metadata = CacheableMetadata::createFromObject($access);
+
+ // Reset the node access cache to check the published node.
+ \Drupal::entityTypeManager()->getAccessControlHandler('node')->resetCache();
+
+ $node->setPublished();
+ $node->save();
+ $access = $node->access('view', $author, TRUE);
+ $published_metadata = CacheableMetadata::createFromObject($access);
+
+ $this->assertSame($unpublished_metadata->getCacheContexts(), $published_metadata->getCacheContexts());
+ }
+
+}
--
GitLab
From 7c76520ea3264c996ab967687ae63892f26e25af Mon Sep 17 00:00:00 2001
From: Eric Smith <ericsmith@catalyst.net.nz>
Date: Thu, 6 Mar 2025 18:08:15 +1300
Subject: [PATCH 2/9] Issue #3504114: If edit own content is added, then the
cache context user is used, if edit any permissions then only user.permission
is added
---
.../NodeAccessCacheRedirectWarning.php | 22 ++++++++++---------
1 file changed, 12 insertions(+), 10 deletions(-)
diff --git a/core/modules/node/tests/src/Functional/NodeAccessCacheRedirectWarning.php b/core/modules/node/tests/src/Functional/NodeAccessCacheRedirectWarning.php
index 461d96f2ba37..5438ed7680c5 100644
--- a/core/modules/node/tests/src/Functional/NodeAccessCacheRedirectWarning.php
+++ b/core/modules/node/tests/src/Functional/NodeAccessCacheRedirectWarning.php
@@ -4,8 +4,6 @@
namespace Drupal\Tests\node\Functional;
-use Drupal\Core\Cache\CacheableMetadata;
-
/**
* Tests the node access grants cache context service.
*
@@ -46,25 +44,29 @@ public function testNodeAccessCacheRedirectWarning(): void {
$author = $this->drupalCreateUser([
'create page content',
- 'edit own page content',
+ 'edit any page content',
'view own unpublished content',
]);
$this->drupalLogin($author);
$node = $this->drupalCreateNode(['uid' => $author->id(), 'status' => 0]);
- $access = $node->access('view', $author, TRUE);
- $unpublished_metadata = CacheableMetadata::createFromObject($access);
+ $this->drupalGet($node->toUrl());
+
+ $node->setPublished();
+ $node->save();
+
+ $this->drupalGet($node->toUrl());
+
+ $node->setUnpublished();
+ $node->save();
- // Reset the node access cache to check the published node.
- \Drupal::entityTypeManager()->getAccessControlHandler('node')->resetCache();
+ $this->drupalGet($node->toUrl());
$node->setPublished();
$node->save();
- $access = $node->access('view', $author, TRUE);
- $published_metadata = CacheableMetadata::createFromObject($access);
- $this->assertSame($unpublished_metadata->getCacheContexts(), $published_metadata->getCacheContexts());
+ $this->drupalGet($node->toUrl());
}
}
--
GitLab
From 9ad37e776a53bf5a44398941f231a21e189f86a3 Mon Sep 17 00:00:00 2001
From: catch <6915-catch@users.noreply.drupalcode.org>
Date: Thu, 6 Mar 2025 09:32:11 +0000
Subject: [PATCH 3/9] Rename test.
---
...directWarning.php => NodeAccessCacheRedirectWarningTest.php} | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
rename core/modules/node/tests/src/Functional/{NodeAccessCacheRedirectWarning.php => NodeAccessCacheRedirectWarningTest.php} (95%)
diff --git a/core/modules/node/tests/src/Functional/NodeAccessCacheRedirectWarning.php b/core/modules/node/tests/src/Functional/NodeAccessCacheRedirectWarningTest.php
similarity index 95%
rename from core/modules/node/tests/src/Functional/NodeAccessCacheRedirectWarning.php
rename to core/modules/node/tests/src/Functional/NodeAccessCacheRedirectWarningTest.php
index 5438ed7680c5..8946ca75fefe 100644
--- a/core/modules/node/tests/src/Functional/NodeAccessCacheRedirectWarning.php
+++ b/core/modules/node/tests/src/Functional/NodeAccessCacheRedirectWarningTest.php
@@ -10,7 +10,7 @@
* @group node
* @group Cache
*/
-class NodeAccessCacheRedirectWarning extends NodeTestBase {
+class NodeAccessCacheRedirectWarningTest extends NodeTestBase {
/**
* {@inheritdoc}
--
GitLab
From f9f1f2ba0f4b6534d80a7ce9c50dbb4fc5106846 Mon Sep 17 00:00:00 2001
From: catch <6915-catch@users.noreply.drupalcode.org>
Date: Thu, 6 Mar 2025 10:20:41 +0000
Subject: [PATCH 4/9] Add extra cache context.
---
core/modules/node/src/NodeGrantDatabaseStorage.php | 1 +
1 file changed, 1 insertion(+)
diff --git a/core/modules/node/src/NodeGrantDatabaseStorage.php b/core/modules/node/src/NodeGrantDatabaseStorage.php
index eea6cc100127..9a4c77d2d9ab 100644
--- a/core/modules/node/src/NodeGrantDatabaseStorage.php
+++ b/core/modules/node/src/NodeGrantDatabaseStorage.php
@@ -121,6 +121,7 @@ public function access(NodeInterface $node, $operation, AccountInterface $accoun
// know it for a fact.
$set_cacheability = function (AccessResult $access_result) use ($operation) {
$access_result->addCacheContexts(['user.node_grants:' . $operation]);
+ $access_result->addCacheContexts(['user.roles.authenticated']);
if ($operation !== 'view') {
$access_result->setCacheMaxAge(0);
}
--
GitLab
From 711552bd9e03eed2ddc041f484ffb57ded6e85ce Mon Sep 17 00:00:00 2001
From: catch <6915-catch@users.noreply.drupalcode.org>
Date: Thu, 6 Mar 2025 18:13:14 +0000
Subject: [PATCH 5/9] Move user.roles:authenticated cache context around.
---
core/modules/node/src/NodeAccessControlHandler.php | 9 ++++++++-
core/modules/node/src/NodeGrantDatabaseStorage.php | 1 -
2 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/core/modules/node/src/NodeAccessControlHandler.php b/core/modules/node/src/NodeAccessControlHandler.php
index 963ab53ded41..f4811bdb21dd 100644
--- a/core/modules/node/src/NodeAccessControlHandler.php
+++ b/core/modules/node/src/NodeAccessControlHandler.php
@@ -207,6 +207,14 @@ protected function checkViewAccess(NodeInterface $node, AccountInterface $accoun
// we need to add the node as a cacheable dependency.
$cacheability->addCacheableDependency($node);
+ // Due to the check below, it is not possible to rely only on account
+ // permissions to determine whether the 'view own unpublished content'
+ // permission can be checked, instead we also need to check if the user has
+ // the authenticated role. Just in case anonymous and authenticated users
+ // are both granted the 'view own unpublished content' permission and also
+ // have otherwise identical permissions.
+ $cacheability->addCacheContext(['user.roles.authenticated']);
+
if ($node->isPublished()) {
return NULL;
}
@@ -216,7 +224,6 @@ protected function checkViewAccess(NodeInterface $node, AccountInterface $accoun
return NULL;
}
- $cacheability->addCacheContexts(['user.roles:authenticated']);
// The "view own unpublished content" permission must not be granted
// to anonymous users for security reasons.
if (!$account->isAuthenticated()) {
diff --git a/core/modules/node/src/NodeGrantDatabaseStorage.php b/core/modules/node/src/NodeGrantDatabaseStorage.php
index 9a4c77d2d9ab..eea6cc100127 100644
--- a/core/modules/node/src/NodeGrantDatabaseStorage.php
+++ b/core/modules/node/src/NodeGrantDatabaseStorage.php
@@ -121,7 +121,6 @@ public function access(NodeInterface $node, $operation, AccountInterface $accoun
// know it for a fact.
$set_cacheability = function (AccessResult $access_result) use ($operation) {
$access_result->addCacheContexts(['user.node_grants:' . $operation]);
- $access_result->addCacheContexts(['user.roles.authenticated']);
if ($operation !== 'view') {
$access_result->setCacheMaxAge(0);
}
--
GitLab
From 025c6e98df936d134ec599b031133cfdbab9515b Mon Sep 17 00:00:00 2001
From: catch <6915-catch@users.noreply.drupalcode.org>
Date: Thu, 6 Mar 2025 20:21:50 +0000
Subject: [PATCH 6/9] Add the node grants cache context in exactly one
additional situation.
---
core/modules/node/src/NodeAccessControlHandler.php | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/core/modules/node/src/NodeAccessControlHandler.php b/core/modules/node/src/NodeAccessControlHandler.php
index f4811bdb21dd..2ee7d4965d20 100644
--- a/core/modules/node/src/NodeAccessControlHandler.php
+++ b/core/modules/node/src/NodeAccessControlHandler.php
@@ -213,12 +213,12 @@ protected function checkViewAccess(NodeInterface $node, AccountInterface $accoun
// the authenticated role. Just in case anonymous and authenticated users
// are both granted the 'view own unpublished content' permission and also
// have otherwise identical permissions.
- $cacheability->addCacheContext(['user.roles.authenticated']);
if ($node->isPublished()) {
return NULL;
}
$cacheability->addCacheContexts(['user.permissions']);
+ $cacheability->addCacheContexts(['user.roles:authenticated']);
if (!$account->hasPermission('view own unpublished content')) {
return NULL;
@@ -230,7 +230,17 @@ protected function checkViewAccess(NodeInterface $node, AccountInterface $accoun
return NULL;
}
+ // When access is granted due to the 'view own unpublished content'
+ // permission and for no other reason, node grants are bypassed. However,
+ // to ensure the full set of cacheable metadata is available to variation
+ // cache, additionally add the node_grants cache context so that if the
+ // status or the owner of the node changes, cache redirects will continue to
+ // reflect the latest state without needing to be invalidated.
$cacheability->addCacheContexts(['user']);
+ if ($this->moduleHandler->hasImplementations('node_grants')) {
+ $cacheability->addCacheContexts(['user.node_grants:view']);
+ }
+
if ($account->id() != $node->getOwnerId()) {
return NULL;
}
--
GitLab
From e4be93b0d7a393b80abc3662dce11e93f97ada9b Mon Sep 17 00:00:00 2001
From: catch <6915-catch@users.noreply.drupalcode.org>
Date: Thu, 6 Mar 2025 20:57:58 +0000
Subject: [PATCH 7/9] Move things back where they were, but keep comment.
---
.../node/src/NodeAccessControlHandler.php | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/core/modules/node/src/NodeAccessControlHandler.php b/core/modules/node/src/NodeAccessControlHandler.php
index 2ee7d4965d20..743f614e61c8 100644
--- a/core/modules/node/src/NodeAccessControlHandler.php
+++ b/core/modules/node/src/NodeAccessControlHandler.php
@@ -207,23 +207,23 @@ protected function checkViewAccess(NodeInterface $node, AccountInterface $accoun
// we need to add the node as a cacheable dependency.
$cacheability->addCacheableDependency($node);
- // Due to the check below, it is not possible to rely only on account
- // permissions to determine whether the 'view own unpublished content'
- // permission can be checked, instead we also need to check if the user has
- // the authenticated role. Just in case anonymous and authenticated users
- // are both granted the 'view own unpublished content' permission and also
- // have otherwise identical permissions.
-
if ($node->isPublished()) {
return NULL;
}
$cacheability->addCacheContexts(['user.permissions']);
- $cacheability->addCacheContexts(['user.roles:authenticated']);
if (!$account->hasPermission('view own unpublished content')) {
return NULL;
}
+ // Due to the check below, it is not possible to rely only on account
+ // permissions to determine whether the 'view own unpublished content'
+ // permission can be checked, instead we also need to check if the user has
+ // the authenticated role. Just in case anonymous and authenticated users
+ // are both granted the 'view own unpublished content' permission and also
+ // have otherwise identical permissions.
+ $cacheability->addCacheContexts(['user.roles:authenticated']);
+
// The "view own unpublished content" permission must not be granted
// to anonymous users for security reasons.
if (!$account->isAuthenticated()) {
--
GitLab
From a993579b4eceb5ff4898e0847978cea6448acbb6 Mon Sep 17 00:00:00 2001
From: Kristiaan Van den Eynde <kristiaan@factorial.io>
Date: Mon, 31 Mar 2025 10:50:14 +0200
Subject: [PATCH 8/9] Add PseudoCacheContext and DependencyVariation, do not
implement yet.
---
core/core.services.yml | 6 ++
.../Core/Cache/Context/PseudoCacheContext.php | 43 +++++++++
.../Drupal/Core/Cache/DependencyVariation.php | 87 +++++++++++++++++++
3 files changed, 136 insertions(+)
create mode 100644 core/lib/Drupal/Core/Cache/Context/PseudoCacheContext.php
create mode 100644 core/lib/Drupal/Core/Cache/DependencyVariation.php
diff --git a/core/core.services.yml b/core/core.services.yml
index bb73e29d505b..84abfc516c4b 100644
--- a/core/core.services.yml
+++ b/core/core.services.yml
@@ -152,6 +152,12 @@ services:
tags:
- { name: cache.context }
+ # Pseudo cache context, not intended for direct use.
+ cache_context.pseudo:
+ class: Drupal\Core\Cache\Context\PseudoCacheContext
+ tags:
+ - { name: cache.context }
+
# Complex cache contexts, that depend on the routing system.
cache_context.route:
class: Drupal\Core\Cache\Context\RouteCacheContext
diff --git a/core/lib/Drupal/Core/Cache/Context/PseudoCacheContext.php b/core/lib/Drupal/Core/Cache/Context/PseudoCacheContext.php
new file mode 100644
index 000000000000..7e1300bfa158
--- /dev/null
+++ b/core/lib/Drupal/Core/Cache/Context/PseudoCacheContext.php
@@ -0,0 +1,43 @@
+<?php
+
+namespace Drupal\Core\Cache\Context;
+
+use Drupal\Core\Cache\CacheableMetadata;
+
+/**
+ * Defines the PseudoCacheContext service.
+ *
+ * This cache context will always return the value of '1', but is not intended
+ * to be added manually. Instead, users are expected to use the associated
+ * helper class \Drupal\Core\Cache\DependencyVariation instead.
+ *
+ * Cache context ID: 'pseudo'.
+ *
+ * @see \Drupal\Core\Cache\DependencyVariation
+ */
+class PseudoCacheContext implements CalculatedCacheContextInterface {
+
+ const ID = 'pseudo';
+
+ /**
+ * {@inheritdoc}
+ */
+ public static function getLabel() {
+ return t('Pseudo');
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function getContext($parameter = NULL) {
+ return 1;
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function getCacheableMetadata($parameter = NULL) {
+ return new CacheableMetadata();
+ }
+
+}
diff --git a/core/lib/Drupal/Core/Cache/DependencyVariation.php b/core/lib/Drupal/Core/Cache/DependencyVariation.php
new file mode 100644
index 000000000000..212b0590eb53
--- /dev/null
+++ b/core/lib/Drupal/Core/Cache/DependencyVariation.php
@@ -0,0 +1,87 @@
+<?php
+
+namespace Drupal\Core\Cache;
+
+use Drupal\Core\Cache\Context\PseudoCacheContext;
+
+/**
+ * Helper class to add a pseudo cache context as a dependency.
+ *
+ * This is intended to be used when you have a code flow that varies based on a
+ * (property of a) dependency, but you have no way to represent said variation
+ * with a regular cache context. An example would be the passed in entity to an
+ * access control handler: You cannot know where this entity came from, so how
+ * would you know what cache context to use?
+ *
+ * You are supposed to use this helper class to wrap the original dependency and
+ * then pass in this class as a cacheable dependency itself. It will then
+ * compile the cache context identifier for you and add it to the cacheable
+ * metadata this helper class was added to.
+ *
+ * @ingroup cache
+ */
+class DependencyVariation implements CacheableDependencyInterface {
+
+ use CacheableDependencyTrait;
+
+ public function __construct(CacheableDependencyInterface $dependency) {
+ $this->cacheTags = $dependency->getCacheTags();
+ $this->cacheMaxAge = $dependency->getCacheMaxAge();
+ assert(empty($dependency->getCacheContexts()), 'A dependency passed into DependencyVariation should not contain any cache contexts.');
+ sort($this->cacheTags);
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function getCacheTags() {
+ return [];
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function getCacheContexts() {
+ $identifier = implode(',', $this->cacheTags) . '|' . $this->cacheMaxAge;
+ return PseudoCacheContext::ID . ':' . $identifier;
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function getCacheMaxAge() {
+ return Cache::PERMANENT;
+ }
+
+ /**
+ * Checks if a cache context identifier represents a pseudo cache context.
+ *
+ * @param string $cache_context
+ * The cache context identifier.
+ *
+ * @return bool
+ * Whether the cache context identifier is for a pseudo cache context.
+ */
+ public static function isPseudoCacheContext(string $cache_context): bool {
+ return str_starts_with($cache_context, PseudoCacheContext::ID . ':');
+ }
+
+ /**
+ * Parses a pseudo cache context into a cacheable dependency.
+ *
+ * @param string $cache_context
+ * The cache context identifier.
+ *
+ * @return \Drupal\Core\Cache\CacheableMetadata
+ * The cacheable metadata that was embedded in the identifier. Note that
+ * this will only ever contain cache tags and max-age.
+ */
+ public static function parsePseudoCacheContext(string $cache_context): CacheableMetadata {
+ [$cache_context_id, $parameter] = explode(':', $cache_context, 2);
+ [$cache_tag_string, $max_age] = explode('|', $parameter);
+ return (new CacheableMetadata())
+ ->addCacheTags(explode(',', $cache_tag_string))
+ ->setCacheMaxAge($max_age);
+ }
+
+}
--
GitLab
From c39d91c02a6dbb94d9233b7086e090170ce3eab0 Mon Sep 17 00:00:00 2001
From: Kristiaan Van den Eynde <kristiaan@factorial.io>
Date: Mon, 31 Mar 2025 11:09:43 +0200
Subject: [PATCH 9/9] phpcs, not sure if this will pass
---
core/lib/Drupal/Core/Cache/DependencyVariation.php | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/core/lib/Drupal/Core/Cache/DependencyVariation.php b/core/lib/Drupal/Core/Cache/DependencyVariation.php
index 212b0590eb53..207df45fbd5a 100644
--- a/core/lib/Drupal/Core/Cache/DependencyVariation.php
+++ b/core/lib/Drupal/Core/Cache/DependencyVariation.php
@@ -77,7 +77,7 @@ public static function isPseudoCacheContext(string $cache_context): bool {
* this will only ever contain cache tags and max-age.
*/
public static function parsePseudoCacheContext(string $cache_context): CacheableMetadata {
- [$cache_context_id, $parameter] = explode(':', $cache_context, 2);
+ [, $parameter] = explode(':', $cache_context, 2);
[$cache_tag_string, $max_age] = explode('|', $parameter);
return (new CacheableMetadata())
->addCacheTags(explode(',', $cache_tag_string))
--
GitLab