Improve comments on PlaceholderGeneratorInterface::shouldAutomaticallyPlaceholder() and DynamicPageCacheSubscriber::shouldCacheResponse()

Assuming #2429617: Make D8 2x as fast: Dynamic Page Cache: context-dependent page caching (for *all* users!) lands, this is what the title should be. We'll need to make the same fixes in both places.

1. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -661,12 +661,20 @@ protected function shouldAutomaticallyPlaceholder(array $element) {
   +    // Auto-placeholder if a cache context is set that is set in the
   ...
   +    // Auto-placeholder if a cache tag is set that is set in the
   

   Agreed with #9 that this is very confusing.

   Note that this has now moved to \Drupal\Core\Render\PlaceholderGeneratorInterface::shouldAutomaticallyPlaceholder(). I think it's better to document it on that interface now, and not in the actual implementation.

2. +++ b/core/modules/dynamic_page_cache/src/EventSubscriber/DynamicPageCacheSubscriber.php
   @@ -221,7 +221,7 @@ public function onResponse(FilterResponseEvent $event) {
   @@ -241,12 +241,20 @@ protected function shouldCacheResponse(CacheableResponseInterface $response) {
   

   I'm not sure I understand why the comments in here need to be updated.

   Why isn't \Drupal\dynamic_page_cache\EventSubscriber\DynamicPageCacheSubscriber::shouldCacheResponse()'s documentation sufficient?

   And perhaps we can make DynamicPageCacheSubscriber::shouldCacheResponse() call PlaceholderGeneratorInterface:;shouldAutomaticallyPlaceholder()? And move the docs from the former to the latter, and improve it. Then all docs and all logic live in a single canonical place. That seems better :)

1. +++ b/core/lib/Drupal/Core/Render/PlaceholderGenerator.php
   @@ -56,12 +56,20 @@ public function shouldAutomaticallyPlaceholder(array $element) {
   -    // Auto-placeholder if a high-cardinality cache context is set.
   +    // Auto-placeholder if a cache context is in the
   +    // renderer.config.auto_placeholder_conditions.contexts configuration array.
   +    // These are cache contexts with high cardinality that are deemed to have
   +    // such high cardinality that there would be too much pollution in the
   +    // configured cache-bin.
        if (isset($element['#cache']['contexts']) && array_intersect($element['#cache']['contexts'], $conditions['contexts'])) {
          return TRUE;
        }
    
   -    // Auto-placeholder if a high-invalidation frequency cache tag is set.
   +    // Auto-placeholder if a cache context is in the
   +    // renderer.config.auto_placeholder_conditions.tags configuration array.
   +    // These are cache tags with high cardinality that are deemed to have
   +    // such high invalidation rate that there would be too much cache
   +    // invalidation for the configured cache-bin.
   

   I think #10.1 still applies here: I think it's better to document it on that interface now, and not in the implementation.

   i.e. move basically this into the second hunk?

2. +++ b/core/lib/Drupal/Core/Render/PlaceholderGeneratorInterface.php
   @@ -35,6 +35,10 @@ public function canCreatePlaceholder(array $element);
   +   * Render array should be placeholdered if its cacheable metadata would create
   

   s/cacheable/cacheability/

#13:

I already explained #12.1?

Also, your change causes it to go beyond 80 characters; those lines need to be reflowed.

1. +++ b/core/lib/Drupal/Core/Render/PlaceholderGenerator.php
   @@ -56,12 +56,20 @@ public function shouldAutomaticallyPlaceholder(array $element) {
   +    // These are cache contexts with high cardinality that are deemed to have
   

   s/with high cardinality/

2. +++ b/core/lib/Drupal/Core/Render/PlaceholderGenerator.php
   @@ -56,12 +56,20 @@ public function shouldAutomaticallyPlaceholder(array $element) {
   +    // such high cardinality that there would be too much pollution in the
   

   s/such high/such a high/

3. +++ b/core/lib/Drupal/Core/Render/PlaceholderGenerator.php
   @@ -56,12 +56,20 @@ public function shouldAutomaticallyPlaceholder(array $element) {
   +    // configured cache-bin.
   

   s/configured cache-bin/cache bin that would make caching ineffective/

4. +++ b/core/lib/Drupal/Core/Render/PlaceholderGenerator.php
   @@ -56,12 +56,20 @@ public function shouldAutomaticallyPlaceholder(array $element) {
   +    // These are cache tags with high cardinality that are deemed to have
   

   s/with high cardinality//

5. +++ b/core/lib/Drupal/Core/Render/PlaceholderGenerator.php
   @@ -56,12 +56,20 @@ public function shouldAutomaticallyPlaceholder(array $element) {
   +    // such high invalidation rate that there would be too much cache
   

   s/such high/such a high/

6. +++ b/core/lib/Drupal/Core/Render/PlaceholderGenerator.php
   @@ -56,12 +56,20 @@ public function shouldAutomaticallyPlaceholder(array $element) {
   +    // invalidation for the configured cache-bin.
   

   s/configured cache-bin/cache bin that would make caching ineffective/

7. +++ b/core/lib/Drupal/Core/Render/PlaceholderGeneratorInterface.php
   @@ -35,6 +35,10 @@ public function canCreatePlaceholder(array $element);
   +   * Render array should be placeholdered if its cacheability metadata would create
   

   80 cols violation.

   s/Render array/The render array/

   s/metadata//

   s/create/have/

8. +++ b/core/lib/Drupal/Core/Render/PlaceholderGeneratorInterface.php
   @@ -35,6 +35,10 @@ public function canCreatePlaceholder(array $element);
   +   * either too high cardinality or too high invalidation rate in the configured
   +   * cache-bin.
   

   s/in the configured cache-bin//

1. +++ b/core/lib/Drupal/Core/Render/PlaceholderGenerator.php
   @@ -56,12 +56,20 @@ public function shouldAutomaticallyPlaceholder(array $element) {
   +    // These are cache contexts that are deemed to have
   +    // such a high cardinality that there would be too much pollution in the
   +    // cache bin that would make caching ineffective.
   

   80 cols.

2. +++ b/core/lib/Drupal/Core/Render/PlaceholderGenerator.php
   @@ -56,12 +56,20 @@ public function shouldAutomaticallyPlaceholder(array $element) {
   +    // These are cache tags that are deemed to have
   +    // such a high invalidation rate that there would be too much cache
   +    // invalidation for the cache bin that would make caching ineffective.
   

   80 cols.

3. +++ b/core/lib/Drupal/Core/Render/PlaceholderGeneratorInterface.php
   @@ -35,6 +35,9 @@ public function canCreatePlaceholder(array $element);
   +   * The render array should be placeholdered if its cacheability would
   +   * have either too high cardinality or too high invalidation rate.
   

   80 cols.

#21: I didn't mean it exceeded 80 characters, I meant it didn't *use* 80 characters: there is lots of bizarre whitespace left open.

Reroll attached that I think is much simpler and addresses all concerns raised here.

Thoughts?

EDIT: crosspost.

#26:

+++ b/core/lib/Drupal/Core/Render/PlaceholderGeneratorInterface.php
@@ -35,6 +35,9 @@ public function canCreatePlaceholder(array $element);
+   * The render array should be placeholdered if its cacheability would* have

Stray asterisk there.

#28: Care to RTBC then? :)

BTW: also finished the handbook page at https://www.drupal.org/developing/api/8/render/arrays/cacheability/auto-...