From ebe946b9e70462dcc76660f9340820a8cab8d457 Mon Sep 17 00:00:00 2001
From: Alex Pott <alex.a.pott@googlemail.com>
Date: Wed, 27 Apr 2016 10:59:24 +0100
Subject: [PATCH] Issue #2699627 by catch, dawehner, Wim Leers: url.path cache
context for breadcrumbs is unnecessarily granular
---
core/core.services.yml | 5 ++
.../Cache/Context/PathParentCacheContext.php | 41 +++++++++++++
.../node/src/Tests/NodeTranslationUITest.php | 2 +-
.../system/src/PathBasedBreadcrumbBuilder.php | 6 +-
.../PathBasedBreadcrumbBuilderTest.php | 14 ++---
.../Breadcrumb/Breadcrumb404Test.php | 57 +++++++++++++++++++
.../Context/PathParentCacheContextTest.php | 43 ++++++++++++++
7 files changed, 157 insertions(+), 11 deletions(-)
create mode 100644 core/lib/Drupal/Core/Cache/Context/PathParentCacheContext.php
create mode 100644 core/tests/Drupal/FunctionalTests/Breadcrumb/Breadcrumb404Test.php
create mode 100644 core/tests/Drupal/Tests/Core/Cache/Context/PathParentCacheContextTest.php
diff --git a/core/core.services.yml b/core/core.services.yml
index d9b1839cdbc4..5877a38c10aa 100644
--- a/core/core.services.yml
+++ b/core/core.services.yml
@@ -80,6 +80,11 @@ services:
arguments: ['@request_stack']
tags:
- { name: cache.context }
+ cache_context.url.path.parent:
+ class: Drupal\Core\Cache\Context\PathParentCacheContext
+ arguments: ['@request_stack']
+ tags:
+ - { name: cache.context }
cache_context.url.query_args:
class: Drupal\Core\Cache\Context\QueryArgsCacheContext
arguments: ['@request_stack']
diff --git a/core/lib/Drupal/Core/Cache/Context/PathParentCacheContext.php b/core/lib/Drupal/Core/Cache/Context/PathParentCacheContext.php
new file mode 100644
index 000000000000..76c22fb32caf
--- /dev/null
+++ b/core/lib/Drupal/Core/Cache/Context/PathParentCacheContext.php
@@ -0,0 +1,41 @@
+<?php
+
+namespace Drupal\Core\Cache\Context;
+
+use Drupal\Core\Cache\CacheableMetadata;
+
+/**
+ * Defines a cache context service for path parents.
+ *
+ * Cache context ID: 'url.path.parent'.
+ *
+ * This allows for caching based on the path, excluding everything after the
+ * last forward slash.
+ */
+class PathParentCacheContext extends RequestStackCacheContextBase implements CacheContextInterface {
+
+ /**
+ * {@inheritdoc}
+ */
+ public static function getLabel() {
+ return t('Parent path');
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function getContext() {
+ $request = $this->requestStack->getCurrentRequest();
+ $path_elements = explode('/', trim($request->getPathInfo(), '/'));
+ array_pop($path_elements);
+ return implode('/', $path_elements);
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function getCacheableMetadata() {
+ return new CacheableMetadata();
+ }
+
+}
diff --git a/core/modules/node/src/Tests/NodeTranslationUITest.php b/core/modules/node/src/Tests/NodeTranslationUITest.php
index b4871b7c6e02..b4dce569bd05 100644
--- a/core/modules/node/src/Tests/NodeTranslationUITest.php
+++ b/core/modules/node/src/Tests/NodeTranslationUITest.php
@@ -25,7 +25,7 @@ class NodeTranslationUITest extends ContentTranslationUITestBase {
'theme',
'route',
'timezone',
- 'url.path',
+ 'url.path.parent',
'url.query_args:_wrapper_format',
'user'
];
diff --git a/core/modules/system/src/PathBasedBreadcrumbBuilder.php b/core/modules/system/src/PathBasedBreadcrumbBuilder.php
index 060f1b1a543e..dcfc0732f3fc 100644
--- a/core/modules/system/src/PathBasedBreadcrumbBuilder.php
+++ b/core/modules/system/src/PathBasedBreadcrumbBuilder.php
@@ -136,9 +136,9 @@ public function build(RouteMatchInterface $route_match) {
// /user is just a redirect, so skip it.
// @todo Find a better way to deal with /user.
$exclude['/user'] = TRUE;
- // Because this breadcrumb builder is entirely path-based, vary by the
- // 'url.path' cache context.
- $breadcrumb->addCacheContexts(['url.path']);
+ // Add the url.path.parent cache context. This code ignores the last path
+ // part so the result only depends on the path parents.
+ $breadcrumb->addCacheContexts(['url.path.parent']);
while (count($path_elements) > 1) {
array_pop($path_elements);
// Copy the path elements for up-casting.
diff --git a/core/modules/system/tests/src/Unit/Breadcrumbs/PathBasedBreadcrumbBuilderTest.php b/core/modules/system/tests/src/Unit/Breadcrumbs/PathBasedBreadcrumbBuilderTest.php
index 29f0cea3ba69..eb095a78076d 100644
--- a/core/modules/system/tests/src/Unit/Breadcrumbs/PathBasedBreadcrumbBuilderTest.php
+++ b/core/modules/system/tests/src/Unit/Breadcrumbs/PathBasedBreadcrumbBuilderTest.php
@@ -142,7 +142,7 @@ public function testBuildOnFrontpage() {
$breadcrumb = $this->builder->build($this->getMock('Drupal\Core\Routing\RouteMatchInterface'));
$this->assertEquals([], $breadcrumb->getLinks());
- $this->assertEquals(['url.path'], $breadcrumb->getCacheContexts());
+ $this->assertEquals(['url.path.parent'], $breadcrumb->getCacheContexts());
$this->assertEquals([], $breadcrumb->getCacheTags());
$this->assertEquals(Cache::PERMANENT, $breadcrumb->getCacheMaxAge());
}
@@ -159,7 +159,7 @@ public function testBuildWithOnePathElement() {
$breadcrumb = $this->builder->build($this->getMock('Drupal\Core\Routing\RouteMatchInterface'));
$this->assertEquals([0 => new Link('Home', new Url('<front>'))], $breadcrumb->getLinks());
- $this->assertEquals(['url.path'], $breadcrumb->getCacheContexts());
+ $this->assertEquals(['url.path.parent'], $breadcrumb->getCacheContexts());
$this->assertEquals([], $breadcrumb->getCacheTags());
$this->assertEquals(Cache::PERMANENT, $breadcrumb->getCacheMaxAge());
}
@@ -194,7 +194,7 @@ public function testBuildWithTwoPathElements() {
$breadcrumb = $this->builder->build($this->getMock('Drupal\Core\Routing\RouteMatchInterface'));
$this->assertEquals([0 => new Link('Home', new Url('<front>')), 1 => new Link('Example', new Url('example'))], $breadcrumb->getLinks());
- $this->assertEquals(['url.path', 'user.permissions'], $breadcrumb->getCacheContexts());
+ $this->assertEquals(['url.path.parent', 'user.permissions'], $breadcrumb->getCacheContexts());
$this->assertEquals([], $breadcrumb->getCacheTags());
$this->assertEquals(Cache::PERMANENT, $breadcrumb->getCacheMaxAge());
}
@@ -245,7 +245,7 @@ public function testBuildWithThreePathElements() {
new Link('Example', new Url('example')),
new Link('Bar', new Url('example_bar')),
], $breadcrumb->getLinks());
- $this->assertEquals(['bar', 'url.path', 'user.permissions'], $breadcrumb->getCacheContexts());
+ $this->assertEquals(['bar', 'url.path.parent', 'user.permissions'], $breadcrumb->getCacheContexts());
$this->assertEquals(['example'], $breadcrumb->getCacheTags());
$this->assertEquals(Cache::PERMANENT, $breadcrumb->getCacheMaxAge());
}
@@ -272,7 +272,7 @@ public function testBuildWithException($exception_class, $exception_argument) {
// No path matched, though at least the frontpage is displayed.
$this->assertEquals([0 => new Link('Home', new Url('<front>'))], $breadcrumb->getLinks());
- $this->assertEquals(['url.path'], $breadcrumb->getCacheContexts());
+ $this->assertEquals(['url.path.parent'], $breadcrumb->getCacheContexts());
$this->assertEquals([], $breadcrumb->getCacheTags());
$this->assertEquals(Cache::PERMANENT, $breadcrumb->getCacheMaxAge());
}
@@ -316,7 +316,7 @@ public function testBuildWithNonProcessedPath() {
// No path matched, though at least the frontpage is displayed.
$this->assertEquals([0 => new Link('Home', new Url('<front>'))], $breadcrumb->getLinks());
- $this->assertEquals(['url.path'], $breadcrumb->getCacheContexts());
+ $this->assertEquals(['url.path.parent'], $breadcrumb->getCacheContexts());
$this->assertEquals([], $breadcrumb->getCacheTags());
$this->assertEquals(Cache::PERMANENT, $breadcrumb->getCacheMaxAge());
}
@@ -364,7 +364,7 @@ public function testBuildWithUserPath() {
$breadcrumb = $this->builder->build($this->getMock('Drupal\Core\Routing\RouteMatchInterface'));
$this->assertEquals([0 => new Link('Home', new Url('<front>')), 1 => new Link('Admin', new Url('user_page'))], $breadcrumb->getLinks());
- $this->assertEquals(['url.path', 'user.permissions'], $breadcrumb->getCacheContexts());
+ $this->assertEquals(['url.path.parent', 'user.permissions'], $breadcrumb->getCacheContexts());
$this->assertEquals([], $breadcrumb->getCacheTags());
$this->assertEquals(Cache::PERMANENT, $breadcrumb->getCacheMaxAge());
}
diff --git a/core/tests/Drupal/FunctionalTests/Breadcrumb/Breadcrumb404Test.php b/core/tests/Drupal/FunctionalTests/Breadcrumb/Breadcrumb404Test.php
new file mode 100644
index 000000000000..7dcafbff85c7
--- /dev/null
+++ b/core/tests/Drupal/FunctionalTests/Breadcrumb/Breadcrumb404Test.php
@@ -0,0 +1,57 @@
+<?php
+
+namespace Drupal\FunctionalTests\Breadcrumb;
+
+use Drupal\simpletest\BlockCreationTrait;
+use Drupal\Tests\BrowserTestBase;
+
+/**
+ * Tests the breadcrumb of 404 pages.
+ *
+ * @group breadcrumb
+ */
+class Breadcrumb404Test extends BrowserTestBase {
+
+ use BlockCreationTrait;
+
+ /**
+ * {@inheritdoc}
+ */
+ public static $modules = ['system', 'block'];
+
+ /**
+ * Tests that different 404s don't create unnecessary cache entries.
+ */
+ public function testBreadcrumbOn404Pages() {
+ $this->placeBlock('system_breadcrumb_block', ['id' => 'breadcrumb']);
+
+ // Prime the cache first.
+ $this->drupalGet('/not-found-1');
+ $base_count = count($this->getBreadcrumbCacheEntries());
+
+ $this->drupalGet('/not-found-2');
+ $next_count = count($this->getBreadcrumbCacheEntries());
+ $this->assertEquals($base_count, $next_count);
+
+ $this->drupalGet('/not-found-3');
+ $next_count = count($this->getBreadcrumbCacheEntries());
+ $this->assertEquals($base_count, $next_count);
+ }
+
+ /**
+ * Gets the breadcrumb cache entries.
+ *
+ * @return array
+ * The breadcrumb cache entries.
+ */
+ protected function getBreadcrumbCacheEntries() {
+ $database = \Drupal::database();
+ $cache_entries = $database->select('cache_render')
+ ->fields('cache_render')
+ ->condition('cid', $database->escapeLike('entity_view:block:breadcrumb') . '%', 'LIKE')
+ ->execute()
+ ->fetchAllAssoc('cid');
+ return $cache_entries;
+ }
+
+}
diff --git a/core/tests/Drupal/Tests/Core/Cache/Context/PathParentCacheContextTest.php b/core/tests/Drupal/Tests/Core/Cache/Context/PathParentCacheContextTest.php
new file mode 100644
index 000000000000..2c447351597c
--- /dev/null
+++ b/core/tests/Drupal/Tests/Core/Cache/Context/PathParentCacheContextTest.php
@@ -0,0 +1,43 @@
+<?php
+
+namespace Drupal\Tests\Core\Cache\Context;
+
+use Drupal\Core\Cache\Context\PathParentCacheContext;
+use Drupal\Tests\UnitTestCase;
+use Symfony\Component\HttpFoundation\Request;
+use Symfony\Component\HttpFoundation\RequestStack;
+
+/**
+ * @coversDefaultClass \Drupal\Core\Cache\Context\PathParentCacheContext
+ * @group Cache
+ */
+class PathParentCacheContextTest extends UnitTestCase {
+
+ /**
+ * @covers ::getContext
+ *
+ * @dataProvider providerTestGetContext
+ */
+ public function testgetContext($original_path, $context) {
+ $request_stack = new RequestStack();
+ $request = Request::create($original_path);
+ $request_stack->push($request);
+ $cache_context = new PathParentCacheContext($request_stack);
+ $this->assertSame($cache_context->getContext(), $context);
+ }
+
+ /**
+ * Provides a list of paths and expected cache contexts.
+ */
+ public function providerTestGetContext() {
+ return [
+ ['/some/path', 'some'],
+ ['/some/other-path', 'some'],
+ ['/some/other/path', 'some/other'],
+ ['/some/other/path?q=foo&b=bar', 'some/other'],
+ ['/some', ''],
+ ['/', ''],
+ ];
+ }
+
+}
--
GitLab