Commit 039eae17 authored by alexpott's avatar alexpott

Issue #3016781 by arpad.rozsa, Berdir, catch, Wim Leers, kristiaanvandeneynde:...

Issue #3016781 by arpad.rozsa, Berdir, catch, Wim Leers, kristiaanvandeneynde: node_node_access() is too eager in adding the user cache context — fixing this makes some pages faster
parent 777d7fc5
......@@ -103,11 +103,11 @@ public function testUsersWithoutPermission() {
$message = "The 'access in-place editing' permission is required.";
}
else {
$message = '';
$message = "The 'edit any article content' permission is required.";
}
$body = Json::decode($response->getBody());
$this->assertIdentical($message, $body['message']);
$this->assertSame($message, $body['message']);
}
}
......
......@@ -974,33 +974,30 @@ function node_form_system_themes_admin_form_submit($form, FormStateInterface $fo
/**
* Implements hook_node_access().
*/
function node_node_access(NodeInterface $node, $op, $account) {
function node_node_access(NodeInterface $node, $op, AccountInterface $account) {
$type = $node->bundle();
$access = AccessResult::neutral();
switch ($op) {
case 'create':
return AccessResult::allowedIfHasPermission($account, 'create ' . $type . ' content');
$access = AccessResult::allowedIfHasPermission($account, 'create ' . $type . ' content');
case 'update':
if ($account->hasPermission('edit any ' . $type . ' content')) {
return AccessResult::allowed()->cachePerPermissions();
}
else {
return AccessResult::allowedIf($account->hasPermission('edit own ' . $type . ' content') && ($account->id() == $node->getOwnerId()))->cachePerPermissions()->cachePerUser()->addCacheableDependency($node);
$access = AccessResult::allowedIfHasPermission($account, 'edit any ' . $type . ' content');
if (!$access->isAllowed() && $account->hasPermission('edit own ' . $type . ' content')) {
$access = $access->orIf(AccessResult::allowedIf($account->id() == $node->getOwnerId())->cachePerUser()->addCacheableDependency($node));
}
break;
case 'delete':
if ($account->hasPermission('delete any ' . $type . ' content')) {
return AccessResult::allowed()->cachePerPermissions();
}
else {
return AccessResult::allowedIf($account->hasPermission('delete own ' . $type . ' content') && ($account->id() == $node->getOwnerId()))->cachePerPermissions()->cachePerUser()->addCacheableDependency($node);
$access = AccessResult::allowedIfHasPermission($account, 'delete any ' . $type . ' content');
if (!$access->isAllowed() && $account->hasPermission('delete own ' . $type . ' content')) {
$access = $access->orIf(AccessResult::allowedIf($account->id() == $node->getOwnerId()))->cachePerUser()->addCacheableDependency($node);
}
default:
// No opinion.
return AccessResult::neutral();
break;
}
return $access;
}
/**
......
......@@ -12,7 +12,7 @@
* @group Cache
* @group cacheability_safeguards
*/
class NodeAccessAutoBubblingTest extends NodeTestBase {
class NodeAccessCacheabilityTest extends NodeTestBase {
use AssertPageCacheContextsAndTagsTrait;
......@@ -67,4 +67,45 @@ public function testNodeAccessCacheabilitySafeguard() {
$this->assertNoCacheContext('user.node_grants:view');
}
/**
* Tests that the user cache contexts are correctly set.
*/
public function testNodeAccessCacheContext() {
// Create a user, with edit/delete own content permission.
$test_user1 = $this->drupalCreateUser([
'access content',
'edit own page content',
'delete own page content',
]);
$this->drupalLogin($test_user1);
$node1 = $this->createNode(['type' => 'page']);
// User should be able to edit/delete their own content.
// Therefore after the access check in node_node_access the user cache
// context should be added.
$this->drupalGet('node/' . $node1->id() . '/edit');
$this->assertCacheContext('user');
$this->drupalGet('node/' . $node1->id() . '/delete');
$this->assertCacheContext('user');
// Create a user without edit/delete permission.
$test_user2 = $this->drupalCreateUser([
'access content',
]);
$this->drupalLogin($test_user2);
$node2 = $this->createNode(['type' => 'page']);
// The user shouldn't have access to the node edit/delete pages.
// Therefore after the access check in node_node_access the user permissions
// cache context should be added.
$this->drupalGet('node/' . $node2->id() . '/edit');
$this->assertCacheContext('user.permissions');
$this->drupalGet('node/' . $node2->id() . '/delete');
$this->assertCacheContext('user.permissions');
}
}
......@@ -71,7 +71,6 @@ public function testPageCacheTags() {
'route',
'theme',
'timezone',
'user',
// The placed block is only visible on certain URLs through a visibility
// condition.
'url.path',
......@@ -81,6 +80,8 @@ public function testPageCacheTags() {
// These two cache contexts are added by BigPipe.
'cookies:big_pipe_nojs',
'session.exists',
'user.roles:anonymous',
'user.roles:authenticated',
];
// Full node page 1.
......@@ -107,7 +108,6 @@ public function testPageCacheTags() {
'config:block.block.bartik_page_title',
'node_view',
'node:' . $node_1->id(),
'user:0',
'user:' . $author_1->id(),
'config:filter.format.basic_html',
'config:color.theme.bartik',
......@@ -164,7 +164,6 @@ public function testPageCacheTags() {
// FinishResponseSubscriber adds this cache tag to responses that have the
// 'user.permissions' cache context for anonymous users.
'config:user.role.anonymous',
'user:0',
]);
}
......
......@@ -188,11 +188,10 @@ protected function doTestRenderedOutput(AccountInterface $account, $check_cache
if ($check_cache) {
$keys = $cache_plugin->getRowCacheKeys($view->result[$index]);
$user_context = !$account->hasPermission('edit any test content') ? 'user' : 'user.permissions';
$cache = [
'#cache' => [
'keys' => $keys,
'contexts' => ['languages:language_interface', 'theme', $user_context],
'contexts' => ['languages:language_interface', 'theme', 'user.permissions'],
],
];
$element = $render_cache->get($cache);
......
......@@ -37,12 +37,11 @@ public function testBigPipe() {
$this->drupalGet('');
$this->assertBigPipePlaceholderReplacementCount(1);
// Node page: 3 placeholders:
// Node page: 2 placeholders:
// 1. messages
// 2. local tasks block
// 3. comment form
// 2. comment form
$this->drupalGet($node->toUrl());
$this->assertBigPipePlaceholderReplacementCount(3);
$this->assertBigPipePlaceholderReplacementCount(2);
}
/**
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment