Cache tag the page cache

Yes yes yes yes yes +1000.

This turned out to be pretty straightforward. We only have access to the processed page array during drupal_render_page() so we have to calculate and stash away our list of tags at that time. I chose to add a #post_render callback at the top level of the $page array and do collection and storage of tags there. We then fetch those collected tags later during drupal_page_set_cache().

We were not correctly tagging nodes with their author's uid so I fixed that line in this patch. It was giving an error.

Better code comments.

#4: drupal8.base-system.2094241-4.patch queued for re-testing.

Fixing nitpicks.

1. +++ b/core/includes/common.inc
   @@ -4455,6 +4455,38 @@ function drupal_render_collect_cache_tags($element, $tags = array()) {
   +    $tags = &drupal_static('system_cache_tags_page', FALSE);
   

   Shouldn't we default to an array here instead of false? Just like in drupal_cache_tags_page_get().

2. +++ b/core/modules/node/lib/Drupal/node/NodeRenderController.php
   @@ -88,7 +88,7 @@ protected function alterBuild(array &$build, EntityInterface $entity, EntityDisp
   +    $build['#cache']['tags']['user'][] = $entity->uid->value;
   

   Since $entity is actually a class that implements NodeInterface, we should use $entity->getAuthorId().

My review points are trivial, I think this is ready to go.

Sorry, to un-RTBC, but this still needs tests.

My view on testing was that we don't need them at the moment because there's nothing to break here, we either get some cache tags from a render array or we don't.

Now, if we want to go further and remove the 'content' cache tag, that will definitely need testing.

Thanks for the feedback and rerolls. Here is a new patch with a test.

Reroll

Actually this one

We are green now, with a proper web test. Anyone care to review and RTBC?

1. +++ b/core/modules/system/lib/Drupal/system/Tests/Bootstrap/PageCacheTest.php
   @@ -42,6 +42,29 @@ function setUp() {
   +   * Assure that cache tags are properly persisted.
   

   Really minor point :) "Tests that cache tags ..."

2. +++ b/core/modules/system/lib/Drupal/system/Tests/Bootstrap/PageCacheTest.php
   @@ -42,6 +42,29 @@ function setUp() {
   +    cache_invalidate_tags($tags);
   

   This procedural wrapper is marked as deprecated, we should use Cache::invalidateTags() directly instead.

3. +++ b/core/modules/system/tests/modules/test_page_test/test_page_test.module
   @@ -22,5 +22,6 @@ function test_page_test_page() {
   +    '#cache' => array('tags' => array('test-page' => TRUE)),
   

   This new tag doesn't seem to be used or invalidated anywhere, shouldn't we remove it?

Good catches. Fixed all those.

Looks good now :)

Yep, looks good :)