Don't invalidate cache tags of referenced entities, use entity list cache tags correctly, add test coverage for entity list cache tags

Looks great, bunch of questions and hints...

1. +++ b/core/lib/Drupal/Core/Entity/Entity.php
   @@ -410,9 +409,9 @@ public function getCacheTag() {
   +  public static function getListCacheTag() {
   +    $entity_type_id = \Drupal::entityManager()->getEntityTypeFromClass(get_called_class());
   +    return [$entity_type_id . 's' => TRUE];
   

   Wondering if we should just move this to EntityTypeInterface, that would be easier to access when it is static, and this could stay non-static and forward to that? Harder to override I guess, not sure how common that is. (Not sure how a solution for that would look like atm)

   => This shorthand static methods and the loading based on the class name is most a shorthand for developers, it is not guaranteed to work (for example, when you have dynamically defined entity types or something like that).

2. +++ b/core/lib/Drupal/Core/Entity/Entity.php
   @@ -439,26 +438,6 @@ public static function create(array $values = array()) {
   -  protected function onSaveOrDelete() {
   

   Beautiful.

3. +++ b/core/modules/book/book.module
   @@ -264,6 +265,11 @@ function book_node_view(array &$build, EntityInterface $node, EntityViewDisplayI
   +        // The book navigation is a listing of Node entities, so associate its
   +        // list cache tag for correct invalidation.
   +        '#cache' => [
   +          'tags' => Node::getListCacheTag()
   +        ],
   

   Nitpick: missing trailing comma.

4. +++ b/core/modules/config/tests/config_test/src/Entity/ConfigTest.php
   @@ -136,4 +136,11 @@ public function calculateDependencies() {
   +  public static function getListCacheTag() {
   +    return ['config_tests' => TRUE];
   +  }
   

   I guess this is one of those where the dynamic lookup doesn't work?

5. +++ b/core/modules/hal/src/Tests/EntityTest.php
   @@ -64,7 +64,8 @@ public function testNode() {
   -      )
   +      ),
   +      'revision_log' => $this->randomString(),
   

   Why is this necessary?

6. +++ b/core/modules/hal/src/Tests/EntityTest.php
   @@ -160,13 +161,32 @@ public function testComment() {
   +    $parent_comment = entity_create('comment', array(
   +      'uid' => $user->id(),
   +      'subject' => $this->randomMachineName(),
   

   What are we testing here exactly?

7. +++ b/core/modules/system/src/Tests/Cache/PageCacheTagsTestBase.php
   @@ -50,6 +57,7 @@ protected function verifyPageCache($path, $hit_or_miss, $tags = FALSE) {
   +      $tags = array_unique($tags);
          sort($tags);
   

   I guess this is related to the non-unique tags issue that we found?

8. +++ b/core/modules/system/src/Tests/Entity/EntityCacheTagsTestBase.php
   @@ -255,16 +268,19 @@ protected function createReferenceTestEntities($referenced_entity) {
   +   * - entity cache tag: "<entity type>:<entity ID>"
   +   * - entity type list cache tag: "<entity type>:<entity ID>"
   

   Are you sure this is correct? should this be the {$id}s tag?

9. +++ b/core/modules/user/src/Tests/UserPictureTest.php
   @@ -113,7 +117,9 @@ function testPictureOnNodeComment() {
   -    \Drupal::entityManager()->getViewBuilder('comment')->resetCache();
   +
   +    // @todo Remove when https://www.drupal.org/node/2040135 lands.
   +    Cache::invalidateTags(['rendered' => TRUE]);
   

   Ah, the previous call didn't work anymore because we're then testing on nodes and comments.

   Looks like we enable the features through the API? Would it work if we would use the UI? And if not, should we make the UI work with this tag for now, so that not only the test works, but actually doing it as a user does too?

10. +++ b/core/modules/views/src/Plugin/views/cache/CachePluginBase.php
    @@ -358,9 +358,10 @@ protected function getCacheTags() {
           foreach (array_keys($entity_information) as $entity_type) {
    -        $tags[$entity_type . '_list'] = TRUE;
    +        $class = \Drupal::entityManager()->getDefinition($entity_type)->getClass();
    +        $tags += $class::getListCacheTag();
    

    Yeah, code like this when knowing that this will then call back to the entity manager to figure out the entity type is.. crazy :)

    But nice find.

    I'm still planning to look into manually optimizing the cache tag handling once it works in a general/global way. What I have in mind is basically a custom cache plugin that allows me to manually set a cache tag + code that clears that tag on certain conditions. Like, for a view of promoted nodes of a specific type, I don't want to add the global list tag, because that will do way more cache clears than necessary). But that's a different topic...

Re-roll.

EntityTest: Had a look. What is/was happening is that onSaveOrDelete() -> getReferencedEntities() goes through all fields, field items and their properties to find EntityReference properties. inlcuding computed

This force-initializes $this->properties for all properties, which only happens when you access it as a property (->get('value') or getProperties() and not ->value).

That affects Map::getValue(), which starts of with $this->value and adds initialized properties, so that any changed value is correctly updated and returned. On HEAD, properties are force-initialized, so they are explicitly added to $values. Now, this no longer happens. That's where the difference is.

I think fixing it by explicitly providing values is good enough for this, the serializiation of empty fields is already weird in HEAD and it has no practical difference, we could actually consider to filter out fields that are empty from the test, not sure and not here.

I think this leaves the behavior and location of getListCacheTag() as the last problem here.

I am a bit confused and curious, as this basically means that the cache tags can't depend on actual information any longer. Won't this make a lot of the really complex usecases
of views even impossible right from the beginning?

Just appending and 's' to the end is still strange, I didn't like that before either. Some entity types this is weird with:

- block_content > block_contents
- editor > editors
- menu_link_content > menu_link_contents
- taxonomy_vocabulary > taxonomy_vocabularys

Also, we need to consider the possibility of getting bundle aware list tags? How do we manage that generally? Let alone with a static method. I'm not sure :)

Or do we just make them ourselves?

The title and summary don't describe a critical issue, and the patch seems to be doing more than adding test coverage. Can we update it with the actual scope of this issue, including explaining how it actually blocks reliable views caching (see Wim's initial comment in #1).

Note that if it's a soft blocker for the related issues Wim links (would make them easier/cleaner/safer to patch, but not a hard blocker nor any actual critical bug), then it would make sense to mark this major. Critical tasks are usually designated at maintainer discretion, and this issue doesn't really fit the description. Thanks!

@xjm: This is critical for two reasons I think:
a) We currently have 3 different notations for "list of entities": nodes, node_view and node_list and we use it differently in different places. That makes it impossible/very hard to use the correct cache tag for clearing/tagging "listings of nodes".

b) In HEAD, we clear cache tags of *referenced* entities. For example, comments display and author and reference that author. When you currently update a comment, we clear the cache tag of that user. That means that we delete every render cached node/comment/entity authored by that user. That is very inefficient and unecessary, because cache tags are supposed to work the other way round. Your cache should be included everywhere where it is relevant, so that is enough to just clear your own cache tag. Changing this shows a number of places that currently don't use cache tags properly (Or incorrectly/unknowingly rely on this behavior to work), so this patch has to fix them.

Those are API/behavior changes that need to happen asap, so that contrib can use cache tags properly.

So a better title/scope would be something like: "Do not propagate cache tag clears referenced entities and standardize tag list names"? And sounds like a bug instead?

#2145751: Introduce ENTITY_TYPE_list:BUNDLE cache tag and add it to single bundle listing was an issue opened when we had the initial views cache tags issue in. Which we then simplified. I would say list tags is a different thing? Maybe that could be re purposed, I don't mind.

Why (getCacheTag() changed somewhere else too) are we also changing to getListCacheTag(). We should stick with tagS, no? It is feasible (and already done) that more than one tag is returned. As well as the return being documented as being an array. Both of these methods should be plural.

Also, when are we going to change the $entity_type . 's' usage? I'm sure we talked about this a few times. Can we make an issue for it?

1. +++ b/core/lib/Drupal/Core/Entity/Entity.php
   @@ -409,9 +408,9 @@ public function getCacheTag() {
   -    // @todo Add bundle-specific listing cache tag? https://drupal.org/node/2145751
   

   This is something we still need to resolve afaik?

2. +++ b/core/lib/Drupal/Core/Entity/Entity.php
   @@ -409,9 +408,9 @@ public function getCacheTag() {
   +    $entity_type_id = \Drupal::entityManager()->getEntityTypeFromClass(get_called_class());
   

   ugh, howcome?

3. +++ b/core/lib/Drupal/Core/Entity/Entity.php
   @@ -469,7 +448,7 @@ protected function invalidateTagsOnSave($update) {
   +    $tags = static::getListCacheTag();
   

   We can call static methods in the context of an instance too, but meh. Does not really matter.

4. +++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
   @@ -349,11 +349,20 @@ public function getCacheTag() {
   +        $tags = Cache::mergeTags($tags, $entity->getCacheTag(), $entity->getListCacheTag());
   

   Nice, it's happening here anyway :)

5. +++ b/core/modules/block/src/Entity/Block.php
   @@ -157,14 +157,31 @@ public function calculateDependencies() {
   +  public function getCacheTag() {
   +    return Cache::mergeTags(parent::getCacheTag(), ['theme:' . $this->theme]);
      }
   

   It is stuff like this I was talking about in my last comment, about being plural.

6. +++ b/core/modules/book/book.module
   @@ -243,6 +244,11 @@ function book_node_view(array &$build, EntityInterface $node, EntityViewDisplayI
   +          'tags' => Node::getListCacheTag(),
   
   +++ b/core/modules/book/src/BookExport.php
   @@ -84,6 +85,9 @@ public function bookExportHtml(NodeInterface $node) {
   +        'tags' => Node::getListCacheTag(),
   
   +++ b/core/modules/book/src/Controller/BookController.php
   @@ -110,6 +111,9 @@ public function bookRender() {
   +        'tags' => Node::getListCacheTag(),
   

   I guess technically we should get the entity type from the class in this case? To future proof the class being switched out?

And generally the test coverage improvements look great.

Wim, saw your tell on IRC:

1. Follow up is fine, created that: #2345139: Rename list cache tag suffixes from 's' to '_list'
2. The fact that this method currently just returns one tag seems like a bad reason to rename it to singular, plus it just looks wrong IMO. I also mentioned before, it's returning an array too. These methods can be overridden etc.. and it is likely more would get added. The singular naming implies that just one tag would always get returned, if that was the case it should return a string.

+1 to the changes about renaming the method and s => _list.

Fixed those test fails I hope. Working on what we discussed.

Ok, moving list cache tags to EntityType, let's see what I missed. Unit tests are passing.

Grr :)

Only the test that failed on HEAD failed, this should be green, so needs review.

Fixed those things.

A conflict in the Shortcut annotation and ViewsUI had one method at the end removed by us and another added, keeping at RTBC.

Cool, this looks good. +1

Looks good, nothing to complain about.

I had reviewed this too the other night and only didn't commit it on a technicality, soooo...

Committed and pushed to 8.x. Thanks!