Forum responses don't set cache tags

+++ b/core/modules/forum/src/Controller/ForumController.php
@@ -208,6 +208,9 @@ protected function build($forums, TermInterface $term, $topics = array(), $paren
+        'tags' => \Drupal::entityManager()->getDefinition('node')->getListCacheTags(),

Could you inject this?

ForumController already has this:

  public static function create(ContainerInterface $container) {
    /** @var \Drupal\Core\Entity\EntityManagerInterface $entity_manager */
    $entity_manager = $container->get('entity.manager');
    return new static(
      $container->get('forum_manager'),
      $entity_manager->getStorage('taxonomy_vocabulary'),
      $entity_manager->getStorage('taxonomy_term'),
      $container->get('current_user'),
      $entity_manager->getAccessControlHandler('node'),
      $entity_manager->getFieldMap(),
      $entity_manager->getStorage('node_type'),
      $container->get('renderer')
    );
  }

So most of the facilities are already there :)

Note that the "x new comments" stuff doesn't need to fixed here; that's a separate can of worms that already has its own issue: #2082317: Forum history markers ("new" and "updated" markers, "x new posts" links) forces render caching to be per user.

Sorry for taking so long to review.

1. +++ b/core/modules/forum/src/Controller/ForumController.php
   @@ -75,6 +76,16 @@ class ForumController extends ControllerBase {
      /**
   +   * @var \Drupal\Core\Entity\EntityTypeInterface
   +   */
   +  protected $nodeListDefinition;
   +
   +  /**
   +   * @var \Drupal\Core\Entity\EntityTypeInterface
   +   */
   +  protected $nodeDefinition;
   
   @@ -93,8 +104,12 @@ class ForumController extends ControllerBase {
   +   * @param \Drupal\Core\Entity\EntityTypeInterface $node_definition
   +   *   Node definition object
   +   * @param \Drupal\Core\Entity\EntityTypeInterface $node_list_definition
   +   *   Node list definition object
   

   Comments here need some love.

2. +++ b/core/modules/forum/src/Controller/ForumController.php
   @@ -103,6 +118,8 @@ public function __construct(ForumManagerInterface $forum_manager, VocabularyStor
   +    $this->nodeDefinition = $node_definition;
   +    $this->nodeListDefinition = $node_list_definition;
   
   @@ -119,7 +137,9 @@ public static function create(ContainerInterface $container) {
   +      $entity_manager->getDefinition('node'),
   +      $entity_manager->getDefinition('node_type')
   
   @@ -208,6 +228,9 @@ protected function build($forums, TermInterface $term, $topics = array(), $paren
        return [
          'action' => $this->buildActionLinks($config->get('vocabulary'), $term),
          'forum' => $build,
   +      '#cache' => [
   +        'tags' => Cache::mergeTags($this->nodeDefinition->getListCacheTags(), $this->nodeListDefinition->getListCacheTags()),
   +      ],
   

   The first one makes sense, the second does not.

   The second was added because of @dawehner's comment earlier:

   For that we probably also need the node type list tags or at least the node types cache tags.

   @dawehner was right that it depends on the node type, but this is currently depending on the *list* of node types. That's wrong. We only need to depend on the node types that are actually depended upon.

   So, in ::buildActionLinks(), we want something like this:

   +        $node_type = $this->nodeTypeStorage->load($type);
            $links[$type] = [
              '#attributes' => ['class' => ['action-links']],
              '#theme' => 'menu_local_action',
              '#link' => [
                'title' => $this->t('Add new @node_type', [
   -              '@node_type' => $this->nodeTypeStorage->load($type)->label(),
   +              '@node_type' => $node_type->label(),
                ]),
                'url' => Url::fromRoute('node.add', ['node_type' => $type]),
              ],
   +          '#cache' => [
   +            'tags' => $node_type->getCacheTags(),
   +          ]
   

3. +++ b/core/modules/forum/src/Tests/ForumIndexTest.php
   @@ -58,6 +58,8 @@ function testForumIndexStatus() {
   +    $this->assertCacheTag('config:node_type_list');
   

   Then this would be replaced with something like config:node_type.forum

#25 fully addresses #23 and #22.1.

That only leaves #23.2:

I still think this lacks a reference to the term and comments. The number of comments will change the cached build.

By this, @larowlan was referring to

    $build = array(
      '#theme' => 'forums',
      '#forums' => $forums,
      '#topics' => $topics,
      '#parents' => $parents,
      '#header' => $header,
      '#term' => $term,
      '#sortby' => $config->get('topics.order'),
      '#forums_per_page' => $config->get('topics.page_limit'),
    );

In there, $forums, $topics, $parents and $term are all arrays of content entities. You basically want to iterate over all of them, and invoke $this->renderer->addCacheableDependency($build, $the_entity_being_iterated) on all of them.

And then, finally, regarding the "number of comments" part of @larowlan's comment, that needs the comment_list cache tag. Which means you want to inject the Comment entity type definition just like the Node entity type definition, and use its list cache tags.

+++ b/core/modules/forum/src/Controller/ForumController.php
@@ -205,9 +229,23 @@ protected function build($forums, TermInterface $term, $topics = array(), $paren
+    foreach ($forums as $forum) {
+      $this->renderer->addCacheableDependency($build, $forum);
+    }

The exceptions in the test runner show that this is also not an array, but a single entity.

Can $topics be NULL? That's the only reason I can think of for that strange failure. Can't you then just change the logic that provides $topics so that it defaults to the empty array instead?

Then... huh? I can't use a debugger ATM, not at my workstation.

I did the necessary debugging to figure out the answer to #30/#31/#32/#33.

The root cause was silly & simple: in one branch, \Drupal\forum\Controller\ForumController::forumPage() was setting $topics to the empty string. That caused it.

That sole tiny fix doesn't prevent me from RTBC'ing this patch IMHO, so: RTBC! :)

Oops, you're right, those bits are missing from test coverage. Sorry.

Awesome, thanks!

Hehe, right :)

Thanks!