Move the remaining procedural page cache code to the page cache stack middleware

Middlewar sounds pretty cool :)

This was blocked on #2347877: Move DrupalKernel::initializeCookieGlobals() into a SessionConfiguration service, which has been committed yesterday!

Test fails look like there's an issue with serializing non-serializable responses, like files, among other things.

+++ b/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php
@@ -136,17 +136,6 @@ public function onRespond(FilterResponseEvent $event) {
-
-    // Currently it is not possible to cache some types of responses. Therefore
-    // exclude binary file responses (generated files, e.g. images with image
-    // styles) and streamed responses (files directly read from the disk).
-    // see: https://github.com/symfony/symfony/issues/9128#issuecomment-25088678
-    if ($is_cacheable && $this->config->get('cache.page.use_internal') && !($response instanceof BinaryFileResponse) && !($response instanceof StreamedResponse)) {
-      // Store the response in the internal page cache.
-      drupal_page_set_cache($response, $request);
-      $response->headers->set('X-Drupal-Cache', 'MISS');
-      drupal_serve_page_from_cache($response, $request);
-    }

I assume we somehow lost this part?

1. +++ b/core/lib/Drupal/Core/StackMiddleware/PageCache.php
   @@ -24,32 +31,241 @@ class PageCache implements HttpKernelInterface {
   +  protected function lookup(Request $request, $catch) {
   +    $cache = $this->cache->get(drupal_page_cache_get_cid($request));
   +    if ($cache) {
   

   Can we move drupal_page_cache_get_cid() into this as well (it is not used anywhere else I think) or at least wrap it in a method that is easy to override?

   Goal: Solve #2062463: allow page cache cid to be alterable by making it easy to subclass this and replace the service.

2. +++ b/core/lib/Drupal/Core/StackMiddleware/PageCache.php
   @@ -24,32 +31,241 @@ class PageCache implements HttpKernelInterface {
   +    $cid = drupal_page_cache_get_cid($request);
   +    $tags = explode(' ', $response->headers->get('X-Drupal-Cache-Tags'));
   +    $this->cache->set($cid, $response, $expire, $tags);
   

   Same here, move this part into a helper method, like writeInternalCache() would make it easier to explore some ideas I had with having a quickly expiring edge cache + a long-living page cache that uses cache tags for invalidation.

+++ b/core/lib/Drupal/Core/StackMiddleware/PageCache.php
@@ -24,32 +31,241 @@ class PageCache implements HttpKernelInterface {
+    // Currently it is not possible to cache some types of responses. Therefore
+    // exclude binary file responses (generated files, e.g. images with image
+    // styles) and streamed responses (files directly read from the disk).
+    // see: https://github.com/symfony/symfony/issues/9128#issuecomment-25088678
+    if ($response instanceof BinaryFileResponse || $response instanceof StreamedResponse) {
+      return $response;
+    }

Extending on #9, I think we are missing the use for those two interfaces?

Can we get an interdiff? :)

+++ b/core/lib/Drupal/Core/StackMiddleware/PageCache.php
@@ -268,4 +271,22 @@ protected function fetch(Request $request, $catch = TRUE) {
+   *   The cache key for this request.
...
+  protected function getCacheKey(Request $request) {

This is a misnomer; we call the parts that make up a cache ID "cache keys", but this returns the "cache ID", not "cache keys".

I bet it's the fact that this can easily be looked at as a key/value store type thing that made you think of this name, that'd make perfect sense in that context? :)

Other than that, this patch looks awesome! I think we want some profiling here to ensure we don't make things slower? But IIRC that already happened somewhere else, before #2257695: [meta] Modernize the page cache got split up into many child issues? Since so much time has passed, we probably want to do new profiling anyway though?

It would be pretty slick if we could stop including the page cache middleware, if its not configured to be executed.

Possibly, but that would require a container rebuild when you change the setting.

That said, *if* we would that, we could drop the settings check there *and* the weird setting, because we would always check if enabled, so that might be quite interesting for performance. So maybe a follow-up to investigate that?

Wow, that sounds fascinating!

dawehner++
Berdir++

Really great work!

@znerol
What do you think about the follow up to pass the configuration on compiler dump time?

1. +++ b/core/lib/Drupal/Core/StackMiddleware/PageCache.php
   @@ -24,32 +32,273 @@ class PageCache implements HttpKernelInterface {
   +  protected function pass(Request $request, $type = self::MASTER_REQUEST, $catch = TRUE) {
   +    return $this->forward($request, $type, $catch);
   +  }
   

   Why do we not just call ->forward in places we call pass?

2. +++ b/core/lib/Drupal/Core/StackMiddleware/PageCache.php
   @@ -24,32 +32,273 @@ class PageCache implements HttpKernelInterface {
   +      \Drupal::service('content_negotiation')->getContentType($request),
   

   Is there a reason we don't inject it atm. already?

forward() is also called from within fetch(). Inlining pass() will result in a situation where a subclass would be incapable of telling the reason for a call to kernel handle().

Okay, that is fair!

One thing which changed now, is that we inject the configFactory, so even if you have set Settings::get('page_cache_without_database'
the db is initialialized, additional to all other dependencies of the config manager. Not sure whether we should deal with it, given the idea of #17/#18
which would solve that problem again.

That's a very good point. Maybe we should not inject the config factory but add a @todo on a follow-up for the dynamic service definition idea? Definitely needs to be profiled.

I think we have an open issue for that already somewhere, not quite sure, however.

Leaving at NR; just nitpicks for the fix of #16 :)

1. +++ b/core/lib/Drupal/Core/StackMiddleware/PageCache.php
   @@ -24,32 +32,332 @@ class PageCache implements HttpKernelInterface {
   +   * Gets the page cache id for this request.
   

   s/id/ID/

2. +++ b/core/lib/Drupal/Core/StackMiddleware/PageCache.php
   @@ -24,32 +32,332 @@ class PageCache implements HttpKernelInterface {
   +   *   The cache key for this request.
   

   s/key/ID/

I think the tiny regression is acceptable […]

Do we understand why there is a regression?

[…] given it is well within the margin of error (= standard deviation multiplied by 2).

That's fair, but if it's consistently reproducible to have a lower throughput, then that argument would be less applicable.

Also note that the standard deviation is lower than that of HEAD, which is good. Can we also consistently reproduce that lower standard deviation? If we can, that'd be another reason in favor of the patch.

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -529,7 +538,7 @@ public function getServiceProviders($origin) {
   public function terminate(Request $request, Response $response) {
-    if (FALSE === $this->booted) {
+    if (FALSE === $this->prepared) {
       return;

Can we add a comment here why we are doing this?

With that I think this is ready.

Will try to do something profiling on this as well.

+1 for the wrapping of \Drupal::config() for now.

@znerol
I think one approach we could indeed do is to use config to fill the compression setting, or lets be honest, this is not really something you need to configure through the UI IMHO.

lets be honest, this is not really something you need to configure through the UI IMHO

+1

#37: awesome!

#38: I'd hope #1973618: DIC: Lazy instantiation of service dependencies (ProxyManager for "proxy services") helps with that? :)

re: #37, why was that not being called prior to this patch? Did we add the code using this service to improve performance characteristics in some way? I don't see a discussion of it in this thread.

Being as its sort of hard to pick them out, are there any other code changes that should be noted that aren't just moving code around that we could document in the summary?

Yeah, I understood that injecting the service was causing this regression. I should have been more clear though. Injecting the services lead to the regression but if that code was being used previously it should have had similar characteristics that made me ask why we started injecting it. So I found the code calling this service which is just this one block of code.

+++ b/core/lib/Drupal/Core/StackMiddleware/PageCache.php
@@ -24,32 +32,332 @@ class PageCache implements HttpKernelInterface {
+    if ($this->responsePolicy->check($response, $request) === ResponsePolicyInterface::DENY) {
+      return $response;
+    }
+

This code did not exist previously so why was it added?

Also made me curious if there was any other code that was added to deal with the code being called differently or something.

Draft CR looks good.

Difference between HEAD and patch with lazy loaded response policy in terms of function calls is 26.

Do you see any way to reduce that increase, or even turn it around into a decrease?

Re: my other questions. I see now how the combination of the request policy check in handle() and the response policy check roughly match the logic in FinalResponseSubscriber.

+++ b/core/lib/Drupal/Core/StackMiddleware/PageCache.php
@@ -24,32 +32,332 @@ class PageCache implements HttpKernelInterface {
+  /**
+   * Sidesteps the page cache and directly forwards a request to the backend.
+   *
+   * @param \Symfony\Component\HttpFoundation\Request $request
+   *   A request object.
+   * @param int $type
+   *   The type of the request (one of HttpKernelInterface::MASTER_REQUEST or
+   *   HttpKernelInterface::SUB_REQUEST)
+   * @param bool $catch
+   *   Whether to catch exceptions or not
+   *
+   * @returns \Symfony\Component\HttpFoundation\Response $response
+   *   A response object.
+   */
+  protected function pass(Request $request, $type = self::MASTER_REQUEST, $catch = TRUE) {
+    return $this->forward($request, $type, $catch);
+  }
+
...
+  /**
+   * Forwards a request to the http kernel.
+   *
+   * @param \Symfony\Component\HttpFoundation\Request $request
+   *   A request object.
+   * @param int $type
+   *   The type of the request (one of HttpKernelInterface::MASTER_REQUEST or
+   *   HttpKernelInterface::SUB_REQUEST)
+   * @param bool $catch
+   *   Whether to catch exceptions or not
+   *
+   * @returns \Symfony\Component\HttpFoundation\Response $response
+   *   A response object.
+   */
+  protected function forward(Request $request, $type = self::MASTER_REQUEST, $catch = TRUE) {
     return $this->httpKernel->handle($request, $type, $catch);
   }

Looking some more, can we merge these into a single method? They seem to do the same thing.

There doesn't seem to be a distinction in the documentation. The interface is our contract and both essentially document "pass the request though to the backend" so I'm still not clear why we need two. At the very least we need to document why we need two methods and what makes each useful.

That works for me. I don't see anything at this point that's a red flag and this is definitely where we where trying to go with the kernel. If Wim doesn't having anything else I'd support RTBC'ing this.

Hopefully we can start looking at tweaking logic to cut those calls out as follow ups too. Things like #1818628: Use Composer's optimized ClassLoader for Core/Component classes might minimize it at least.

PS - Thanks for putting up my the late questions and confusion! Great work!

+++ b/core/includes/bootstrap.inc
@@ -68,25 +68,18 @@
  * Fourth bootstrap phase: load code for subsystems and modules.

Dang-it I lied. We should fix this.

Repeatingn znerol's test methodology of #31 on my system, to see whether I can reproduce. With one major difference: I'm not using the MockCache module, so I'm testing actual responses, of the actual D8 frontpage. PHP 5.5.11, no XHProf, no XDebug, with OpCache, with DB. $ ab -k -c1 -n1000 http://tres/.

HEAD

Requests per second:    133.66 [#/sec] (mean)

              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:     6    7   0.6      7      11
Waiting:        5    7   0.6      7      10
Total:          6    7   0.6      8      11

patch

Requests per second:    132.05 [#/sec] (mean)

              min  mean[+/-sd] median   max
Connect:        0    0   0.1      0       0
Processing:     6    7   0.6      7      14
Waiting:        5    7   0.6      7      14
Total:          6    8   0.6      8      14

The difference is negligible: the min, mean, sd and median are identical when comparing HEAD with the patch; only in the max is there a significant difference, but since all other numbers are the same, that doesn't really have any noteworthy effect.

Therefore, together with #59: RTBC. Great work, znerol! :)

Found these nitpicks, and fixed them:

1. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -77,6 +77,13 @@ class DrupalKernel implements DrupalKernelInterface, TerminableInterface {
   +   * Whether essential services have been set up properly by preHandle.
   

   Nit: s/preHandle/preHandle()/

2. +++ b/core/lib/Drupal/Core/StackMiddleware/PageCache.php
   @@ -24,32 +32,314 @@ class PageCache implements HttpKernelInterface {
      /**
       * Constructs a ReverseProxyMiddleware object.
       *
       * @param \Symfony\Component\HttpKernel\HttpKernelInterface $http_kernel
       *   The decorated kernel.
   -   * @param \Drupal\Core\DrupalKernelInterface $drupal_kernel
   -   *   The main Drupal kernel.
       */
   -  public function __construct(HttpKernelInterface $http_kernel, DrupalKernelInterface $drupal_kernel) {
   +  public function __construct(HttpKernelInterface $http_kernel, CacheBackendInterface $cache, RequestPolicyInterface $request_policy, ResponsePolicyInterface $response_policy, ContentNegotiation $content_negotiation) {
   

   These docs hadn't been updated accordingly.

+++ b/core/lib/Drupal/Core/StackMiddleware/PageCache.php
@@ -24,32 +32,322 @@ class PageCache implements HttpKernelInterface {
+    $config = $this->config('system.performance');
+    if (extension_loaded('zlib') && !$response->headers->get('Content-Encoding') && $config->get('response.gzip')) {

Let's not do the config load if we don't have too. Yes we probably have already loaded system.performance - but we might not have so

    if (extension_loaded('zlib') && !$response->headers->get('Content-Encoding') && $this->config('system.performance')->get('response.gzip')) {

Oohh, well-spotted, Alex!

Committed 1e985b0 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation.

Follow-up: #2368987: Move internal page caching to a module to avoid relying on config get on runtime