Refactor page caching into a service

Great work so far.

I tried to figure out whether this will let run more code or less code and indeed this does not change the behavior, as we still basically execute after the kernel is ready. This should be better documented in the issue summary to be honest.

Here is a bit more deep review.

1. +++ b/core/core.services.yml
   @@ -66,6 +66,11 @@ services:
        arguments: [path]
   +  page_cache.internal:
   +    class: Drupal\Core\PageCache\InternalPageCache
   +    arguments: ['@cache.page', '@content_negotiation', '@config.factory', '@settings']
   +  page_cache.policy:
   +    class: Drupal\Core\PageCache\DefaultPageCachePolicy
   

   Initial thought:
   Just in general it is odd that we need two services to be injected in the finish response subscriber.

2. +++ b/core/includes/bootstrap.inc
   @@ -1456,7 +1301,7 @@ function drupal_set_message($message = NULL, $type = 'status', $repeat = FALSE)
   +    \Drupal::service('page_cache.policy')->cancel();
   ...
        // Mark this page as being uncacheable.
   

   The new code is certainly less descriptive.

3. +++ b/core/includes/bootstrap.inc
   @@ -1705,6 +1549,19 @@ function drupal_handle_request($test_only = FALSE) {
   +
   +  // Attempt to serve a cached response from the internal page cache.
   +  $use_internal_cache = \Drupal::config('system.performance')->get('cache.page.use_internal');
   +  if ($use_internal_cache && $cache_policy->isCacheable()) {
   +    $page_cache = \Drupal::service('page_cache.internal');
   +    if ($page_cache->deliver($request)) {
   +      exit();
   +    }
   

   Is it just me that the configuration should be hidden inside the page_cache.internal service? Maybe we should also find a better name.

4. +++ b/core/includes/bootstrap.inc
   @@ -2670,6 +2480,57 @@ function drupal_check_memory_limit($required, $memory_limit = NULL) {
   + * @} End of "defgroup lock".
   

   This certainly looks wrong.

5. +++ b/core/includes/bootstrap.inc
   @@ -2670,6 +2480,57 @@ function drupal_check_memory_limit($required, $memory_limit = NULL) {
   + * @defgroup cache Functions for cache handling
   

   Is it just me that moving this deprecated methods feels out of scope of this method? Can't we simply load cache.inc a little bit earlier?

6. +++ b/core/includes/utility.inc
   @@ -45,15 +45,6 @@ function drupal_rebuild() {
    
   -  // Disable the page cache.
   -  drupal_page_is_cacheable(FALSE);
   -
   -  // Bootstrap up to where caches exist and clear them.
   -  drupal_bootstrap(DRUPAL_BOOTSTRAP_PAGE_CACHE);
   -  foreach (Cache::getBins() as $bin) {
   -    $bin->deleteAll();
   -  }
   

   What happened with this functionality?

7. +++ b/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php
   @@ -22,18 +26,49 @@ class FinishResponseSubscriber implements EventSubscriberInterface {
   +   * @var \Drupal\Core\Language\LanguageManager
   

   Note: we do have an interface for this now.

8. +++ b/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php
   @@ -22,18 +26,49 @@ class FinishResponseSubscriber implements EventSubscriberInterface {
   +   * The PageCachePolicy object necessary to decide whether a page is cacheable.
   

   Maybe we find a better word for the pageCachePolicy object that this becomes basically obvious.

9. +++ b/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php
   @@ -22,18 +26,49 @@ class FinishResponseSubscriber implements EventSubscriberInterface {
   +   * The InternalPageCache object used to store cached pages.
   ...
   +  protected $internalPageCache;
   

   So what about using PageCacheStorage instead?

10. +++ b/core/lib/Drupal/Core/PageCache/DefaultPageCachePolicy.php
    @@ -0,0 +1,101 @@
    + * Definition of Drupal\Core\PageCache\DefaultPageCachePolicy
    

    The convention is to use "Contains \Drupal..."

11. +++ b/core/lib/Drupal/Core/PageCache/DefaultPageCachePolicy.php
    @@ -0,0 +1,101 @@
    +/**
    + * Page caching service based on the internal cache.
    + */
    +class DefaultPageCachePolicy implements PageCachePolicyInterface {
    

    Both the name and the documentation should explain what this thing is doing.

12. +++ b/core/lib/Drupal/Core/PageCache/DefaultPageCachePolicy.php
    @@ -0,0 +1,101 @@
    +  /**
    +   * Prevent that a page gets saved to the cache.
    +   */
    +  public function cancel() {
    +    $this->canceled = TRUE;
    +  }
    

    maybe we could use something like disableForThisRequest() or something like that?

13. +++ b/core/lib/Drupal/Core/PageCache/DefaultPageCachePolicy.php
    @@ -0,0 +1,101 @@
    +  public function isCacheable() {
    ...
    +  public function isCanceled() {
    ...
    +  public function isExcluded() {
    

    I wonder whether we need all this public methods if just isCacheable is important at the end..

14. +++ b/core/lib/Drupal/Core/PageCache/DefaultPageCachePolicy.php
    @@ -0,0 +1,101 @@
    +    $this->excludeOpenSession($request);
    ...
    +    $this->excludeUncacheableHTTPMethod($request);
    +    $this->excludeCLIRequest($request);
    

    Nice splitup!

15. +++ b/core/lib/Drupal/Core/PageCache/DefaultPageCachePolicy.php
    @@ -0,0 +1,101 @@
    +    if ($_SERVER['REQUEST_METHOD'] != 'GET' && $_SERVER['REQUEST_METHOD'] != 'HEAD') {
    

    Aren't these two information stored on the request object as well? Request->getMethod() should do that.

16. +++ b/core/modules/toolbar/lib/Drupal/toolbar/PageCache/ToolbarPageCachePolicy.php
    @@ -0,0 +1,23 @@
    +}
    ...
    +/**
    + * Page caching service based on the internal cache.
    + */
    +class ToolbarPageCachePolicy extends DefaultPageCachePolicy {
    +  /**
    +   * Do *not* exclude users with an open session.
    +   */
    +  protected function excludeOpenSession(Request $request) {
    +    // Intentionally left blank.
    +  }
    

    So just to be clear, this caches the response of a toolbar not something we consider as "page"?

17. +++ b/core/modules/toolbar/toolbar.module
    @@ -114,28 +114,19 @@ function toolbar_element_info() {
    +  // Replace page cache policy with our own implementation.
    +  $toolbar_page_cache_policy = \Drupal::service('toolbar.page_cache.policy');
    +  \Drupal::getContainer()->set('page_cache.policy', $toolbar_page_cache_policy);
    

    Mhh it really feels wrong to dynamically exchange the service to be honest, though copying the page cache logic, as it was before, is certainly even worse. ... possible patterns we could use here would be observer (throwing an event on which multiple page cache policies can work on)

Marking the old function names as deprecated as part of #2183187: [META] Remove or document all obsolete APIs before beta. I've also opened #2184985: Remove cache_invalidate_tags() and cache() to actually remove them, postponed on this.

Doing this I spotted that the use statement in cache.inc was incorrect. I'm surprised this wasn't picked up by tests. Hopefully that's because we are testing the new versions of the functions, rather than the legacy wrappers.

I clearly need to brush up on my namespaces. This just reverses the change I made to the use statement.

I discussed the figures in #42 on irc and pointed out that my real-life D7 site takes around 40-80ms to serve a page from the cache, rather than the 9ms shown in the tests above.

If we assume the network is adding 40ms to the page load time, then moving from 7x DB cache to 8x patch default loader would take the mean load time from 49ms to 63ms. That's still a 14ms regression, but it's now only about 30%.

Max page throughput is likely to be greatly reduced, but if that's a concern then you should probably be using a dedicated cache such as varnish or a CDN rather than Drupal's page cache.

Didn't we use to have basically both with the old no_database thing, that you could set in settings.php, before it connected to the database to load variables? Should be easy enough to keep that concept with a setting that wins when set otherwise fall back to a config?

Oh BTW, the weird test fails that you had locally have been fixed in #2182077: Editor manager in $form_state causes test failures on PHP 5.4

I came here via #2209583: Remove use of drupal_set_title()/drupal_get_title()/_system_path from page cache — didn't know about this at all!

Great work so far, and thanks for your excellent issue summary and comments — that really helps when digesting a patch as big as this one! Also, big kudos for taking this on in the first place, and finally: thanks for cutting away so much cruft!

General feedback

1. I like DRY and small, easier-to-understand components as much as anyone, but it feels like we're adding far more services than is really necessary. I was already thinking that in the early iterations of this patch (yeah, I read the entire issue and looked at most patches along the way, to get a sense of evolution), but definitely in the latest patch in #57. If I didn't miscount, that's a grand total of eighteen new services! What is the performance cost of that? Do we have performance numbers that show the cost of adding more services?
2. I said I really liked your earlier comments that explained the architecture. That's what I'm missing now, in the latest iteration. Could you provide a high-level explanation of how it all fits together? Usually, it's not that hard to figure out, but in this case, you're adding a few dozen new files, which makes it much harder.
3. I'm still sorely missing additional test coverage — but that's fine, because you're still experimenting in the first place :)
4. One thing that hasn't been mentioned at all (probably because it's so crazy): what if we switch to storing cached pages on disk instead of in DB? Sites that have multiple web heads (for which this wouldn't work probably) usually have (and probably should have) a custom caching strategy anyway. Then the smaller sites that are satisfied by Drupal's page cache can simply not hit Drupal at all!

(Partial) code review

1. +++ b/core/core.services.yml
   @@ -64,6 +64,63 @@ services:
   +  page_cache.public_external_cache:
   
   +++ b/core/includes/bootstrap.inc
   @@ -1579,19 +1413,36 @@ function drupal_handle_request($test_only = FALSE) {
   +  if ($page_cache_helper->deliverFromCacheBeforeKernel(settings(), $request)) {
   ...
   +  if ($page_cache_helper->deliverFromCacheAfterContainer(\Drupal::service('page_cache'), $request)) {
   

   "before kernel" vs. "after container"

   s/container/kernel/? :)

2. +++ b/core/includes/bootstrap.inc
   @@ -1579,19 +1413,36 @@ function drupal_handle_request($test_only = FALSE) {
   +  $page_cache_helper = new PageCacheHelper();
   

   s/PageCacheHelper/PageCache/ would make the code cleaner.

3. +++ b/core/includes/utility.inc
   @@ -41,16 +41,12 @@ function drupal_rebuild() {
      // Bootstrap up to where caches exist and clear them.
   -  drupal_bootstrap(DRUPAL_BOOTSTRAP_PAGE_CACHE);
   +  drupal_bootstrap(DRUPAL_BOOTSTRAP_KERNEL);
   

   This comment is now probably outdated.

4. +++ b/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php
   @@ -22,17 +21,17 @@ class FinishResponseSubscriber implements EventSubscriberInterface {
   -   * @var LanguageManager
   +   * @var \Drupal\Core\Language\LanguageManagerInterface
   

   Just an FYI: splitting these sorts of changes into separate issues would make this patch a bit smaller and easier to review.

5. +++ b/core/lib/Drupal/Core/PageCache/CacheControl/PublicExternal.php
   @@ -0,0 +1,136 @@
   +   * Whether or not "Vary: Cookie" should be added to the response.
   

   'a "Vary: Cookie" header should be"

6. +++ b/core/lib/Drupal/Core/PageCache/CacheControl/PublicExternal.php
   @@ -0,0 +1,136 @@
   +  public function setCacheable(Response $response, Request $request) {
   +    // Disable caching in ancient browsers and for HTTP/1.0 proxies and clients.
   +    if (!$response->headers->has('Expires')) {
   +      $this->setExpiresNoCache($response);
   +    }
   

   An override for setCacheable(), yet disabling caching in ancient browsers. Why?

7. +++ b/core/lib/Drupal/Core/PageCache/CacheControl/PublicExternal.php
   @@ -0,0 +1,136 @@
   +    $max_age = !$request->cookies->has(session_name()) || $vary_customized ? $this->maxAge : 0;
   +    $response->setMaxAge($max_age);
   

   max-age should be limited to 1 year in case it's greater than one year, according to Google PageSpeed's docs, which say that's according to the RFC.

   It's known that (enormously) huge values confuse browsers, so better to be safe than sorry.

8. +++ b/core/lib/Drupal/Core/PageCache/CacheControl/PublicExternal.php
   @@ -0,0 +1,136 @@
   +    // Entity tag should change if the output changes.
   

   s/Entity tag/ETag/

9. +++ b/core/lib/Drupal/Core/PageCache/CacheControlBase.php
   @@ -0,0 +1,50 @@
   + * Defines an interface for Cache-Control header generators.
   ...
   +abstract class CacheControlBase implements CacheControlInterface {
   

   This is not the interface, but a base class.

10. +++ b/core/lib/Drupal/Core/PageCache/CacheControlBase.php
    @@ -0,0 +1,50 @@
    +    // place. Therefore remove Etag and Last-Modified in that case.
    +    $response->setEtag(NULL);
    +    $response->setLastModified(NULL);
    +    $response->setVary(NULL);
    

    Apparently also Vary.

11. +++ b/core/lib/Drupal/Core/PageCache/CacheControlBase.php
    @@ -0,0 +1,50 @@
    +  protected function setCacheControlNoCache(Response $response) {
    ...
    +  protected function setExpiresNoCache(Response $response) {
    

    I like this separation between HTTP 1.1 and 1.0. Very clear, very explicit. This really helps with long-term maintenance :)

12. +++ b/core/lib/Drupal/Core/PageCache/CacheControlBase.php
    @@ -0,0 +1,50 @@
    +   * HTTP/1.0 proxies does not support the Vary header, so prevent any caching
    

    s/does/do/

13. +++ b/core/lib/Drupal/Core/PageCache/CacheControlDefaultFactory.php
    @@ -0,0 +1,52 @@
    + * A cache control factory configured through config and settings.
    

    Additional comments here explaining which things are configurable through config and which are configurable through settings, would be most welcome here.

14. +++ b/core/lib/Drupal/Core/PageCache/CacheControlInterface.php
    @@ -0,0 +1,36 @@
    + * Defines an interface for Cache-Control header generators.
    

    Im missing docs here that contain a justification to explain why Symfony\Component\HttpFoundation\HeaderBag is insufficient. That also has a addCacheControlDirective() method, plus siblings.

15. +++ b/core/lib/Drupal/Core/PageCache/CacheControlInterface.php
    @@ -0,0 +1,36 @@
    +interface CacheControlInterface {
    +  /**
    ...
    +  public function setNotCacheable(Response $response, Request $request);
    +}
    

    We always have a newline after the opening of a class/interface and a newline before its closing.

    Please fix that here and elsewhere.

16. +++ b/core/lib/Drupal/Core/PageCache/CacheControlInterface.php
    @@ -0,0 +1,36 @@
    +   * Add Cache-Control and Expire header to a cacheable response.
    ...
    +   * Add Cache-Control and Expire header for a response which is not cacheable.
    

    s/Expire/Expires/

17. +++ b/core/lib/Drupal/Core/PageCache/CompoundPolicy.php
    @@ -0,0 +1,93 @@
    + * A policy implementation which can contain any number of rules.
    

    s/A policy/A page cache policy/

    ?

18. +++ b/core/lib/Drupal/Core/PageCache/CompoundPolicyObserverInterface.php
    @@ -0,0 +1,23 @@
    + * Defines the interface for policy rule observers.
    ...
    +interface CompoundPolicyObserverInterface {
    

    Docs don't match.

19. +++ b/core/lib/Drupal/Core/PageCache/CompoundPolicyObserverInterface.php
    @@ -0,0 +1,23 @@
    +   * Verify results produced by applying a set of policy rules.
    ...
    +   *   A list of policy rules applied.
    ...
    +  public function onCompoundPolicyResult(array $rules, $result);
    

    "set" vs. "list" vs. "compound"

    Let's make this consistent.

    Does order matter? -> "list"
    Does order not matter? -> "set"

20. +++ b/core/lib/Drupal/Core/PageCache/DelegatePageCache/CacheControlSelectorInterface.php
    @@ -0,0 +1,26 @@
    + * Defines the interface for storage selector implementations.
    ...
    +interface CacheControlSelectorInterface {
    

    "storage selector" vs. "cache control selector"

    Confusing.

21. +++ b/core/lib/Drupal/Core/PageCache/PageCacheBase.php
    @@ -0,0 +1,183 @@
    +      $this->prepareCachePass($response, $request);
    

    Why "cache pass"? It's always been called "cache hit".

22. +++ b/core/lib/Drupal/Core/PageCache/PolicyRule/DenyCommandLineOrUnsafeMethod.php
    @@ -0,0 +1,40 @@
    +class DenyCommandLineOrUnsafeMethod implements PolicyRuleInterface {
    
    +++ b/core/lib/Drupal/Core/PageCache/PolicyRule/DenyOpenSession.php
    @@ -0,0 +1,28 @@
    +class DenyOpenSession implements PolicyRuleInterface {
    
    +++ b/core/lib/Drupal/Core/PageCache/PolicyRule/InternalCacheConfig.php
    @@ -0,0 +1,42 @@
    +class InternalCacheConfig implements PolicyRuleInterface {
    

    I appreciate the elegance here.

    Maybe rename InternalCacheConfig to RequireInternalCacheConfig for consistency?

23. +++ b/core/lib/Drupal/Core/PageCache/Storage/CidGeneratorInterface.php
    @@ -0,0 +1,26 @@
    +interface CidGeneratorInterface {
    

    Instead of returning a cache ID, you could also opt to return cache keys.

    Ideally, you'd leverage Drupal\Core\CacheableInterface in this patch.

Add cache-control headers before saving pages to the internal cache. This would also simplify delivery of cached pages.

I don't see why that can't happen? :)

Also in this case it would be possible to leverage the cache-control stuff already implemented in the Symfony HeaderBag

Excellent!

storing cached pages on disk instead of in DB

This shouldn't be too much hassle with the current architecture.

Interesting. Would you say that's a valid alternative to circumvent the problems of "before vs. after kernel initialization"?

I am sorry that you actually use matlab :P Impressive work in general.

• Please use units on your y-axes. Assume these are milliseconds?
• Have you made similar measurements for D8 without your patch, like the D7 results?
• Did you also made some xhprof tests? Where are the main parts the code is slower. Is this mostly file loading due to the involved classes?
• It would be great if you would link to your contrib module in the issue summary to explain why you have such many ideas

1. +++ b/core/includes/bootstrap.inc
   @@ -1463,19 +1297,36 @@ function drupal_handle_request($test_only = FALSE) {
   +  if ($page_cache_helper->deliverFromCacheBeforeKernel(settings(), $request)) {
   +    return;
   +  }
   ...
   +  // Try to retrieve the page from the cache.
   +  if ($page_cache_helper->deliverFromCacheAfterContainer($request)) {
   

   What are your long-term goals here would both code paths merge one day, for example in a stacked http kernel? I guess we always need a pre-kernel page cache for performance?

2. +++ b/core/includes/bootstrap.inc
   @@ -1463,19 +1297,36 @@ function drupal_handle_request($test_only = FALSE) {
   -  $response = $kernel->handle($request)->prepare($request)->send();
   +  $response = $kernel->handle($request);
   +
   +  // Save the rendered markup to the page cache and add relevant headers for
   +  // external caches like browsers and proxies if necessary.
   +  $page_cache_helper->recordAndDeliver($response, $request);
    
   

   tbh it is a bit odd that we kind of require the page cache to be part of our main tool chain. I wonder whether we should at least still deliver/send the result in this method.

3. +++ b/core/includes/bootstrap.inc
   @@ -1599,8 +1450,8 @@ function _drupal_bootstrap_configuration() {
   +  # set_error_handler('_drupal_error_handler');
   

   Do you plan to change that?

4. +++ b/core/includes/session.inc
   @@ -253,9 +253,6 @@ function drupal_session_initialize() {
   -    if ($user->isAuthenticated() || !empty($_SESSION)) {
   -      drupal_page_is_cacheable(FALSE);
   -    }
   
   +++ b/core/includes/utility.inc
   @@ -41,16 +41,12 @@ function drupal_rebuild() {
   -  // Disable the page cache.
   -  drupal_page_is_cacheable(FALSE);
   

   These once got coverted to vetos?

5. +++ b/core/lib/Drupal/Core/PageCache/BuildPolicyPass.php
   @@ -0,0 +1,53 @@
   +
   +        $policydef->addMethodCall('deny', array(new Reference($id)));
   

   Have you tried to lazy-load the service references. Would that speed up the performance? We could for example introduce service proxies to achieve this goal.

6. +++ b/core/lib/Drupal/Core/PageCache/BuildPolicyPass.php
   @@ -0,0 +1,53 @@
   +        if (!$reflection_class->implementsInterface('Drupal\Core\PageCache\PolicyRuleInterface')) {
   +          throw new \InvalidArgumentException(sprintf('Service "%s" must implement interface "%s".', $id, $interface));
   +        }
   

   +1 for this DX!

7. +++ b/core/lib/Drupal/Core/PageCache/CacheControlResponseSubscriber.php
   @@ -0,0 +1,89 @@
   + * Definition of Drupal\Core\PageCache\CacheControlResponseSubscriber.
   

   Ensure that all classes use "Contains ..."

8. +++ b/core/lib/Drupal/Core/PageCache/PageCacheBase.php
   @@ -0,0 +1,98 @@
   +/**
   + * Base class for page cache implementations.
   + */
   +class PageCacheBase implements PageCacheInterface {
   

   Is this really a base class?

9. +++ b/core/lib/Drupal/Core/PageCache/Policy.php
   @@ -0,0 +1,106 @@
   +  /**
   +   * Return the singleton policy instance.
   +   *
   +   * If it does not exist, construct a new policy instance and populate it with
   +   * default rules.
   +   *
   +   * @return \Drupal\Core\PageCache\Policy
   +   *   The singleton policy instance.
   +   */
   +  public static function getSingleton() {
   +    if (!isset(static::$instance)) {
   +      $policy = new static();
   +      $policy->deny(new PolicyRule\CommandLineOrUnsafeMethod());
   +      $policy->allow(new PolicyRule\NoSessionOpen());
   +      static::$instance = $policy;
   +    }
   

   Do we really need a singleton here?

Code review only:

1. +++ b/core/lib/Drupal/Core/PageCache/BuildPolicyPass.php
   @@ -0,0 +1,53 @@
   +        $reflection_class = new \ReflectionClass($class);
   +        if (!$reflection_class->implementsInterface('Drupal\Core\PageCache\PolicyRuleInterface')) {
   +          throw new \InvalidArgumentException(sprintf('Service "%s" must implement interface "%s".', $id, $interface));
   +        }
   

   We don't have this added protection on any other tagged services. No need to do it here.

2. +++ b/core/lib/Drupal/Core/PageCache/BuildPolicyPass.php
   @@ -0,0 +1,53 @@
   +        $reflection_class = new \ReflectionClass($class);
   +        if (!$reflection_class->implementsInterface('Drupal\Core\PageCache\PolicyRuleInterface')) {
   +          throw new \InvalidArgumentException(sprintf('Service "%s" must implement interface "%s".', $id, $interface));
   +        }
   

   Ibid.

3. +++ b/core/lib/Drupal/Core/PageCache/FastPageCacheFactoryBase.php
   @@ -0,0 +1,49 @@
   +  /**
   +   * Return the global page cache policy.
   +   *
   +   * @return \Drupal\Core\PageCache\PolicyRuleInterface
   +   *   The global policy to be used in order to test whether caching is allowed.
   +   */
   +  protected function getPolicy(Settings $settings, Request $request) {
   +    return Policy::getSingleton();
   +  }
   

   Wait, what? this patch has a big OO framework to handle cache policy and then tosses in a global singleton? Does not compute.

4. +++ b/core/lib/Drupal/Core/PageCache/PageCacheBase.php
   @@ -0,0 +1,108 @@
   +  public function revalidate(Response $response, Request $request) {
   +    $last_modified = $response->getLastModified();
   +    if (!$last_modified) {
   +      return;
   +    }
   +    $created = $last_modified->getTimestamp();
   +
   +    // See if the client has provided the required HTTP headers.
   +    $if_modified_since = $request->server->has('HTTP_IF_MODIFIED_SINCE') ? strtotime($request->server->get('HTTP_IF_MODIFIED_SINCE')) : FALSE;
   +    $if_none_match = $request->server->has('HTTP_IF_NONE_MATCH') ? stripslashes($request->server->get('HTTP_IF_NONE_MATCH')) : FALSE;
   +
   +    if ($if_modified_since && $if_none_match
   +      && $if_none_match == $response->getEtag() // etag must match
   +      && $if_modified_since == $created) {  // if-modified-since must match
   +      $response->setStatusCode(304);
   +      $response->setContent(null);
   +
   +      // In the case of a 304 response, certain headers must be sent, and the
   +      // remaining may not (see RFC 2616, section 10.3.5).
   +      foreach (array_keys($response->headers->all()) as $name) {
   +        if (!in_array($name, array('content-location', 'expires', 'cache-control', 'vary'))) {
   +          $response->headers->remove($name);
   +        }
   +      }
   +    }
   +  }
   

   The response class already does most if not all of this in the prepare() method.

5. +++ b/core/lib/Drupal/Core/PageCache/PolicyRule/CommandLineOrUnsafeMethod.php
   @@ -0,0 +1,37 @@
   +class CommandLineOrUnsafeMethod implements PolicyRuleInterface {
   

   The name of the class says explicitly that the class is doing too much.

znerol: I appreciate the work you're putting into this issue. Really. But I can't help but think that this is all horribly over-engineered.

1) The baseline for page caching in 2014 is a reverse proxy. We should assume that anyone "serious" is using a reverse proxy (Varnish, nginx, etc.), and our PHP-space page caching is simply for those people who don't have one (but should).

2) That means we *should not* offer more functionality and flexibility than can be represented by HTTP itself, because that's all the proxy will see.

3) That means, effectively, writing a PHP reverse proxy.

4) We already have a PHP reverse proxy in core. It's called HttpCache, and is linked from the issue summary. :-)

5) That issue was bumped to Drupal 9 in June of 2013 on the grounds that Simpletest and the installer prevented us from using a wrapped Kernel, which is what HttpCache is. However, the work you're doing here is all pre-kernel... which means it could just as easily wrap the kernel. Meanwhile, Larowlan has made huge progress on #2016629: Refactor bootstrap to better utilize the kernel. (larwolan++!) It may be we can actually pull that off after all.

6) All of that points to saying that we should revisit HttpCache, as it is *existing code that does what we need* that we're already even shipping, and it would be less work to use than writing our own mini-framework. In fact, the Response object already tracks nearly all of the data we need, and as noted above does the necessary header mangling for various response statuses already. That in turn means that all we need are a few response listeners to set the headers needed, and maybe a service or two to act as an API to the rest of the system, maybe. (Although for partial page caching purposes we really want to make caching automatic when it is practical rather than relying on module developers to get it right.)

That is... I am really not at all convinced this is close to the right approach for us to take.

+++ b/core/lib/Drupal/Core/PageCache/PageCacheFactoryInterface.php
@@ -0,0 +1,28 @@
+interface PageCacheFactoryInterface {
+
+  /**
+   * Construct an appropriate page cache implementation for the given settings.
+   *
+   * @param \Drupal\Core\DrupalKernelInterface $kernel
+   *   The drual kernel instance to wrap.
+   * @param \Drupal\Component\Utility\Settings $settings
+   *   The settings instance.
+   */
+  public static function createFromSettings(DrupalKernelInterface $kernel, Settings $settings);
+
+}

There's no reason for this to be static. A for-reals object should be the default. If you want a static single-method factory, don't bother with a separate class for that.

That said, holy crap you managed to wrap another HttpKernelInterface around DrupalKernel! :-) Please humor me... Can you do that with HttpCache? It's *already there*, so if we can use it directly we should. (With all the usual caveats of "you really should use varnish instead of all of this crap.")

The architecture looks straight forward to me. thanks for the great issue summary @znerol

Tbh, i think i agree with Crell here, but i havent check HttpCache much yet, so i wont comment on that yet

Here is a code review. There are docs missing in several places but i show the @todos so i didnt bother

1. +++ b/core/lib/Drupal/Core/PageCache/BuildPolicyPass.php
   @@ -0,0 +1,53 @@
   +   * Implements CompilerPassInterface::process().
   
   +++ b/core/lib/Drupal/Core/PageCache/BuildVetoPass.php
   @@ -0,0 +1,41 @@
   +   * Implements CompilerPassInterface::process().
   

   should be {@inheritdoc}

2. +++ b/core/lib/Drupal/Core/PageCache/CacheControlResponseSubscriberBase.php
   @@ -0,0 +1,193 @@
   +  public function setMaxAge($max_age) {
   +    $this->maxAge;
   

   you probably forgot to assign the passed variable to the property

3. +++ b/core/lib/Drupal/Core/PageCache/PageCacheBase.php
   @@ -0,0 +1,188 @@
   +    $this->status &= $this->getPolicy()->apply($request);
   ...
   +    $this->status &= !$this->getVeto()->apply($response, $request);
   

   It is not clear to me as to why the reference is needed here?

4. +++ b/core/lib/Drupal/Core/PageCache/PolicyRule/CommandLineOrUnsafeMethod.php
   @@ -0,0 +1,37 @@
   +  protected function isCLIRequest(Request $request) {
   

   per our coding standards it should be isCliRequest

oh, thanks, sorry, i am blind, totally misread this

HttpCache can wrap anything that implements HttpKernelInterface; that includes HttpKernel, Symfony's KernelInterface, or DrupalKernel. That's the point.

HttpKernel subrequests never leave HttpKernel, so the wrapper (or lack thereof) doesn't matter. ESI-esque requests, from the POV of the DrupalKernel, are the same regardless of whether they come from a wrapping kernel or from Varnish/nginx. Remember, the goal is that midrange to high-level sites will be using a real reverse proxy anyway, so using only what HTTP supports; we just need a fallback for sites that don't have that option available.

Awesome work! This is significantly simpler already, which was the major concern for several people in earlier iterations AFAICT.

I think the policy vs. veto concept is almost sufficiently clear. Almost, becasue maybe the naming isn't: AFAICT policies are for requests and vetoes are for responses? If this is the case, this should be made much more explicit.

Then there is PageCache vs. FastPageCache. The differences there also aren't clear enough. AFAICT FastPageCache is for the pre-kernel case, and PageCache for the post-kernel case?

1. +++ b/core/lib/Drupal/Core/PageCache/CacheControlResponseSubscriber.php
   @@ -0,0 +1,91 @@
   +class CacheControlResponseSubscriber extends CacheControlResponseSubscriberBase {
   

   This response subscriber contains "default Drupal assumptions". Maybe we should rename it to DefaultCacheControlResponseScruber?

2. +++ b/core/lib/Drupal/Core/PageCache/CacheControlResponseSubscriber.php
   @@ -0,0 +1,91 @@
   +    // @todo: Let's get rid of that mechanism and instead instruct site
   +    // builders and module authors to swap out
   +    // page_cache.cache_control_response_subscriber when custom Vary headers
   +    // are required.
   

   Agreed! I didn't even know we supported this right now.

3. +++ b/core/lib/Drupal/Core/PageCache/CacheControlResponseSubscriber.php
   @@ -0,0 +1,91 @@
   +    $max_age = !$request->cookies->has(session_name()) || $vary_customized ? $this->getMaxAge() : 0;
   

   Can we have parentheses here to remove any ambiguity? :)

4. +++ b/core/lib/Drupal/Core/PageCache/CacheControlResponseSubscriberBase.php
   @@ -0,0 +1,193 @@
   +   * Do not specify a value greater than one year (31536000 seconds).
   

   Hurray! :) Maybe refer to the RFC, to explain why there is this seemingly arbitrary limit?

5. +++ b/core/lib/Drupal/Core/PageCache/FastPageCache.php
   @@ -0,0 +1,55 @@
   +class FastPageCache extends PageCacheBase {
   
   +++ b/core/lib/Drupal/Core/PageCache/PageCache.php
   @@ -0,0 +1,43 @@
   +class PageCache extends PageCacheBase {
   

   The docs for these are identical, though they serve very different purposes. The differences should be more explicit.

6. +++ b/core/lib/Drupal/Core/PageCache/PageCacheBase.php
   @@ -0,0 +1,194 @@
   +    // The RequestCloseSubscriber currently breaks on lean requests because it
   

   What are "lean requests"?

7. +++ b/core/lib/Drupal/Core/PageCache/PageCacheBase.php
   @@ -0,0 +1,194 @@
   +    if ($if_modified_since && $if_none_match
   +      && $if_none_match == $response->getEtag() // etag must match
   +      && $if_modified_since == $created) {  // if-modified-since must match
   

   This implies a 304 will only be sent if both an If-Modified-Since and If-None-Match are sent. That seems wrong.

8. +++ b/core/lib/Drupal/Core/PageCache/Policy.php
   @@ -0,0 +1,94 @@
   +  public static function createDefault() {
   

   It feels weird/wrong to have this (useful) helper hardcoded in here. I think a DefaultPolicy extends Policy might be cleaner?

9. +++ b/core/lib/Drupal/Core/PageCache/Storage/CidGenerator.php
   @@ -0,0 +1,46 @@
   +class CidGenerator implements CidGeneratorInterface {
   

   Can you explain the value of having the page cache ID generator in a separate interface and class? Why not make it part of the thing that uses it?

10. +++ b/core/modules/toolbar/lib/Drupal/toolbar/Controller/ToolbarController.php
    @@ -23,10 +23,22 @@ class ToolbarController extends ControllerBase {
    +    $max_age = 365 * 24 * 60;
    

    This is the number of hours in a year, not the number of seconds :)

    But this was apparently already the case in the old code!

(Apologies if the review is off, I'm still wrecked from Drupal Dev Days Szeged.)

Minor tag cleanup - please ignore.

Lots of progress has been made on this meta issue #2257695: [meta] Modernize the page cache - should this one still be marked postponed?

related: #2549041: Remove deprecated _drupal_set_preferred_header_name()

Ugh. Added to the wrong issue. But still related. :-)