Follow-up for #2429617: small improvements

Quoting @effulgentsia in #2429617-425: Make D8 2x as fast: Dynamic Page Cache: context-dependent page caching (for *all* users!):

I read through the patch again, and am about to commit it shortly. Here's some feedback for follow-up material though.

1. +++ b/core/modules/dynamic_page_cache/dynamic_page_cache.module
   @@ -0,0 +1,27 @@
   +      $output .= '<dd>' . t('Pages are cached the first time they are requested if they are suitable from caching, then the cached version is served for all later requests. Dynamic content is handled automatically so that both cache correctness and hit ratio is maintained.') . '</dd>';
   

   Something is grammatically (and maybe semantically) wrong with that first sentence.

2. +++ b/core/modules/dynamic_page_cache/dynamic_page_cache.services.yml
   @@ -0,0 +1,29 @@
   +  cache.dynamic_page_cache:
   +    class: Drupal\Core\Cache\CacheBackendInterface
   +    tags:
   +      - { name: cache.bin }
   +    factory: cache_factory:get
   +    arguments: [dynamic_page_cache]
   

   I think page_cache and dynamic_page_cache should use the same cache bin.

3. +++ b/core/modules/dynamic_page_cache/src/EventSubscriber/DynamicPageCacheSubscriber.php
   @@ -0,0 +1,328 @@
   + * Dynamic Page Cache is able to cache so much because it exploits cache
   

   "exploits" sounds like something bad and security related. Let's find another word.

4. +++ b/core/modules/dynamic_page_cache/src/EventSubscriber/DynamicPageCacheSubscriber.php
   @@ -0,0 +1,328 @@
   + * RESPONSE subscriber for cache misses) is because many cache contexts can only
   + * be evaluated after routing. (Examples: 'user', 'user.permissions', 'route' …)
   + * Consequently, it is impossible to implement Dynamic Page Cache as a kernel
   + * middleware that simply caches per URL.
   

   I think Fabianx had comments somewhere on this thread to change from caching by 'route' to caching by 'url'. Also, I wonder if 'user' and user.* could be determined without routing (if we can either deal with or ignore the dependencies of authentication providers on routes). In which case, we can get this into an earlier running REQUEST priority and maybe eventually into a middleware.

5. +++ b/core/modules/dynamic_page_cache/src/EventSubscriber/DynamicPageCacheSubscriber.php
   @@ -0,0 +1,328 @@
   +    // access and modify the cacheability metadata associated with the response.)
   

   Exceeds 80 characters.

6. +++ b/core/modules/dynamic_page_cache/src/EventSubscriber/DynamicPageCacheSubscriber.php
   @@ -0,0 +1,328 @@
   +   * of the placeholdered content does not bubble up to the response level. ¶
   

   Trailing space.

7. +++ b/core/modules/dynamic_page_cache/src/EventSubscriber/DynamicPageCacheSubscriber.php
   @@ -0,0 +1,328 @@
   +    // Run after AuthenticationSubscriber (necessary for the 'user' cache
   +    // context) and MaintenanceModeSubscriber (Dynamic Page Cache should not be
   +    // polluted by maintenance mode-specific behavior), but before
   +    // ContentControllerSubscriber (updates _controller, but that is a no-op
   +    // when Dynamic Page Cache runs).
   +    $events[KernelEvents::REQUEST][] = ['onRouteMatch', 27];
   

   AuthenticationSubscriber has 2 request listeners, so this comment is ambiguous as to whether/why it needs to run after the 2nd one. Also, can maintenance mode be dealt with via its already existing page cache kill switch instead of requiring this to run later?

8. +++ b/core/modules/dynamic_page_cache/tests/dynamic_page_cache_test/src/DynamicPageCacheTestController.php
   @@ -0,0 +1,139 @@
   +      '#markup' => SafeMarkup::format('Hello there, %animal.', ['%animal' => \Drupal::requestStack()->getCurrentRequest()->query->get('animal')]),
   

   Any reason not to receive $request as a controller parameter instead?

9. +++ b/core/modules/dynamic_page_cache/tests/dynamic_page_cache_test/src/DynamicPageCacheTestController.php
   @@ -0,0 +1,139 @@
   +      '#markup' => 'Drupal cannot handle the awesomeness of llamas.',
   +      '#cache' => [
   +        'contexts' => [
   +          'user',
   +        ],
   +      ],
   

   Let's either put something user-specific into the markup or comment why we aren't (i.e., if it's to test this independently of #2558599: Automatically assign user cache contexts/tags when using current_user service or some other important reason to remind future people reading this code of).

10. +++ b/core/modules/dynamic_page_cache/tests/dynamic_page_cache_test/src/DynamicPageCacheTestController.php
    @@ -0,0 +1,139 @@
    +   * A route returning a render array (with max-age=0, so uncacheable)
    +  public function htmlUncacheableTags() {
    

    Wrong comment for this function.

11. +++ b/core/modules/system/src/Tests/Session/SessionTest.php
    @@ -153,6 +153,11 @@ public function testSessionPersistenceOnLogin() {
    +    // Disable the dynamic_page_cache module; it'd cause session_test's debug
    +    // output (that is added in
    +    // SessionTestSubscriber::onKernelResponseSessionTest()) to not be added.
    +    $this->container->get('module_installer')->uninstall(['dynamic_page_cache']);
    

    Shouldn't we have some test coverage for this with the module enabled as well?

1. Ok, suggestions? This comes directly from @catch, a native speaker :)
2. I think it's better if they're in separate cache bins. But surely it's better to have them use the same cache bin rather than having the page cache use the render cache bin.
3. What about leverages?
4. See #2429617-219: Make D8 2x as fast: Dynamic Page Cache: context-dependent page caching (for *all* users!), #223, #224, #285, #287, #288 and the issue that was created for it: #2541284: Investigate moving Dynamic Page Cache into a middleware at the cost of doing authentication manually if needed.
5. Ok, will fix.
6. Ok, will fix.
7. We should indeed make the comment regarding AuthenticatioSubscriber more specific. About maintenance mode: we want to run after maintenance mode because if we run before, then we'll end up returning Dynamic Page Cache's cached responses instead of maintenance mode's "hey, the site is in maintenance" page (\Drupal\Core\EventSubscriber\MaintenanceModeSubscriber::onKernelRequestMaintenance() sets a response).
8. I forgot that that was possible. Yes, let's do that.
9. Ok, will fix.
10. Indeed, will fix.
11. See the comment, and read the code of the debug-output-adding-subscriber-just-for-that-test: then I think you'll also reach the conclusion that it's impossible. It's also besides the point of the test IMO.

Addressed everything except point 2.

+++ b/core/modules/dynamic_page_cache/tests/dynamic_page_cache_test/src/DynamicPageCacheTestController.php
@@ -109,7 +109,7 @@ public function htmlUncacheableMaxAge() {
+      '#markup' => t('@username cannot handle the awesomeness of llamas.', ['@username' => \Drupal::currentUser()->getDisplayName()]),

Let's use StringTranslationTrait;, so that this can call $this->t() instead.

You forgot the actual patch :P

Thanks!