From 09ff81ae521ffe69c816015480550117369d7358 Mon Sep 17 00:00:00 2001 From: Nathaniel Catchpole Date: Wed, 29 Jan 2014 10:47:12 +0000 Subject: [PATCH] Issue #2167039 by likin, dawehner, Wim Leers, tstoeckler, vijaycs85: Regression: page cache tags broken, also: some routes incorrectly use _controller -> No page object in template variables -> Fatal in seven_preprocess_page(). --- core/includes/common.inc | 33 +++-------- .../Drupal/Core/Cache/CacheableInterface.php | 54 +++++++++++++++++ .../Core/Controller/HtmlControllerBase.php | 22 ++++++- .../EventSubscriber/HtmlViewSubscriber.php | 12 ++++ .../Core/Page/DefaultHtmlPageRenderer.php | 16 +++-- core/lib/Drupal/Core/Page/HtmlFragment.php | 58 ++++++++++++++++++- core/lib/Drupal/Core/Page/HtmlPage.php | 6 +- .../modules/aggregator/aggregator.routing.yml | 2 +- .../config_translation/ConfigNamesMapper.php | 2 +- .../Tests/ConfigNamesMapperTest.php | 2 +- .../Drupal/node/Tests/NodePageCacheTest.php | 9 ++- core/modules/simpletest/simpletest.module | 1 + .../system/Controller/BatchController.php | 2 +- .../system/Tests/Bootstrap/PageCacheTest.php | 7 +++ core/modules/system/system.module | 1 - .../system_test/system_test.routing.yml | 2 +- 16 files changed, 188 insertions(+), 41 deletions(-) create mode 100644 core/lib/Drupal/Core/Cache/CacheableInterface.php diff --git a/core/includes/common.inc b/core/includes/common.inc index 580ef214ad..b97cc5e509 100644 --- a/core/includes/common.inc +++ b/core/includes/common.inc @@ -3147,7 +3147,7 @@ function drupal_page_set_cache(Response $response, Request $request) { // because by the time it is read, the configuration might change. 'page_compressed' => $page_compressed, ), - 'tags' => array('content' => TRUE) + drupal_cache_tags_page_get(), + 'tags' => array('content' => TRUE) + drupal_cache_tags_page_get($response), 'expire' => Cache::PERMANENT, 'created' => REQUEST_TIME, ); @@ -4358,36 +4358,21 @@ function drupal_render_collect_cache_tags($element, $tags = array()) { return $tags; } -/** - * A #post_render callback at the top level of the $page array. Collects the - * tags for use in page cache. - * - * @param string $children - * An HTML string of rendered output. - * @param array $elements - * A render array. - * - * @return string - * The same $children that was passed in - no modifications. - */ -function drupal_post_render_cache_tags_page_set($children, array $elements) { - if (drupal_page_is_cacheable()) { - $tags = &drupal_static('system_cache_tags_page', array()); - $tags = drupal_render_collect_cache_tags($elements); - } - return $children; -} - /** * Return the cache tags that were stored during drupal_render_page(). * + * @param \Symfony\Component\HttpFoundation\Response $response + * The response object. * @return array * An array of cache tags. * - * @see drupal_post_render_cache_tags_page_set() + * @see \Drupal\Core\EventSubscriber\HtmlViewSubscriber::onHtmlPage() */ -function drupal_cache_tags_page_get() { - return drupal_static('system_cache_tags_page', array()); +function drupal_cache_tags_page_get(Response $response) { + if (($tags = $response->headers->get('cache_tags')) && $tags = unserialize($tags)) { + return $tags; + } + return array(); } /** diff --git a/core/lib/Drupal/Core/Cache/CacheableInterface.php b/core/lib/Drupal/Core/Cache/CacheableInterface.php new file mode 100644 index 0000000000..da4f04c2ac --- /dev/null +++ b/core/lib/Drupal/Core/Cache/CacheableInterface.php @@ -0,0 +1,54 @@ +drupalRenderCollectCacheTags($page_content); + $cache = !empty($cache_tags) ? array('tags' => $cache_tags) : array(); + $fragment = new HtmlFragment($this->drupalRender($page_content), $cache); // A title defined in the return always wins. if (isset($page_content['#title'])) { @@ -84,4 +86,22 @@ protected function createHtmlFragment($page_content, Request $request) { return $fragment; } + /** + * Wraps drupal_render(). + * + * @todo: Remove as part of https://drupal.org/node/2182149 + */ + protected function drupalRender(&$elements, $is_recursive_call = FALSE) { + return drupal_render($elements, $is_recursive_call); + } + + /** + * Wraps drupal_render_collect_cache_tags() + * + * @todo: Remove as part of https://drupal.org/node/2182149 + */ + protected function drupalRenderCollectCacheTags($element, $tags = array()) { + return drupal_render_collect_cache_tags($element, $tags); + } + } diff --git a/core/lib/Drupal/Core/EventSubscriber/HtmlViewSubscriber.php b/core/lib/Drupal/Core/EventSubscriber/HtmlViewSubscriber.php index f66b6040f0..ae7c9a1b74 100644 --- a/core/lib/Drupal/Core/EventSubscriber/HtmlViewSubscriber.php +++ b/core/lib/Drupal/Core/EventSubscriber/HtmlViewSubscriber.php @@ -65,6 +65,18 @@ public function onHtmlPage(GetResponseForControllerResultEvent $event) { // to return an object implementing __toString(), but that is not // recommended. $response = new Response((string) $this->renderer->renderPage($page), $page->getStatusCode()); + if ($tags = $page->getCacheTags()) { + $response->headers->set('cache_tags', serialize($tags)); + } + if ($keys = $page->getCacheKeys()) { + $response->headers->set('cache_keys', serialize($keys)); + } + if ($bin = $page->getCacheBin()) { + $response->headers->set('cache_bin', $bin); + } + if ($max_age = $page->getCacheMaxAge()) { + $response->headers->set('cache_max_age', $max_age); + } $event->setResponse($response); } } diff --git a/core/lib/Drupal/Core/Page/DefaultHtmlPageRenderer.php b/core/lib/Drupal/Core/Page/DefaultHtmlPageRenderer.php index ad29a5b084..0d3b6d50d6 100644 --- a/core/lib/Drupal/Core/Page/DefaultHtmlPageRenderer.php +++ b/core/lib/Drupal/Core/Page/DefaultHtmlPageRenderer.php @@ -36,21 +36,27 @@ public function __construct(LanguageManager $language_manager) { * {@inheritdoc} */ public function render(HtmlFragment $fragment, $status_code = 200) { - $page = new HtmlPage('', $fragment->getTitle()); - + // Converts the given HTML fragment which represents the main content region + // of the page into a render array. $page_content['main'] = array( '#markup' => $fragment->getContent(), + '#cache' => array('tags' => $fragment->getCacheTags()), ); - $page_content['#title'] = $page->getTitle(); + $page_content['#title'] = $fragment->getTitle(); + // Build the full page array by calling drupal_prepare_page(), which invokes + // hook_page_build(). This adds the other regions to the page. $page_array = drupal_prepare_page($page_content); - $page = $this->preparePage($page, $page_array); + // Collect cache tags for all the content in all the regions on the page. + $tags = drupal_render_collect_cache_tags($page_array); + // Build the HtmlPage object. + $page = new HtmlPage('', array('tags' => $tags), $fragment->getTitle()); + $page = $this->preparePage($page, $page_array); $page->setBodyTop(drupal_render($page_array['page_top'])); $page->setBodyBottom(drupal_render($page_array['page_bottom'])); $page->setContent(drupal_render($page_array)); - $page->setStatusCode($status_code); return $page; diff --git a/core/lib/Drupal/Core/Page/HtmlFragment.php b/core/lib/Drupal/Core/Page/HtmlFragment.php index 3a50d5e970..cc5cac5a6c 100644 --- a/core/lib/Drupal/Core/Page/HtmlFragment.php +++ b/core/lib/Drupal/Core/Page/HtmlFragment.php @@ -9,6 +9,7 @@ use Drupal\Component\Utility\String; use Drupal\Component\Utility\Xss; +use Drupal\Core\Cache\CacheableInterface; use Drupal\Core\Utility\Title; /** @@ -18,7 +19,7 @@ * https://drupal.org/node/1871596#comment-7134686 * @todo Add method replacements for *all* data sourced by html.tpl.php. */ -class HtmlFragment { +class HtmlFragment implements CacheableInterface { /** * HTML content string. @@ -34,14 +35,30 @@ class HtmlFragment { */ protected $title = ''; + /** + * The cache metadata of this HtmlFragment. + * + * @var array + */ + protected $cache = array(); + /** * Constructs a new HtmlFragment. * * @param string $content * The content for this fragment. + * @param array $cache_info + * The cache information. */ - public function __construct($content = '') { + public function __construct($content = '', array $cache_info = array()) { $this->content = $content; + $this->cache = $cache_info + array( + 'keys' => array(), + 'tags' => array(), + 'bin' => NULL, + 'max_age' => 0, + 'is_cacheable' => TRUE, + ); } /** @@ -123,4 +140,41 @@ public function getTitle() { return $this->title; } + /** + * {@inheritdoc} + * + * @TODO Use a trait once we require php 5.4 for all the cache methods. + */ + public function getCacheKeys() { + return $this->cache['keys']; + } + + /** + * {@inheritdoc} + */ + public function getCacheTags() { + return $this->cache['tags']; + } + + /** + * {@inheritdoc} + */ + public function getCacheBin() { + return $this->cache['bin']; + } + + /** + * {@inheritdoc} + */ + public function getCacheMaxAge() { + return $this->cache['max_age']; + } + + /** + * {@inheritdoc} + */ + public function isCacheable() { + return $this->cache['is_cacheable']; + } + } diff --git a/core/lib/Drupal/Core/Page/HtmlPage.php b/core/lib/Drupal/Core/Page/HtmlPage.php index 6996d7c1e1..c2f09edc5a 100644 --- a/core/lib/Drupal/Core/Page/HtmlPage.php +++ b/core/lib/Drupal/Core/Page/HtmlPage.php @@ -54,11 +54,13 @@ class HtmlPage extends HtmlFragment { * * @param string $content * (optional) The body content of the page. + * @param array $cache_info + * The cache information. * @param string $title * (optional) The title of the page. */ - public function __construct($content = '', $title = '') { - parent::__construct($content); + public function __construct($content = '', array $cache_info = array(), $title = '') { + parent::__construct($content, $cache_info); $this->title = $title; diff --git a/core/modules/aggregator/aggregator.routing.yml b/core/modules/aggregator/aggregator.routing.yml index 33ac34dd03..1f3464deeb 100644 --- a/core/modules/aggregator/aggregator.routing.yml +++ b/core/modules/aggregator/aggregator.routing.yml @@ -57,7 +57,7 @@ aggregator.feed_edit: aggregator.feed_refresh: path: '/admin/config/services/aggregator/update/{aggregator_feed}' defaults: - _controller: '\Drupal\aggregator\Controller\AggregatorController::feedRefresh' + _content: '\Drupal\aggregator\Controller\AggregatorController::feedRefresh' _title: 'Update items' requirements: _permission: 'administer news feeds' diff --git a/core/modules/config_translation/lib/Drupal/config_translation/ConfigNamesMapper.php b/core/modules/config_translation/lib/Drupal/config_translation/ConfigNamesMapper.php index 2df738732e..7ba5b2a216 100644 --- a/core/modules/config_translation/lib/Drupal/config_translation/ConfigNamesMapper.php +++ b/core/modules/config_translation/lib/Drupal/config_translation/ConfigNamesMapper.php @@ -176,7 +176,7 @@ public function getOverviewRoute() { return new Route( $this->getBaseRoute()->getPath() . '/translate', array( - '_controller' => '\Drupal\config_translation\Controller\ConfigTranslationController::itemPage', + '_content' => '\Drupal\config_translation\Controller\ConfigTranslationController::itemPage', 'plugin_id' => $this->getPluginId(), ), array('_config_translation_overview_access' => 'TRUE') diff --git a/core/modules/config_translation/tests/Drupal/config_translation/Tests/ConfigNamesMapperTest.php b/core/modules/config_translation/tests/Drupal/config_translation/Tests/ConfigNamesMapperTest.php index 3df6ac0245..ac8a13a30b 100644 --- a/core/modules/config_translation/tests/Drupal/config_translation/Tests/ConfigNamesMapperTest.php +++ b/core/modules/config_translation/tests/Drupal/config_translation/Tests/ConfigNamesMapperTest.php @@ -176,7 +176,7 @@ public function testGetOverviewRouteParameters() { public function testGetOverviewRoute() { $expected = new Route('/admin/config/system/site-information/translate', array( - '_controller' => '\Drupal\config_translation\Controller\ConfigTranslationController::itemPage', + '_content' => '\Drupal\config_translation\Controller\ConfigTranslationController::itemPage', 'plugin_id' => 'system.site_information_settings', ), array( diff --git a/core/modules/node/lib/Drupal/node/Tests/NodePageCacheTest.php b/core/modules/node/lib/Drupal/node/Tests/NodePageCacheTest.php index 4a2385cd01..49bc190fae 100644 --- a/core/modules/node/lib/Drupal/node/Tests/NodePageCacheTest.php +++ b/core/modules/node/lib/Drupal/node/Tests/NodePageCacheTest.php @@ -48,11 +48,18 @@ function setUp() { * Tests deleting nodes clears page cache. */ public function testNodeDelete() { - $node_path = 'node/' . $this->drupalCreateNode()->id(); + $author = $this->drupalCreateUser(); + $node_path = 'node/' . $this->drupalCreateNode(array('uid' => $author->id()))->id(); // Populate page cache. $this->drupalGet($node_path); + // Verify the presence of the correct cache tags. + $cid_parts = array(url($node_path, array('absolute' => TRUE)), 'html'); + $cid = sha1(implode(':', $cid_parts)); + $cache_entry = \Drupal::cache('page')->get($cid); + $this->assertIdentical($cache_entry->tags, array('content:1', 'user:' . $author->id(), 'filter_format:plain_text')); + // Login and delete the node. $this->drupalLogin($this->adminUser); $this->drupalGet($node_path . '/delete'); diff --git a/core/modules/simpletest/simpletest.module b/core/modules/simpletest/simpletest.module index 6efecef525..c6b53bf0c4 100644 --- a/core/modules/simpletest/simpletest.module +++ b/core/modules/simpletest/simpletest.module @@ -1,6 +1,7 @@ titleResolver->getTitle($request, $request->attributes->get(RouteObjectInterface::ROUTE_OBJECT)); } - $page = new HtmlPage('', $output['#title']); + $page = new HtmlPage('', isset($output['#cache']) ? $output['#cache'] : array(), $output['#title']); $page_array = drupal_prepare_page($output); diff --git a/core/modules/system/lib/Drupal/system/Tests/Bootstrap/PageCacheTest.php b/core/modules/system/lib/Drupal/system/Tests/Bootstrap/PageCacheTest.php index aec3355fca..3ca1f19f34 100644 --- a/core/modules/system/lib/Drupal/system/Tests/Bootstrap/PageCacheTest.php +++ b/core/modules/system/lib/Drupal/system/Tests/Bootstrap/PageCacheTest.php @@ -58,8 +58,15 @@ function testPageCacheTags() { $tags = array('system_test_cache_tags_page' => TRUE); $this->drupalGet($path); $this->assertEqual($this->drupalGetHeader('X-Drupal-Cache'), 'MISS'); + + // Verify a cache hit, but also the presence of the correct cache tags. $this->drupalGet($path); $this->assertEqual($this->drupalGetHeader('X-Drupal-Cache'), 'HIT'); + $cid_parts = array(url($path, array('absolute' => TRUE)), 'html'); + $cid = sha1(implode(':', $cid_parts)); + $cache_entry = \Drupal::cache('page')->get($cid); + $this->assertIdentical($cache_entry->tags, array('content:1', 'system_test_cache_tags_page:1')); + Cache::invalidateTags($tags); $this->drupalGet($path); $this->assertEqual($this->drupalGetHeader('X-Drupal-Cache'), 'MISS'); diff --git a/core/modules/system/system.module b/core/modules/system/system.module index 4f92d7352c..cc647a23c9 100644 --- a/core/modules/system/system.module +++ b/core/modules/system/system.module @@ -269,7 +269,6 @@ function system_element_info() { '#theme_wrappers' => array('form'), ); $types['page'] = array( - '#post_render' => array('drupal_post_render_cache_tags_page_set'), '#show_messages' => TRUE, '#theme' => 'page', ); diff --git a/core/modules/system/tests/modules/system_test/system_test.routing.yml b/core/modules/system/tests/modules/system_test/system_test.routing.yml index ecd03ed59c..0a6ee6f555 100644 --- a/core/modules/system/tests/modules/system_test/system_test.routing.yml +++ b/core/modules/system/tests/modules/system_test/system_test.routing.yml @@ -48,7 +48,7 @@ system_test.lock_exit: system_test.cache_tags_page: path: '/system-test/cache_tags_page' defaults: - _controller: '\Drupal\system_test\Controller\SystemTestController::system_test_cache_tags_page' + _content: '\Drupal\system_test\Controller\SystemTestController::system_test_cache_tags_page' requirements: _access: 'TRUE' -- GitLab