Problem/Motivation

The function drupal_page_is_cacheable() currently has too many responsibilities. Especially troublesome is the fact that it incorporates the default logic to determine whether a page is cacheable but also allows a caller to override the result. When the cacheability of a page is enforced (allowed or denied) multiple times during a page request, the last caller wins. In some situations this can be dangerous. For example when the page cache was disabled by drupal_set_message() but later in the request enabled again by third party code.

Also it is currently only possible to enforce recording of a page in the cache. There is no way of extending the logic used to check whether an attempt to retrieve and deliver a page from the cache should be made. This may lead to rather ugly hacks, e.g. _toolbar_initialize_page_cache().

Proposed resolution

Replace drupal_page_is_cacheable() with a set of services forming a flexible and extensible page cache policy. Those services are page_cache_request_policy, page_cache_response_policy, and page_cache_kill_switch. Sites implement their policies by implementing a "policy chain". The default request policy chain is \Drupal\Core\PageCache\DefaultRequestPolicy, and its behavior is unchanged in this patch. It consists of RequestPolicy\NoSessionOpen() and RequestPolicy\CommandLineOrUnsafeMethod() which disable page cache when unsafe method, CLI, or a session cookie are found.

The framework makes it possible in a clean fashion to cache some pages even for authenticated users. See AllowToolbarPath.

Remaining tasks

User interface changes

API changes

Modules can disable page cache during the response by: \Drupal::service('page_cache_kill_switch')->trigger();. This used to be drupal_page_is_cacheable(FALSE)

CommentFileSizeAuthor
#62 2263981-replace-drupal-page-is-cacheable-g2-62.diff52.46 KBznerol
#60 interdiff.txt1.28 KBznerol
#60 2263981-replace-drupal-page-is-cacheable-g2-60.diff52.52 KBznerol
#56 interdiff.txt849 bytesznerol
#56 2263981-replace-drupal-page-is-cacheable-g2-56.diff52.51 KBznerol
#53 interdiff.txt3.57 KBznerol
#53 2263981-replace-drupal-page-is-cacheable-g2-53.diff52.44 KBznerol
#52 interdiff.txt7.61 KBznerol
#52 2263981-replace-drupal-page-is-cacheable-g2-52.diff51.8 KBznerol
#50 interdiff.txt15.04 KBznerol
#50 2263981-replace-drupal-page-is-cacheable-g2-50.diff51.47 KBznerol
#48 interdiff.txt6.8 KBznerol
#48 2263981-replace-drupal-page-is-cacheable-g2-48.diff51.4 KBznerol
#46 interdiff.txt24.26 KBznerol
#46 2263981-replace-drupal-page-is-cacheable-g2-46.diff50.33 KBznerol
#42 interdiff.txt9 KBznerol
#42 2263981-replace-drupal-page-is-cacheable-g2-42.diff30.49 KBznerol
#39 interdiff.txt2.03 KBznerol
#39 2263981-replace-drupal-page-is-cacheable-g2-39.diff29.03 KBznerol
#36 interdiff-11-36.txt2.11 KBznerol
#36 2263981-replace-drupal-page-is-cacheable-g2-36.diff27 KBznerol
#20 page-cache.png419.53 KBAnonymous (not verified)
#19 2263981-18.patch25.35 KBAnonymous (not verified)
#16 2263981-15.patch24.69 KBAnonymous (not verified)
#11 2263981-replace-drupal-page-is-cacheable-g2-11.diff24.78 KBznerol
#10 2263981-replace-drupal-page-is-cacheable-10.diff22.46 KBznerol
#6 interdiff.txt1.86 KBznerol
#6 2263981-replace-drupal-page-is-cacheable-6.diff26.46 KBznerol
#3 interdiff.txt7.89 KBznerol
#3 2263981-replace-drupal-page-is-cacheable-3.diff26.5 KBznerol
#1 2263981-replace-drupal-page-is-cacheable.diff26.12 KBznerol
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

znerol’s picture

Wim Leers’s picture

