From fb6c562c9eef10adf986959dbeca1e7bc6d490a9 Mon Sep 17 00:00:00 2001 From: Alex Pott Date: Sat, 20 Sep 2014 11:14:29 +0100 Subject: [PATCH] Issue #2263981 by znerol, beejeebus: Introduce a robust and extensible page cache-policy framework. --- core/core.services.yml | 14 +- core/includes/bootstrap.inc | 26 +-- core/includes/utility.inc | 4 +- core/lib/Drupal/Core/DrupalKernel.php | 6 +- .../FinishResponseSubscriber.php | 31 +++- .../Core/PageCache/ChainRequestPolicy.php | 65 +++++++ .../PageCache/ChainRequestPolicyInterface.php | 25 +++ .../Core/PageCache/ChainResponsePolicy.php | 56 ++++++ .../ChainResponsePolicyInterface.php | 24 +++ .../Core/PageCache/DefaultRequestPolicy.php | 27 +++ .../CommandLineOrUnsafeMethod.php | 38 ++++ .../PageCache/RequestPolicy/NoSessionOpen.php | 49 ++++++ .../Core/PageCache/RequestPolicyInterface.php | 54 ++++++ .../PageCache/ResponsePolicy/KillSwitch.php | 42 +++++ .../PageCache/ResponsePolicyInterface.php | 42 +++++ .../Drupal/Core/Session/SessionManager.php | 3 - core/modules/image/image.services.yml | 5 + .../ImageStyleDownloadController.php | 1 - .../DenyPrivateImageStyleDownload.php | 49 ++++++ .../DenyPrivateImageStyleDownloadTest.php | 90 ++++++++++ core/modules/node/node.services.yml | 5 + .../src/Controller/NodePreviewController.php | 3 - .../node/src/PageCache/DenyNodePreview.php | 49 ++++++ .../Unit/PageCache/DenyNodePreviewTest.php | 90 ++++++++++ .../src/Controller/ToolbarController.php | 14 +- .../src/PageCache/AllowToolbarPath.php | 32 ++++ .../Unit/PageCache/AllowToolbarPathTest.php | 65 +++++++ core/modules/toolbar/toolbar.module | 37 ---- core/modules/toolbar/toolbar.services.yml | 4 + .../Core/PageCache/ChainRequestPolicyTest.php | 165 ++++++++++++++++++ .../PageCache/ChainResponsePolicyTest.php | 140 +++++++++++++++ .../CommandLineOrUnsafeMethodTest.php | 83 +++++++++ .../Core/PageCache/NoSessionOpenTest.php | 54 ++++++ 33 files changed, 1314 insertions(+), 78 deletions(-) create mode 100644 core/lib/Drupal/Core/PageCache/ChainRequestPolicy.php create mode 100644 core/lib/Drupal/Core/PageCache/ChainRequestPolicyInterface.php create mode 100644 core/lib/Drupal/Core/PageCache/ChainResponsePolicy.php create mode 100644 core/lib/Drupal/Core/PageCache/ChainResponsePolicyInterface.php create mode 100644 core/lib/Drupal/Core/PageCache/DefaultRequestPolicy.php create mode 100644 core/lib/Drupal/Core/PageCache/RequestPolicy/CommandLineOrUnsafeMethod.php create mode 100644 core/lib/Drupal/Core/PageCache/RequestPolicy/NoSessionOpen.php create mode 100644 core/lib/Drupal/Core/PageCache/RequestPolicyInterface.php create mode 100644 core/lib/Drupal/Core/PageCache/ResponsePolicy/KillSwitch.php create mode 100644 core/lib/Drupal/Core/PageCache/ResponsePolicyInterface.php create mode 100644 core/modules/image/src/PageCache/DenyPrivateImageStyleDownload.php create mode 100644 core/modules/image/tests/src/Unit/PageCache/DenyPrivateImageStyleDownloadTest.php create mode 100644 core/modules/node/src/PageCache/DenyNodePreview.php create mode 100644 core/modules/node/tests/src/Unit/PageCache/DenyNodePreviewTest.php create mode 100644 core/modules/toolbar/src/PageCache/AllowToolbarPath.php create mode 100644 core/modules/toolbar/tests/src/Unit/PageCache/AllowToolbarPathTest.php create mode 100644 core/tests/Drupal/Tests/Core/PageCache/ChainRequestPolicyTest.php create mode 100644 core/tests/Drupal/Tests/Core/PageCache/ChainResponsePolicyTest.php create mode 100644 core/tests/Drupal/Tests/Core/PageCache/CommandLineOrUnsafeMethodTest.php create mode 100644 core/tests/Drupal/Tests/Core/PageCache/NoSessionOpenTest.php diff --git a/core/core.services.yml b/core/core.services.yml index 0f4d5863753..173f608c2be 100644 --- a/core/core.services.yml +++ b/core/core.services.yml @@ -100,6 +100,18 @@ services: factory_method: get factory_service: cache_factory arguments: [discovery] + page_cache_request_policy: + class: Drupal\Core\PageCache\DefaultRequestPolicy + tags: + - { name: service_collector, tag: page_cache_request_policy, call: addPolicy} + page_cache_response_policy: + class: Drupal\Core\PageCache\ChainResponsePolicy + tags: + - { name: service_collector, tag: page_cache_response_policy, call: addPolicy} + page_cache_kill_switch: + class: Drupal\Core\PageCache\ResponsePolicy\KillSwitch + tags: + - { name: page_cache_response_policy } config.manager: class: Drupal\Core\Config\ConfigManager arguments: ['@entity.manager', '@config.factory', '@config.typed', '@string_translation', '@config.storage', '@event_dispatcher'] @@ -729,7 +741,7 @@ services: class: Drupal\Core\EventSubscriber\FinishResponseSubscriber tags: - { name: event_subscriber } - arguments: ['@language_manager', '@config.factory'] + arguments: ['@language_manager', '@config.factory', '@page_cache_request_policy', '@page_cache_response_policy'] redirect_response_subscriber: class: Drupal\Core\EventSubscriber\RedirectResponseSubscriber arguments: ['@url_generator'] diff --git a/core/includes/bootstrap.inc b/core/includes/bootstrap.inc index 1e1cc30de21..553811fc437 100644 --- a/core/includes/bootstrap.inc +++ b/core/includes/bootstrap.inc @@ -389,30 +389,6 @@ function drupal_page_get_cache(Request $request) { } } -/** - * Determines the cacheability of the current page. - * - * Note: we do not serve cached pages to authenticated users, or to anonymous - * users when $_SESSION is non-empty. $_SESSION may contain status messages - * from a form submission, the contents of a shopping cart, or other user- - * specific content that should not be cached and displayed to other users. - * - * @param $allow_caching - * Set to FALSE if you want to prevent this page to get cached. - * - * @return - * TRUE if the current page can be cached, FALSE otherwise. - */ -function drupal_page_is_cacheable($allow_caching = NULL) { - $allow_caching_static = &drupal_static(__FUNCTION__, TRUE); - if (isset($allow_caching)) { - $allow_caching_static = $allow_caching; - } - - return $allow_caching_static && ($_SERVER['REQUEST_METHOD'] == 'GET' || $_SERVER['REQUEST_METHOD'] == 'HEAD') - && PHP_SAPI !== 'cli'; -} - /** * Sets an HTTP response header for the current page. * @@ -931,7 +907,7 @@ function drupal_set_message($message = NULL, $type = 'status', $repeat = FALSE) } // Mark this page as being uncacheable. - drupal_page_is_cacheable(FALSE); + \Drupal::service('page_cache_kill_switch')->trigger(); } // Messages not set when DB connection fails. diff --git a/core/includes/utility.inc b/core/includes/utility.inc index 3192debd1c2..ec6a20cb2cc 100644 --- a/core/includes/utility.inc +++ b/core/includes/utility.inc @@ -62,8 +62,8 @@ function drupal_rebuild(ClassLoader $classloader, Request $request) { $bin->deleteAll(); } - // Disable the page cache. - drupal_page_is_cacheable(FALSE); + // Disable recording of cached pages. + \Drupal::service('page_cache_kill_switch')->trigger(); drupal_flush_all_caches(); diff --git a/core/lib/Drupal/Core/DrupalKernel.php b/core/lib/Drupal/Core/DrupalKernel.php index 6f7b011a4d2..065afae8889 100644 --- a/core/lib/Drupal/Core/DrupalKernel.php +++ b/core/lib/Drupal/Core/DrupalKernel.php @@ -19,6 +19,7 @@ use Drupal\Core\DependencyInjection\ServiceProviderInterface; use Drupal\Core\DependencyInjection\YamlFileLoader; use Drupal\Core\Extension\ExtensionDiscovery; use Drupal\Core\Language\Language; +use Drupal\Core\PageCache\RequestPolicyInterface; use Drupal\Core\PhpStorage\PhpStorageFactory; use Drupal\Core\Site\Settings; use Symfony\Component\DependencyInjection\ContainerInterface; @@ -466,9 +467,8 @@ class DrupalKernel implements DrupalKernelInterface, TerminableInterface { $cache_enabled = $config->get('cache.page.use_internal'); } - // If there is no session cookie and cache is enabled (or forced), try to - // serve a cached page. - if (!$request->cookies->has(session_name()) && $cache_enabled && drupal_page_is_cacheable()) { + $request_policy = \Drupal::service('page_cache_request_policy'); + if ($cache_enabled && $request_policy->check($request) === RequestPolicyInterface::ALLOW) { // Get the page from the cache. $response = drupal_page_get_cache($request); // If there is a cached page, display it. diff --git a/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php b/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php index f882c2929ea..c5a7be1afe1 100644 --- a/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php +++ b/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php @@ -11,6 +11,8 @@ use Drupal\Component\Datetime\DateTimePlus; use Drupal\Core\Config\Config; use Drupal\Core\Config\ConfigFactoryInterface; use Drupal\Core\Language\LanguageManagerInterface; +use Drupal\Core\PageCache\RequestPolicyInterface; +use Drupal\Core\PageCache\ResponsePolicyInterface; use Drupal\Core\Site\Settings; use Symfony\Component\HttpFoundation\BinaryFileResponse; use Symfony\Component\HttpFoundation\Request; @@ -40,6 +42,20 @@ class FinishResponseSubscriber implements EventSubscriberInterface { */ protected $config; + /** + * A policy rule determining the cacheability of a request. + * + * @var \Drupal\Core\PageCache\RequestPolicyInterface + */ + protected $requestPolicy; + + /** + * A policy rule determining the cacheability of the response. + * + * @var \Drupal\Core\PageCache\ResponsePolicyInterface + */ + protected $responsePolicy; + /** * Constructs a FinishResponseSubscriber object. * @@ -47,10 +63,16 @@ class FinishResponseSubscriber implements EventSubscriberInterface { * The language manager object for retrieving the correct language code. * @param \Drupal\Core\Config\ConfigFactoryInterface $config_factory * A config factory for retrieving required config objects. + * @param \Drupal\Core\PageCache\RequestPolicyInterface $request_policy + * A policy rule determining the cacheability of a request. + * @param \Drupal\Core\PageCache\ResponsePolicyInterface $response_policy + * A policy rule determining the cacheability of a response. */ - public function __construct(LanguageManagerInterface $language_manager, ConfigFactoryInterface $config_factory) { + public function __construct(LanguageManagerInterface $language_manager, ConfigFactoryInterface $config_factory, RequestPolicyInterface $request_policy, ResponsePolicyInterface $response_policy) { $this->languageManager = $language_manager; $this->config = $config_factory->get('system.performance'); + $this->requestPolicy = $request_policy; + $this->responsePolicy = $response_policy; } /** @@ -83,16 +105,21 @@ class FinishResponseSubscriber implements EventSubscriberInterface { $response->headers->set($name, $value, FALSE); } - $is_cacheable = drupal_page_is_cacheable(); + $is_cacheable = ($this->requestPolicy->check($request) === RequestPolicyInterface::ALLOW) && ($this->responsePolicy->check($response, $request) !== ResponsePolicyInterface::DENY); // Add headers necessary to specify whether the response should be cached by // proxies and/or the browser. if ($is_cacheable && $this->config->get('cache.page.max_age') > 0) { if (!$this->isCacheControlCustomized($response)) { + // Only add the default Cache-Control header if the controller did not + // specify one on the response. $this->setResponseCacheable($response, $request); } } else { + // If either the policy forbids caching or the sites configuration does + // not allow to add a max-age directive, then enforce a Cache-Control + // header declaring the response as not cacheable. $this->setResponseNotCacheable($response, $request); } diff --git a/core/lib/Drupal/Core/PageCache/ChainRequestPolicy.php b/core/lib/Drupal/Core/PageCache/ChainRequestPolicy.php new file mode 100644 index 00000000000..23dc1d6cca9 --- /dev/null +++ b/core/lib/Drupal/Core/PageCache/ChainRequestPolicy.php @@ -0,0 +1,65 @@ + + *
  • Returns static::DENY if any of the rules evaluated to static::DENY
    • + *
  • Returns static::ALLOW if at least one of the rules evaluated to + * static::ALLOW and none to static::DENY
    • + *
  • Otherwise returns NULL
    • + * + */ +class ChainRequestPolicy implements ChainRequestPolicyInterface { + + /** + * A list of policy rules to apply when this policy is evaluated. + * + * @var \Drupal\Core\PageCache\RequestPolicyInterface[] + */ + protected $rules = []; + + /** + * {@inheritdoc} + */ + public function check(Request $request) { + $final_result = NULL; + + foreach ($this->rules as $rule) { + $result = $rule->check($request); + if ($result === static::DENY) { + return $result; + } + elseif ($result === static::ALLOW) { + $final_result = $result; + } + elseif (isset($result)) { + throw new \UnexpectedValueException('Return value of RequestPolicyInterface::check() must be one of RequestPolicyInterface::ALLOW, RequestPolicyInterface::DENY or NULL'); + } + } + + return $final_result; + } + + /** + * {@inheritdoc} + */ + public function addPolicy(RequestPolicyInterface $policy) { + $this->rules[] = $policy; + return $this; + } + +} diff --git a/core/lib/Drupal/Core/PageCache/ChainRequestPolicyInterface.php b/core/lib/Drupal/Core/PageCache/ChainRequestPolicyInterface.php new file mode 100644 index 00000000000..e8548df7e59 --- /dev/null +++ b/core/lib/Drupal/Core/PageCache/ChainRequestPolicyInterface.php @@ -0,0 +1,25 @@ + + *
  • Returns static::DENY if any of the rules evaluated to static::DENY
    • + *
  • Otherwise returns NULL
    • + * + */ +class ChainResponsePolicy implements ChainResponsePolicyInterface { + + /** + * A list of policy rules to apply when this policy is checked. + * + * @var \Drupal\Core\PageCache\ResponsePolicyInterface[] + */ + protected $rules = []; + + /** + * {@inheritdoc} + */ + public function check(Response $response, Request $request) { + foreach ($this->rules as $rule) { + $result = $rule->check($response, $request); + if ($result === static::DENY) { + return $result; + } + elseif (isset($result)) { + throw new \UnexpectedValueException('Return value of ResponsePolicyInterface::check() must be one of ResponsePolicyInterface::DENY or NULL'); + } + } + } + + /** + * {@inheritdoc} + */ + public function addPolicy(ResponsePolicyInterface $policy) { + $this->rules[] = $policy; + return $this; + } + +} diff --git a/core/lib/Drupal/Core/PageCache/ChainResponsePolicyInterface.php b/core/lib/Drupal/Core/PageCache/ChainResponsePolicyInterface.php new file mode 100644 index 00000000000..5018f19abf4 --- /dev/null +++ b/core/lib/Drupal/Core/PageCache/ChainResponsePolicyInterface.php @@ -0,0 +1,24 @@ +addPolicy(new RequestPolicy\CommandLineOrUnsafeMethod()); + $this->addPolicy(new RequestPolicy\NoSessionOpen()); + } + +} diff --git a/core/lib/Drupal/Core/PageCache/RequestPolicy/CommandLineOrUnsafeMethod.php b/core/lib/Drupal/Core/PageCache/RequestPolicy/CommandLineOrUnsafeMethod.php new file mode 100644 index 00000000000..66a3bf692e3 --- /dev/null +++ b/core/lib/Drupal/Core/PageCache/RequestPolicy/CommandLineOrUnsafeMethod.php @@ -0,0 +1,38 @@ +isCli() || !$request->isMethodSafe()) { + return static::DENY; + } + } + + /** + * Indicates whether this is a CLI request. + */ + protected function isCli() { + return PHP_SAPI === 'cli'; + } + +} diff --git a/core/lib/Drupal/Core/PageCache/RequestPolicy/NoSessionOpen.php b/core/lib/Drupal/Core/PageCache/RequestPolicy/NoSessionOpen.php new file mode 100644 index 00000000000..c18cf566603 --- /dev/null +++ b/core/lib/Drupal/Core/PageCache/RequestPolicy/NoSessionOpen.php @@ -0,0 +1,49 @@ +sessionCookieName = $session_cookie_name ?: session_name(); + } + + /** + * {@inheritdoc} + */ + public function check(Request $request) { + if (!$request->cookies->has($this->sessionCookieName)) { + return static::ALLOW; + } + } + +} diff --git a/core/lib/Drupal/Core/PageCache/RequestPolicyInterface.php b/core/lib/Drupal/Core/PageCache/RequestPolicyInterface.php new file mode 100644 index 00000000000..3a4ef212795 --- /dev/null +++ b/core/lib/Drupal/Core/PageCache/RequestPolicyInterface.php @@ -0,0 +1,54 @@ +kill) { + return static::DENY; + } + } + + /** + * Deny any page caching on the current request. + */ + public function trigger() { + $this->kill = TRUE; + } + +} diff --git a/core/lib/Drupal/Core/PageCache/ResponsePolicyInterface.php b/core/lib/Drupal/Core/PageCache/ResponsePolicyInterface.php new file mode 100644 index 00000000000..596f43fd9aa --- /dev/null +++ b/core/lib/Drupal/Core/PageCache/ResponsePolicyInterface.php @@ -0,0 +1,42 @@ +startNow(); - if ($user->isAuthenticated() || !$this->isSessionObsolete()) { - drupal_page_is_cacheable(FALSE); - } } if (empty($result)) { diff --git a/core/modules/image/image.services.yml b/core/modules/image/image.services.yml index 1447e036f61..7f1f985f736 100644 --- a/core/modules/image/image.services.yml +++ b/core/modules/image/image.services.yml @@ -6,3 +6,8 @@ services: plugin.manager.image.effect: class: Drupal\image\ImageEffectManager parent: default_plugin_manager + image.page_cache_request_policy.deny_private_image_style_download: + class: Drupal\image\PageCache\DenyPrivateImageStyleDownload + arguments: ['@current_route_match'] + tags: + - { name: page_cache_response_policy } diff --git a/core/modules/image/src/Controller/ImageStyleDownloadController.php b/core/modules/image/src/Controller/ImageStyleDownloadController.php index 3e4bcf00d14..a18c6774f47 100644 --- a/core/modules/image/src/Controller/ImageStyleDownloadController.php +++ b/core/modules/image/src/Controller/ImageStyleDownloadController.php @@ -155,7 +155,6 @@ class ImageStyleDownloadController extends FileDownloadController { } if ($success) { - drupal_page_is_cacheable(FALSE); $image = $this->imageFactory->get($derivative_uri); $uri = $image->getSource(); $headers += array( diff --git a/core/modules/image/src/PageCache/DenyPrivateImageStyleDownload.php b/core/modules/image/src/PageCache/DenyPrivateImageStyleDownload.php new file mode 100644 index 00000000000..02cb27c88d5 --- /dev/null +++ b/core/modules/image/src/PageCache/DenyPrivateImageStyleDownload.php @@ -0,0 +1,49 @@ +routeMatch = $route_match; + } + + /** + * {@inheritdoc} + */ + public function check(Response $response, Request $request) { + if ($this->routeMatch->getRouteName() === 'image.style_private') { + return static::DENY; + } + } + +} diff --git a/core/modules/image/tests/src/Unit/PageCache/DenyPrivateImageStyleDownloadTest.php b/core/modules/image/tests/src/Unit/PageCache/DenyPrivateImageStyleDownloadTest.php new file mode 100644 index 00000000000..fab54d36844 --- /dev/null +++ b/core/modules/image/tests/src/Unit/PageCache/DenyPrivateImageStyleDownloadTest.php @@ -0,0 +1,90 @@ +routeMatch = $this->getMock('Drupal\Core\Routing\RouteMatchInterface'); + $this->policy = new DenyPrivateImageStyleDownload($this->routeMatch); + $this->response = new Response(); + $this->request = new Request(); + } + + /** + * Asserts that caching is denied on the private image style download route. + * + * @dataProvider providerPrivateImageStyleDownloadPolicy + * @covers ::check + */ + public function testPrivateImageStyleDownloadPolicy($expected_result, $route_name) { + $this->routeMatch->expects($this->once()) + ->method('getRouteName') + ->will($this->returnValue($route_name)); + + $actual_result = $this->policy->check($this->response, $this->request); + $this->assertSame($expected_result, $actual_result); + } + + /** + * Provides data and expected results for the test method. + * + * @return array + * Data and expected results. + */ + public function providerPrivateImageStyleDownloadPolicy() { + return [ + [ResponsePolicyInterface::DENY, 'image.style_private'], + [NULL, 'some.other.route'], + [NULL, NULL], + [NULL, FALSE], + [NULL, TRUE], + [NULL, new \StdClass()], + [NULL, [1, 2, 3]], + ]; + } + +} diff --git a/core/modules/node/node.services.yml b/core/modules/node/node.services.yml index 546f47f4358..68f0af1936b 100644 --- a/core/modules/node/node.services.yml +++ b/core/modules/node/node.services.yml @@ -34,3 +34,8 @@ services: arguments: ['@user.tempstore'] tags: - { name: paramconverter } + node.page_cache_request_policy.deny_node_preview: + class: Drupal\node\PageCache\DenyNodePreview + arguments: ['@current_route_match'] + tags: + - { name: page_cache_response_policy } diff --git a/core/modules/node/src/Controller/NodePreviewController.php b/core/modules/node/src/Controller/NodePreviewController.php index 9fb8d131140..35c65d9654f 100644 --- a/core/modules/node/src/Controller/NodePreviewController.php +++ b/core/modules/node/src/Controller/NodePreviewController.php @@ -20,9 +20,6 @@ class NodePreviewController extends EntityViewController { * {@inheritdoc} */ public function view(EntityInterface $node_preview, $view_mode_id = 'full', $langcode = NULL) { - // Do not cache this page. - drupal_page_is_cacheable(FALSE); - $node_preview->preview_view_mode = $view_mode_id; $build = array('nodes' => parent::view($node_preview, $view_mode_id)); diff --git a/core/modules/node/src/PageCache/DenyNodePreview.php b/core/modules/node/src/PageCache/DenyNodePreview.php new file mode 100644 index 00000000000..5e749a04967 --- /dev/null +++ b/core/modules/node/src/PageCache/DenyNodePreview.php @@ -0,0 +1,49 @@ +routeMatch = $route_match; + } + + /** + * {@inheritdoc} + */ + public function check(Response $response, Request $request) { + if ($this->routeMatch->getRouteName() === 'entity.node.preview') { + return static::DENY; + } + } + +} diff --git a/core/modules/node/tests/src/Unit/PageCache/DenyNodePreviewTest.php b/core/modules/node/tests/src/Unit/PageCache/DenyNodePreviewTest.php new file mode 100644 index 00000000000..ea90c53952a --- /dev/null +++ b/core/modules/node/tests/src/Unit/PageCache/DenyNodePreviewTest.php @@ -0,0 +1,90 @@ +routeMatch = $this->getMock('Drupal\Core\Routing\RouteMatchInterface'); + $this->policy = new DenyNodePreview($this->routeMatch); + $this->response = new Response(); + $this->request = new Request(); + } + + /** + * Asserts that caching is denied on the node preview route. + * + * @dataProvider providerPrivateImageStyleDownloadPolicy + * @covers ::check + */ + public function testPrivateImageStyleDownloadPolicy($expected_result, $route_name) { + $this->routeMatch->expects($this->once()) + ->method('getRouteName') + ->will($this->returnValue($route_name)); + + $actual_result = $this->policy->check($this->response, $this->request); + $this->assertSame($expected_result, $actual_result); + } + + /** + * Provides data and expected results for the test method. + * + * @return array + * Data and expected results. + */ + public function providerPrivateImageStyleDownloadPolicy() { + return [ + [ResponsePolicyInterface::DENY, 'entity.node.preview'], + [NULL, 'some.other.route'], + [NULL, NULL], + [NULL, FALSE], + [NULL, TRUE], + [NULL, new \StdClass()], + [NULL, [1, 2, 3]], + ]; + } + +} diff --git a/core/modules/toolbar/src/Controller/ToolbarController.php b/core/modules/toolbar/src/Controller/ToolbarController.php index 26639515a94..aba53579a38 100644 --- a/core/modules/toolbar/src/Controller/ToolbarController.php +++ b/core/modules/toolbar/src/Controller/ToolbarController.php @@ -22,10 +22,22 @@ class ToolbarController extends ControllerBase { * @return \Symfony\Component\HttpFoundation\JsonResponse */ public function subtreesJsonp() { - _toolbar_initialize_page_cache(); $subtrees = toolbar_get_rendered_subtrees(); $response = new JsonResponse($subtrees); $response->setCallback('Drupal.toolbar.setSubtrees.resolve'); + + // The Expires HTTP header is the heart of the client-side HTTP caching. The + // additional server-side page cache only takes effect when the client + // accesses the callback URL again (e.g., after clearing the browser cache + // or when force-reloading a Drupal page). + $max_age = 365 * 24 * 60 * 60; + $response->setPrivate(); + $response->setMaxAge($max_age); + + $expires = new \DateTime(); + $expires->setTimestamp(REQUEST_TIME + $max_age); + $response->setExpires($expires); + return $response; } diff --git a/core/modules/toolbar/src/PageCache/AllowToolbarPath.php b/core/modules/toolbar/src/PageCache/AllowToolbarPath.php new file mode 100644 index 00000000000..81d57348c9e --- /dev/null +++ b/core/modules/toolbar/src/PageCache/AllowToolbarPath.php @@ -0,0 +1,32 @@ +getPathInfo())) { + return static::ALLOW; + } + } + +} diff --git a/core/modules/toolbar/tests/src/Unit/PageCache/AllowToolbarPathTest.php b/core/modules/toolbar/tests/src/Unit/PageCache/AllowToolbarPathTest.php new file mode 100644 index 00000000000..f1223be02e2 --- /dev/null +++ b/core/modules/toolbar/tests/src/Unit/PageCache/AllowToolbarPathTest.php @@ -0,0 +1,65 @@ +policy = new AllowToolbarPath(); + } + + /** + * Asserts that caching is allowed if the request goes to toolbar subtree. + * + * @dataProvider providerTestAllowToolbarPath + * @covers ::check + */ + public function testAllowToolbarPath($expected_result, $path) { + $request = Request::create($path); + $result = $this->policy->check($request); + $this->assertSame($expected_result, $result); + } + + /** + * Provides data and expected results for the test method. + * + * @return array + * Data and expected results. + */ + public function providerTestAllowToolbarPath() { + return [ + [NULL, '/'], + [NULL, '/other-path?q=/toolbar/subtrees/'], + [NULL, '/toolbar/subtrees/'], + [NULL, '/toolbar/subtrees/some-hash/langcode/additional-stuff'], + [RequestPolicyInterface::ALLOW, '/de/toolbar/subtrees/abcd'], + [RequestPolicyInterface::ALLOW, '/en-us/toolbar/subtrees/xyz'], + [RequestPolicyInterface::ALLOW, '/en-us/toolbar/subtrees/xyz/de'], + [RequestPolicyInterface::ALLOW, '/a/b/c/toolbar/subtrees/xyz/de'], + [RequestPolicyInterface::ALLOW, '/toolbar/subtrees/some-hash'], + [RequestPolicyInterface::ALLOW, '/toolbar/subtrees/some-hash/en'], + ]; + } + +} diff --git a/core/modules/toolbar/toolbar.module b/core/modules/toolbar/toolbar.module index 07deb3cb4c9..96439810579 100644 --- a/core/modules/toolbar/toolbar.module +++ b/core/modules/toolbar/toolbar.module @@ -95,43 +95,6 @@ function toolbar_element_info() { return $elements; } -/** - * Use Drupal's page cache for toolbar/subtrees/*, even for authenticated users. - * - * This gets invoked after full bootstrap, so must duplicate some of what's - * done by \Drupal\Core\DrupalKernel::handlePageCache(). - * - * @todo Replace this hack with something better integrated with DrupalKernel - * once Drupal's page caching itself is properly integrated. - */ -function _toolbar_initialize_page_cache() { - $GLOBALS['conf']['system.performance']['cache']['page']['enabled'] = TRUE; - drupal_page_is_cacheable(TRUE); - - // If we have a cache, serve it. - // @see \Drupal\Core\DrupalKernel::handlePageCache() - $request = \Drupal::request(); - $response = drupal_page_get_cache($request); - if ($response) { - $response->headers->set('X-Drupal-Cache', 'HIT'); - - drupal_serve_page_from_cache($response, $request); - - $response->prepare($request); - $response->send(); - // We are done. - exit; - } - - // The Expires HTTP header is the heart of the client-side HTTP caching. The - // additional server-side page cache only takes effect when the client - // accesses the callback URL again (e.g., after clearing the browser cache or - // when force-reloading a Drupal page). - $max_age = 3600 * 24 * 365; - drupal_add_http_header('Expires', gmdate(DateTimePlus::RFC7231, REQUEST_TIME + $max_age)); - drupal_add_http_header('Cache-Control', 'private, max-age=' . $max_age); -} - /** * Implements hook_page_build(). * diff --git a/core/modules/toolbar/toolbar.services.yml b/core/modules/toolbar/toolbar.services.yml index 7f269683c14..278d3c7313e 100644 --- a/core/modules/toolbar/toolbar.services.yml +++ b/core/modules/toolbar/toolbar.services.yml @@ -6,3 +6,7 @@ services: factory_method: get factory_service: cache_factory arguments: [toolbar] + toolbar.page_cache_request_policy.allow_toolbar_path: + class: Drupal\toolbar\PageCache\AllowToolbarPath + tags: + - { name: page_cache_request_policy } diff --git a/core/tests/Drupal/Tests/Core/PageCache/ChainRequestPolicyTest.php b/core/tests/Drupal/Tests/Core/PageCache/ChainRequestPolicyTest.php new file mode 100644 index 00000000000..334373557df --- /dev/null +++ b/core/tests/Drupal/Tests/Core/PageCache/ChainRequestPolicyTest.php @@ -0,0 +1,165 @@ +policy = new ChainRequestPolicy(); + $this->request = new Request(); + } + + /** + * Asserts that check() returns NULL if the chain is empty. + * + * @covers ::check + */ + public function testEmptyChain() { + $result = $this->policy->check($this->request); + $this->assertSame(NULL, $result); + } + + /** + * Asserts that check() returns NULL if a rule returns NULL. + * + * @covers ::check + */ + public function testNullRuleChain() { + $rule = $this->getMock('Drupal\Core\PageCache\RequestPolicyInterface'); + $rule->expects($this->once()) + ->method('check') + ->with($this->request) + ->will($this->returnValue(NULL)); + + $this->policy->addPolicy($rule); + + $result = $this->policy->check($this->request); + $this->assertSame(NULL, $result); + } + + /** + * Asserts that check() throws an exception if a rule returns an invalid value. + * + * @expectedException \UnexpectedValueException + * @dataProvider providerChainExceptionOnInvalidReturnValue + * @covers ::check + */ + public function testChainExceptionOnInvalidReturnValue($return_value) { + $rule = $this->getMock('Drupal\Core\PageCache\RequestPolicyInterface'); + $rule->expects($this->once()) + ->method('check') + ->with($this->request) + ->will($this->returnValue($return_value)); + + $this->policy->addPolicy($rule); + + $this->policy->check($this->request); + } + + /** + * Provides test data for testChainExceptionOnInvalidReturnValue. + * + * @return array + * Test input and expected result. + */ + public function providerChainExceptionOnInvalidReturnValue() { + return [ + [FALSE], + [0], + [1], + [TRUE], + [[1, 2, 3]], + [new \stdClass()], + ]; + } + + /** + * Asserts that check() returns ALLOW if any of the rules returns ALLOW. + * + * @dataProvider providerAllowIfAnyRuleReturnedAllow + * @covers ::check + */ + public function testAllowIfAnyRuleReturnedAllow($return_values) { + foreach ($return_values as $return_value) { + $rule = $this->getMock('Drupal\Core\PageCache\RequestPolicyInterface'); + $rule->expects($this->once()) + ->method('check') + ->with($this->request) + ->will($this->returnValue($return_value)); + + $this->policy->addPolicy($rule); + } + + $actual_result = $this->policy->check($this->request); + $this->assertSame(RequestPolicyInterface::ALLOW, $actual_result); + } + + /** + * Provides test data for testAllowIfAnyRuleReturnedAllow. + * + * @return array + * Test input and expected result. + */ + public function providerAllowIfAnyRuleReturnedAllow() { + return [ + [[RequestPolicyInterface::ALLOW]], + [[NULL, RequestPolicyInterface::ALLOW]], + ]; + } + + /** + * Asserts that check() returns immediately when a rule returned DENY. + */ + public function testStopChainOnFirstDeny() { + $rule1 = $this->getMock('Drupal\Core\PageCache\RequestPolicyInterface'); + $rule1->expects($this->once()) + ->method('check') + ->with($this->request) + ->will($this->returnValue(RequestPolicyInterface::ALLOW)); + $this->policy->addPolicy($rule1); + + $deny_rule = $this->getMock('Drupal\Core\PageCache\RequestPolicyInterface'); + $deny_rule->expects($this->once()) + ->method('check') + ->with($this->request) + ->will($this->returnValue(RequestPolicyInterface::DENY)); + $this->policy->addPolicy($deny_rule); + + $ignored_rule = $this->getMock('Drupal\Core\PageCache\RequestPolicyInterface'); + $ignored_rule->expects($this->never()) + ->method('check'); + $this->policy->addPolicy($ignored_rule); + + $actual_result = $this->policy->check($this->request); + $this->assertsame(RequestPolicyInterface::DENY, $actual_result); + } + +} diff --git a/core/tests/Drupal/Tests/Core/PageCache/ChainResponsePolicyTest.php b/core/tests/Drupal/Tests/Core/PageCache/ChainResponsePolicyTest.php new file mode 100644 index 00000000000..d9916782dba --- /dev/null +++ b/core/tests/Drupal/Tests/Core/PageCache/ChainResponsePolicyTest.php @@ -0,0 +1,140 @@ +policy = new ChainResponsePolicy(); + $this->response = new Response(); + $this->request = new Request(); + } + + /** + * Asserts that check() returns NULL if the chain is empty. + * + * @covers ::check + */ + public function testEmptyChain() { + $result = $this->policy->check($this->response, $this->request); + $this->assertSame(NULL, $result); + } + + /** + * Asserts that check() returns NULL if a rule returns NULL. + * + * @covers ::check + */ + public function testNullRuleChain() { + $rule = $this->getMock('Drupal\Core\PageCache\ResponsePolicyInterface'); + $rule->expects($this->once()) + ->method('check') + ->with($this->response, $this->request) + ->will($this->returnValue(NULL)); + + $this->policy->addPolicy($rule); + + $result = $this->policy->check($this->response, $this->request); + $this->assertSame(NULL, $result); + } + + /** + * Asserts that check() throws an exception if a rule returns an invalid value. + * + * @expectedException \UnexpectedValueException + * @dataProvider providerChainExceptionOnInvalidReturnValue + * @covers ::check + */ + public function testChainExceptionOnInvalidReturnValue($return_value) { + $rule = $this->getMock('Drupal\Core\PageCache\ResponsePolicyInterface'); + $rule->expects($this->once()) + ->method('check') + ->with($this->response, $this->request) + ->will($this->returnValue($return_value)); + + $this->policy->addPolicy($rule); + + $actual_result = $this->policy->check($this->response, $this->request); + $this->assertSame(NULL, $actual_result); + } + + /** + * Provides test data for testChainExceptionOnInvalidReturnValue. + * + * @return array + * Test input and expected result. + */ + public function providerChainExceptionOnInvalidReturnValue() { + return [ + [FALSE], + [0], + [1], + [TRUE], + [[1, 2, 3]], + [new \stdClass()], + ]; + } + + /** + * Asserts that check() returns immediately when a rule returned DENY. + */ + public function testStopChainOnFirstDeny() { + $rule1 = $this->getMock('Drupal\Core\PageCache\ResponsePolicyInterface'); + $rule1->expects($this->once()) + ->method('check') + ->with($this->response, $this->request); + $this->policy->addPolicy($rule1); + + $deny_rule = $this->getMock('Drupal\Core\PageCache\ResponsePolicyInterface'); + $deny_rule->expects($this->once()) + ->method('check') + ->with($this->response, $this->request) + ->will($this->returnValue(ResponsePolicyInterface::DENY)); + $this->policy->addPolicy($deny_rule); + + $ignored_rule = $this->getMock('Drupal\Core\PageCache\ResponsePolicyInterface'); + $ignored_rule->expects($this->never()) + ->method('check'); + $this->policy->addPolicy($ignored_rule); + + $actual_result = $this->policy->check($this->response, $this->request); + $this->assertsame(ResponsePolicyInterface::DENY, $actual_result); + } + +} diff --git a/core/tests/Drupal/Tests/Core/PageCache/CommandLineOrUnsafeMethodTest.php b/core/tests/Drupal/Tests/Core/PageCache/CommandLineOrUnsafeMethodTest.php new file mode 100644 index 00000000000..bb54fbca050 --- /dev/null +++ b/core/tests/Drupal/Tests/Core/PageCache/CommandLineOrUnsafeMethodTest.php @@ -0,0 +1,83 @@ +policy = $this->getMock('Drupal\Core\PageCache\RequestPolicy\CommandLineOrUnsafeMethod', array('isCli')); + } + + /** + * Asserts that check() returns DENY for unsafe HTTP methods. + * + * @dataProvider providerTestHttpMethod + * @covers ::check + */ + public function testHttpMethod($expected_result, $method) { + $this->policy->expects($this->once()) + ->method('isCli') + ->will($this->returnValue(FALSE)); + + $request = Request::create('/', $method); + $actual_result = $this->policy->check($request); + $this->assertSame($expected_result, $actual_result); + } + + /** + * Provides test data and expected results for the HTTP method test. + * + * @return array + * Test data and expected results. + */ + public function providerTestHttpMethod() { + return [ + [NULL, 'GET'], + [NULL, 'HEAD'], + [RequestPolicyInterface::DENY, 'POST'], + [RequestPolicyInterface::DENY, 'PUT'], + [RequestPolicyInterface::DENY, 'DELETE'], + [RequestPolicyInterface::DENY, 'OPTIONS'], + [RequestPolicyInterface::DENY, 'TRACE'], + [RequestPolicyInterface::DENY, 'CONNECT'], + ]; + } + + /** + * Asserts that check() returns DENY if running from the command line. + * + * @covers ::check + */ + public function testIsCli() { + $this->policy->expects($this->once()) + ->method('isCli') + ->will($this->returnValue(TRUE)); + + $request = Request::create('/', 'GET'); + $actual_result = $this->policy->check($request); + $this->assertSame(RequestPolicyInterface::DENY, $actual_result); + } + +} diff --git a/core/tests/Drupal/Tests/Core/PageCache/NoSessionOpenTest.php b/core/tests/Drupal/Tests/Core/PageCache/NoSessionOpenTest.php new file mode 100644 index 00000000000..f0fe4cfca20 --- /dev/null +++ b/core/tests/Drupal/Tests/Core/PageCache/NoSessionOpenTest.php @@ -0,0 +1,54 @@ +sessionCookieName = 'B1ESkdf3V4F8u27myaSAShuuHc'; + $this->policy = new RequestPolicy\NoSessionOpen($this->sessionCookieName); + } + + /** + * Asserts that caching is allowed unless there is a session cookie present. + * + * @covers ::check + */ + public function testNoAllowUnlessSessionCookiePresent() { + $request = new Request(); + $result = $this->policy->check($request); + $this->assertSame(RequestPolicyInterface::ALLOW, $result); + + $request = Request::create('/', 'GET', [], [$this->sessionCookieName => 'some-session-id']); + $result = $this->policy->check($request); + $this->assertSame(NULL, $result); + } +}