Title: Introduce a robust and extensible cache-policy framework » Introduce a robust and extensible page cache-policy framework
Status: Needs review » Needs work
Issue tags: +Performance

Sorry it took so long to do a review!


  1. +++ b/core/includes/bootstrap.inc
    @@ -1582,8 +1558,26 @@ function _drupal_bootstrap_kernel() {
    + *   obsolete as soon as deliver of cached pages is removed from the bootstrap.
    

    s/deliver/delivery/

    Please refer to a specific issue.

  2. +++ b/core/includes/bootstrap.inc
    @@ -1582,8 +1558,26 @@ function _drupal_bootstrap_kernel() {
    +function _drupal_bootstrap_page_cache($disable = NULL) {
    
    +++ b/core/includes/utility.inc
    @@ -46,8 +46,8 @@ function drupal_rebuild() {
    -  // Disable the page cache.
    -  drupal_page_is_cacheable(FALSE);
    +  // Disable delivery of cached pages.
    +  _drupal_bootstrap_page_cache(TRUE);
    

    Wouldn't $enable be more clear? Or, alternatively, rename _drupal_bootstrap_page_cache().

  3. +++ b/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php
    @@ -39,16 +41,36 @@ class FinishResponseSubscriber implements EventSubscriberInterface {
    +   * A policy rule determining the cacheability of a request.
    +   *
    +   * @var \Drupal\Core\PageCache\RequestPolicyRuleInterface
    +   */
    +  protected $requestPolicy;
    

    First, it says "policy rule".
    Then, the typehint also says "policy rule".
    But the variable name says "policy".

    "Policy" implies that there may be multiple rules, that together form a policy.

    Hence this is quite confusing. I do see how e.g. DefaultRequestPolicy implements PolicyRuleInterface, accepts multiple policy rules and then applies them all — i.e. forming an "aggregate policy rule". However, in the grand scheme of things this is only minor. But if you have an idea to make it clearer, that's of course welcome.

  4. +++ b/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php
    @@ -81,18 +103,19 @@ public function onRespond(FilterResponseEvent $event) {
    +    $is_cacheable = $this->requestPolicy->apply($request) && $this->responsePolicy->apply($response, $request);
    

    This reads as "is cachable if the request policy is applied and the response policy is applied".

    Hence I think apply() is a misnomer. What about isCachable(), accepts(), or something else that indicates whether the policy does not forbid the cacheability?

  5. +++ b/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php
    @@ -81,18 +103,19 @@ public function onRespond(FilterResponseEvent $event) {
    +    elseif ($is_configured && !$is_customized) {
    +      $this->setResponseCacheable($response, $request);
    +    }
    

    At first sight, this seems to imply that if the Cache-Control header is customized, that the response will not be marked as cacheable.
    But I don't think that's the case. I think what this essentially is saying is "if the response should be cacheable, and nothing has marked it as cacheable yet, then mark it as cacheable according to the configuration".

    If that's correct, this could use a comment explaining exactly that.

  6. +++ b/core/lib/Drupal/Core/PageCache/DefaultRequestPolicy.php
    @@ -0,0 +1,23 @@
    + * The default request policy.
    
    +++ b/core/lib/Drupal/Core/PageCache/DefaultResponsePolicy.php
    @@ -0,0 +1,23 @@
    + * Default response policy.
    

    Super nitpicky: Either both should have "The", or neither.

    And it's not really the "default request/response policy", but the "default page caching request/response policy".

    The namespace implies that for the class name, so that's fine, but the docs should "expand" to the full meaning.

  7. +++ b/core/lib/Drupal/Core/PageCache/DefaultRequestPolicy.php
    @@ -0,0 +1,23 @@
    +   * Construct the default request policy.
    
    +++ b/core/lib/Drupal/Core/PageCache/DefaultResponsePolicy.php
    @@ -0,0 +1,23 @@
    +   * Construct the default response policy.
    

    s/Construct/Constructs/

    Always 3rd person singular.

  8. +++ b/core/lib/Drupal/Core/PageCache/DefaultRequestPolicy.php
    @@ -0,0 +1,23 @@
    +    $this->deny(new RequestPolicyRule\CommandLineOrUnsafeMethod());
    +    $this->allow(new RequestPolicyRule\NoSessionOpen());
    

    These should be mentioned in the docblock: a brief explanation of the default request policy.

  9. +++ b/core/lib/Drupal/Core/PageCache/RequestPolicyRule/CommandLineOrUnsafeMethod.php
    @@ -0,0 +1,37 @@
    +   * Exclude a page request when run from a command line script.
    

    Indicates whether this is a CLI request.

  10. +++ b/core/lib/Drupal/Core/PageCache/ResponsePolicyRuleInterface.php
    @@ -0,0 +1,31 @@
    +   * @return bool
    +   *   TRUE when the policy rule allows caching of the response.
    +   */
    +  public function apply(Response $response, Request $request);
    

    This is the mismatch I mentioned earlier. The @return documentation is clear and logical, but the method name doesn't match that, IMO.

    We indeed apply policy rules to a request or a response, but IMHO isCacheable() would be much more appropriate than apply().

znerol’s picture

Status: Needs work » Needs review
FileSize
26.5 KB
7.89 KB

Thanks for the review. Addressed everything except 3, 4 and 10. I need some more time for those.

Wim Leers’s picture

Interdiff looks great. Hopefully we can get this to RTBC quickly now :) Looking forward to the fix for 3/4/10.

Status: Needs review » Needs work

The last submitted patch, 3: 2263981-replace-drupal-page-is-cacheable-3.diff, failed testing.

znerol’s picture

Status: Needs work » Needs review
FileSize
26.46 KB
1.86 KB

Relocate IsToolbarPath.php to the proper PSR-4 directory.

znerol’s picture

Regarding #2.10:

The @return documentation is clear and logical, but the method name doesn't match that, IMO.

I feel that it is the other way around. The @return documentation in fact does not reflect the implementation. Currently the RequestPolicy::apply() works like this:

  1. Immediately return FALSE if any of the deny-rules returns TRUE
  2. Immediately return TRUE if any of the allow-rules returns TRUE
  3. By default return FALSE

So the meaning of the RequestRuleInterface::apply() is inverted when used as a deny-rule. This helps preventing double-negation. For example KillSwitch::apply() returns TRUE if the kill-switch was triggered, CommandLineOrUnsafeMethod::apply() returns TRUE when in command-line mode, IsBinaryFileResponse::apply() returns TRUE if the given response is a BinaryFileResponse.

On the other hand IsToolbarPath::apply() returns TRUE if the request is directed to /toolbar/subtrees/{hash}. The IsToolbarPath is used in the allow-chain of the RequestPolicy.

Perhaps we should figure out whether this is the proper approach before trying to fix the comment or the API.

msonnabaum’s picture

I need to look at the rest of the patch more closely, but it seems like this is introducing behavior that didnt exist before. Can we please keep this patch scoped to a refactor and leave the rest for followups?

For example, it appears that #6 is denying on binary files, which is very bad because they need to be sending cacheable headers.

znerol’s picture

@msonnabaum Regarding stream / binary file response, please take a look at #2218463: Private file and image style downloads throw 500 error when page cache is active.

znerol’s picture

Reroll after the kernel bootstrap patch. As a result it is possible to kill the workaround which was necessary to disable retrieval of cached pages during manual rebuild in drupal_rebuild(). Also this removes the response policy rules for binary file / stream responses.

znerol’s picture

Trying a new approach modeled after the access-checker we have in core already. This should address #10 regarding the naming and behavior of apply() by replacing it with a check() method which is inline with the other policy-implementations we have already in HEAD (Symfony and Drupal).

Berdir’s picture

Issue tags: +needs profiling

This looks great, but we should probably do some profiling to see how much it affects performance. Obviously it will be slower, but we will be able to deal with cases like toolbar in a much nicer way so that it will be possible to return a cached response there much faster. But we want to know how much we lose for that :)

Wim Leers’s picture

Like Berdir, I think this looks great!

I only have one naming concern., Other than that I think this is good to go, but could use more reviews and of course the profiling that Berdir asked for.


  1. +++ b/core/lib/Drupal/Core/PageCache/ChainRequestPolicy.php
    @@ -0,0 +1,69 @@
    + * Implements a compound request policy.
    ...
    +class ChainRequestPolicy implements ChainRequestPolicyInterface {
    

    "compound" vs. "chain". Let's always call it either? I think "compound" makes most sense, but I haven't got a strong preference. (The fact that it's evaluated as a chain is an implementation detail.)

  2. +++ b/core/lib/Drupal/Core/PageCache/ChainRequestPolicy.php
    @@ -0,0 +1,69 @@
    +  protected $rules = [];
    

    :)

moshe weitzman’s picture

Anyone available to move this ahead? @znerol?

Anonymous’s picture

Status: Needs review » Needs work

apply patch fails all over the place, i'll try to get a reroll working today.

Anonymous’s picture

FileSize
24.69 KB

here's a reroll.

$ git apply 2263981-replace-drupal-page-is-cacheable-g2-11.diff
error: patch failed: core/core.services.yml:618
error: core/core.services.yml: patch does not apply
error: patch failed: core/includes/bootstrap.inc:385
error: core/includes/bootstrap.inc: patch does not apply
error: patch failed: core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php:10
error: core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php: patch does not apply
error: patch failed: core/lib/Drupal/Core/Session/SessionManager.php:126
error: core/lib/Drupal/Core/Session/SessionManager.php: patch does not apply
error: patch failed: core/modules/toolbar/toolbar.module:107
error: core/modules/toolbar/toolbar.module: patch does not apply

had to manually put the hunks for the 5 files above back in, hopefully i didn't break too much.

Anonymous’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 16: 2263981-15.patch, failed testing.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
25.35 KB

there was a test that still used drupal_page_is_cacheable(), interdiff:

diff --git a/core/modules/image/src/Controller/ImageStyleDownloadController.php b/core/modules/image/src/Controller/ImageStyleDownloadCo
index 3e4bcf0..dfce924 100644
--- a/core/modules/image/src/Controller/ImageStyleDownloadController.php
+++ b/core/modules/image/src/Controller/ImageStyleDownloadController.php
@@ -155,7 +155,7 @@ public function deliver(Request $request, $scheme, ImageStyleInterface $image_st
     }
 
     if ($success) {
-      drupal_page_is_cacheable(FALSE);
+      \Drupal::service('page_cache_kill_switch')->trigger();
       $image = $this->imageFactory->get($derivative_uri);
       $uri = $image->getSource();
       $headers += array(
Anonymous’s picture

FileSize
419.53 KB

i did some profiling, and this patch adds 68 function calls to the page cache hit code path.

unsurprisingly, a bunch of the added overhead is in loading more class code and calling some implementation methods.

i didn't get a consistent enough wall-time across runs to say much about the difference there, but i'm satisfied that the overhead this patch introduces is not 'too much'.

diff

Wim Leers’s picture

Issue tags: -needs profiling

Awesome, thanks!

That only leaves this remark of mine from #13:

+++ b/core/lib/Drupal/Core/PageCache/ChainRequestPolicy.php
@@ -0,0 +1,69 @@
+ * Implements a compound request policy.
...
+class ChainRequestPolicy implements ChainRequestPolicyInterface {

"compound" vs. "chain". Let's always call it either? I think "compound" makes most sense, but I haven't got a strong preference. (The fact that it's evaluated as a chain is an implementation detail.)

What do you think about this, beejeebus and moshe? If you think it's unimportant, feel free to ignore it and RTBC this :)

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

I'm fine with either word. If someone rerolls it, they can pick :). Its such a nit - lets go to RTBC.

Crell’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/lib/Drupal/Core/PageCache/RequestPolicy/CommandLineOrUnsafeMethod.php
    @@ -0,0 +1,38 @@
    +class CommandLineOrUnsafeMethod implements RequestPolicyInterface {
    

    OR is a code smell. These shouldn't be tucked into one rule class.

  2. +++ b/core/lib/Drupal/Core/PageCache/RequestPolicy/CommandLineOrUnsafeMethod.php
    @@ -0,0 +1,38 @@
    +    if ($this->isCliRequest($request) || !$request->isMethodSafe()) {
    

    Disabling caching if the method isn't safe really ought to go in Symfony itself. Can we submit an upstream patch for Response::prepare() so that this isn't necessary?

  3. +++ b/core/lib/Drupal/Core/PageCache/ResponsePolicy/KillSwitch.php
    @@ -0,0 +1,42 @@
    +class KillSwitch implements ResponsePolicyInterface {
    +
    +  /**
    +   * A flag indicating whether the kill switch was triggered.
    +   *
    +   * @var bool
    +   */
    +  protected $kill = FALSE;
    +
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function check(Response $response, Request $request) {
    +    if ($this->kill) {
    +      return static::DENY;
    +    }
    +  }
    +
    +  /**
    +   * Deny any page caching on the current request.
    +   */
    +  public function trigger() {
    +    $this->kill = TRUE;
    +  }
    +
    +}
    

    This isn't a service. It's a stateful global wrapped up into a class. We need a better way to handle this because stateful global services are how we destroy the loose coupling work we've been trying to do.

  4. +++ b/core/lib/Drupal/Core/PageCache/ResponsePolicyInterface.php
    @@ -0,0 +1,37 @@
    +  /**
    +   * Deny storage of a page in the cache.
    +   */
    +  const DENY = 'deny';
    

    Can we centralize all of these constants? There's no need to repeat them in 3 different interfaces.

Crell’s picture

Issue tags: +TCDrupal 2014
znerol’s picture

  1. My reasons for not splitting this class up after your review in #2177461-68: Refactor page caching into a service are the followings:
    • Both of those rules are very fundamental and I cannot think of any use case where it would be appropriate to use only one of them without using the other.
    • The class is more than trivial. Splitting it up would make the code more complex for no benefit.

    There would also be the option of renaming the class into something like [...]\RequestPolicyRule\FundamentalRule. However, I prefer the current precise name.

  2. Response::prepare() is called just before the response is sent to the browser and after it is stored in / retrieved from the cache. Response::isCacheable() is no option either, because there we do not have access to the request (to test the HTTP method). In fact Request::isMethodSafe() is called from within symfony HttpCache::handle() but the symfony code never checks whether it is running in the CLI. We need a home for both checks as long as we do not use HttpCache and even if we manage to implement it, we still need the CLI-check. Unless of course we manage to not wrap the kernel into HttpCache when running from the CLI.
  3. I do not see a way around global state here as long as we do not have access to the response from within drupal_set_message() and drupal_rebuild(), or alternatively those methods get eliminated and we have other ways to check whether a message was rendered on a page or not.
  4. If we centralize them we'd loose the option to document them separately and also cannot write static::DENY anymore.
Crell’s picture

3. There's another issue that is replacing drupal_set_message() with a service. It's not quite in but worth looking into to see it helps here: #2278383: Create an injectible service for drupal_set_message()

4. What we did for access control was to move the constants to an interface that all of the other objects/interfaces can implement. Since it's just constants, no methods, it means the static:: constants become available immediately as soon as the interface is implemented. That should work fine here, too, as the various other interfaces can just extend that interface and all is well.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community

So #23.1 and #23.2 have been answered by znerol in #25.

#23.3 was answered and #26.3 shows that improving this further is indeed out of scope — I don't think any of us want to block this issue on #2278383: Create an injectible service for drupal_set_message(), which hasn't been making progress lately.

So I think that just leaves point 4. I agree with #26's general reasoning, but AccessInterface is about access checking, not page caching. So I don't think this would make a lot of sense. Especially because AccessInterface::ALLOW and AccessInterface::KILL would be meaningless. Finally, we don't want to make the page cache system dependent on the access system: they're orthogonal.

Therefore: back to RTBC.

Crell’s picture

I'm not suggesting we use the same interface as Access is using. Just that the cache system use its own "constants interface" rather than repeating things. It would have on it the same constants that are defined multiple times here.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record, +Needs issue summary update

We need a change record and issue summary update.

with a set of services forming a flexible and extensible page cache policy.

A bit more detail would be great. Thanks!

moshe weitzman’s picture

Issue summary: View changes
moshe weitzman’s picture

Status: Needs work » Reviewed & tested by the community

I expanded the Issue Summary and drafted a CR

Crell’s picture

Status: Reviewed & tested by the community » Needs work

If nothing else #28 is still outstanding.

znerol’s picture

Re #28/#32 I'd like to point out that the check() method of request policy rules (those implementing the RequestPolicyInterface) may return any of allow or deny while the one of a response policy rule only may return a deny action.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs change record, -Needs issue summary update

#30 and #31 addressed #29.

And #33 addressed #32.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 19: 2263981-18.patch, failed testing.

znerol’s picture

Status: Needs work » Needs review
FileSize
27 KB
2.11 KB

This is a reroll of #11. Instead of triggering the kill-switch as suggested in #19, the proper way to disable the page cache for the image-style download controller is to exclude it using a request-rule. This has the advantage that sites using an advanced cache system can swap out that rule if necessary.

znerol’s picture

Updated the change record.

Status: Needs review » Needs work

The last submitted patch, 36: 2263981-replace-drupal-page-is-cacheable-g2-36.diff, failed testing.

znerol’s picture

Status: Needs work » Needs review
FileSize
29.03 KB
2.03 KB

Fix NodePreviewController.

Berdir’s picture

+++ b/core/modules/node/src/PageCache/IsPreviewPath.php
@@ -0,0 +1,30 @@
+  public function check(Request $request) {
+    if (strpos($request->getPathInfo(), '/node/preview/') === 0) {
+      return RequestPolicyInterface::DENY;
+    }
+  }

After having similar problems in the globalredirect 8.x port, are you sure this is working correctly with language prefixes? AFAIK, the language prefix will be included in getPathInfo(). Especially because this probably happens before language negotiation?

znerol’s picture

@Berdir: Right, this is bad. What did you do about that over at Global Redirect?

znerol’s picture

Let's switch to a response-policy for the image and node modules: Prevent that the response is saved to the page cache based on the route name.

The toolbar policy cannot be converted to a response policy, because it needs to be run before attempting to retrieve a page from the cache. Therefore let's just extend the check such that it also works when there is exactly one path-component before the toolbar-subtree path.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

Interdiff looks good, and we are green. RTBC.

chx’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Needs work

if ($cache_enabled && $request_policy->check($request) == RequestPolicyInterface::ALLOW)

erm nope.

if ($cache_enabled && $request_policy->check($request) === RequestPolicyInterface::ALLOW)

better.

  protected function isCliRequest(Request $request) {
    return PHP_SAPI === 'cli';
  }

why does this even exist? And why does it get $request? Very weird. Just inline it.

znerol’s picture

erm nope.

Doh, you're certainly right. There are more, e.g. in ChainRequestPolicy.

why does this even exist?

I was about to say because of the unit-tests, then I realized that they are missing. I'm sure I've written some, they probably live in some other branch or in a stash...

znerol’s picture

Status: Needs work » Needs review
FileSize
50.33 KB
24.26 KB

Use identity instead of equality operator when checking return-values, add unit-tests.

chx’s picture

Status: Needs review » Reviewed & tested by the community

Much better.

znerol’s picture

As suggested by @chx in IRC, make ChainRequestPolicy and ChainResponsePolicy throw an exception if garbage is returned from a rule.

chx’s picture

I like robust APIs. It is not unreasonable that someone will return an unwanted TRUE or FALSE instead of the constants. This very nicely covers that. Thanks, znerol.

znerol’s picture

Uhm, turns out that I've put my unit-tests into the wrong place. Inside modules they need to be in tests/src/Unit instead of src/Tests.

Also changed the name of the new result variable in ChainRequestPolicy::check() from $overall_result to $final_result.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/lib/Drupal/Core/PageCache/RequestPolicy/NoSessionOpen.php
    @@ -0,0 +1,46 @@
    +  /**
    +   * Constructs a new page cache session policy.
    +   */
    +  public function __construct($session_cookie_name = NULL) {
    

    Missing @param

  2. +++ b/core/lib/Drupal/Core/PageCache/RequestPolicyInterface.php
    @@ -0,0 +1,41 @@
    +  /**
    +   * Determines whether delivery of a cached page should be attempted.
    +   *
    +   * @return string|NULL
    +   *   One of static::ALLOW, static::DENY or NULL. Calling code may attempt to
    +   *   deliver a cached page if static::ALLOW is returned. Returns NULL if the
    +   *   policy is not specified for the given request.
    +   */
    +  public function check(Request $request);
    

    Missing @param

  3. +++ b/core/lib/Drupal/Core/PageCache/ResponsePolicyInterface.php
    @@ -0,0 +1,37 @@
    +  /**
    +   * Determines whether it is save to store a page in the cache.
    +   *
    +   * @return string|NULL
    +   *   Either static::DENY or NULL. Calling code may attempt to store a page in
    +   *   the cache unless static::DENY is returned. Returns NULL if the policy
    +   *   policy is not specified for the given response.
    +   */
    +  public function check(Response $response, Request $request);
    

    Missing @params

  4. +++ b/core/modules/image/src/PageCache/DenyPrivateImageStyleDownload.php
    @@ -0,0 +1,49 @@
    +  /**
    +   * Test whether this request goes to /image/preview.
    +   */
    +  public function check(Response $response, Request $request) {
    

    {@inheritdoc}

  5. +++ b/core/modules/image/tests/src/Unit/PageCache/DenyPrivateImageStyleDownloadTest.php
    @@ -0,0 +1,90 @@
    +   * @var \Drupal\Core\PageCache\DenyPrivateImageStyleDownload
    

    This class does not exist

  6. +++ b/core/modules/image/tests/src/Unit/PageCache/DenyPrivateImageStyleDownloadTest.php
    @@ -0,0 +1,90 @@
    +   * Asserts that chaching is denied on the private image style download route.
    
    +++ b/core/modules/node/tests/src/Unit/PageCache/DenyNodePreviewTest.php
    @@ -0,0 +1,90 @@
    +   * Asserts that chaching is denied on the node preview route.
    

    chaching? :)

  7. +++ b/core/modules/node/src/PageCache/DenyNodePreview.php
    @@ -0,0 +1,49 @@
    +  /**
    +   * Test whether this request goes to /node/preview.
    +   */
    +  public function check(Response $response, Request $request) {
    

    {@inheritdoc}

  8. +++ b/core/modules/node/tests/src/Unit/PageCache/DenyNodePreviewTest.php
    @@ -0,0 +1,90 @@
    +   * @var \Drupal\Core\PageCache\DenyNodePreview
    

    This class does not exist.

  9. +++ b/core/modules/toolbar/src/PageCache/AllowToolbarPath.php
    @@ -0,0 +1,37 @@
    +    $path = $request->getPathInfo();
    +    $toolbar_path_offset = strpos($path, '/toolbar/subtrees/');
    +    if ($toolbar_path_offset !== FALSE) {
    +      // Also accept paths matching at the first slash in order to cater for
    +      // sites using language prefixes.
    +      $second_slash = strpos($path, '/', 1);
    +      if ($toolbar_path_offset === 0 || $toolbar_path_offset === $second_slash) {
    +        return static::ALLOW;
    +      }
    +    }
    

    This could (perhaps should) be simplified by using the RouteMatch service. Plus the example in the CR is wrong because of this - it does not take account of the language prefix issue either.

  10. +++ b/core/modules/toolbar/tests/src/Unit/PageCache/AllowToolbarPathTest.php
    @@ -0,0 +1,60 @@
    +   * @var \Drupal\Core\PageCache\RequestPolicy\AllowToolbarPath
    

    This class not exist.

  11. +++ b/core/tests/Drupal/Tests/Core/PageCache/ChainRequestPolicyTest.php
    @@ -0,0 +1,165 @@
    +   * Asserts that check() returns imediately when a rule returned DENY.
    
    +++ b/core/tests/Drupal/Tests/Core/PageCache/ChainResponsePolicyTest.php
    @@ -0,0 +1,140 @@
    +   * Asserts that check() returns imediately when a rule returned DENY.
    

    imediately?

  12. +++ b/core/modules/toolbar/src/PageCache/AllowToolbarPath.php
    @@ -0,0 +1,37 @@
    +  /**
    +   * Test whether this request goes to /toolbar/subtrees.
    +   */
    +  public function check(Request $request) {
    

    {@inheritdoc}

znerol’s picture

Thanks for the review @alexpott, fixed the documentation strings. Regarding #51.9, the request-policy-checks run way before the router. I've updated the change notice to better highlight that fact.

znerol’s picture

Status: Needs work » Needs review
FileSize
52.44 KB
3.57 KB

Discussed the AllowToolbarPath::check() method with @alexpott and @Berdir in IRC. Let's use a regular expression matching the end of pathinfo in order to cater for arbitrary path prefixes. Also added a note to the RequestPolicyInterface which highlights the peculiarities that implementors need to consider.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

Thx - back to RTBC

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
diff --git a/core/modules/toolbar/tests/src/Unit/PageCache/AllowToolbarPathTest.php b/core/modules/toolbar/tests/src/Unit/PageCache/AllowToolbarPathTest.php
index aef4ec8..f1223be 100644
--- a/core/modules/toolbar/tests/src/Unit/PageCache/AllowToolbarPathTest.php
+++ b/core/modules/toolbar/tests/src/Unit/PageCache/AllowToolbarPathTest.php
@@ -56,6 +56,7 @@ public function providerTestAllowToolbarPath() {
       [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'],
     ];

I think we should add another test - something like.

Once that is in I'm going to assign to @catch for rtbc review since I think this is an important and really good patch and therefore the more eyes the better.

znerol’s picture

Status: Needs work » Needs review
FileSize
52.51 KB
849 bytes

Done.

alexpott’s picture

Assigned: Unassigned » catch
Status: Needs review » Reviewed & tested by the community

Great - I think this is good to go - assigning to catch for rtbc review.

catch’s picture

  1. +++ b/core/lib/Drupal/Core/PageCache/ChainRequestPolicy.php
    @@ -0,0 +1,66 @@
    + * </ol>
    

    Should this line clarify "if any of the rules evaluated to static::ALLOW and none have evaluated to static::DENY"?

  2. +++ b/core/lib/Drupal/Core/PageCache/ChainRequestPolicy.php
    @@ -0,0 +1,66 @@
    +      }
    

    I'd probably change this to check DENY first and early return - makes it extra clear that that overrides everything else, right now there is no note in the documentation, and only the break; enforces it. Also similar methods elsewhere in the patch are doing early return.

catch’s picture

+++ b/core/lib/Drupal/Core/PageCache/ChainRequestPolicy.php
@@ -0,0 +1,66 @@
+        break;

Er, failed with dreditor last comment, meant here...

znerol’s picture

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 60: 2263981-replace-drupal-page-is-cacheable-g2-60.diff, failed testing.

znerol’s picture

Status: Needs work » Needs review
FileSize
52.46 KB
chx’s picture

Status: Needs review » Reviewed & tested by the community

Round #2.

alexpott’s picture

Assigned: catch » Unassigned
Status: Reviewed & tested by the community » Fixed

I'm happy that @catch review this in #58 and left it rtbc.

Committed fb6c562 and pushed to 8.0.x. Thanks!

  • alexpott committed fb6c562 on 8.0.x
    Issue #2263981 by znerol, beejeebus: Introduce a robust and extensible...
Wim Leers’s picture

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.