Problem/Motivation

There is ENTITY_TYPE_list cache tag and it is being invalidated very often. For example, if we have 20 different content types and created views page to display the list of content for each type, then whenever editor adds/edits/deletes one of the content - all 20 views pages will be invalidated BUT only related should be invalidated, others still have no changes. So, it means that on current sites if somebody changes something then Drupal will clear cache for almost the entire website.

Proposed resolution

Add bundle specific list cache tags - ENTITY_TYPE_list:BUNDLE

Also:

  • Rename getListCacheTags to getListCacheTagsToInvalidate.
  • Add getListCacheTags in the follow up where we determine bundle specific cache tag based on query object.

so that listing pages can add bundle specific list cache tags or generic list cache tags

Remaining tasks

  1. Write tests as per #93.5
  2. Create change record.
  3. Open follow-up issues for views, search_api and other modules that will require an update.

Release notes snippet

Added an ENTITY_TYPE_list:BUNDLE cache tag for all entity types with bundles, e.g. node_list:article, which allows for more targeted cache invalidation in custom code and views in combination with a module like Views Custom Cache Tag

Original report by [effulgentsia]

Follow up from #1712456: How to leverage cache tags in Views.

Two problems:

  1. That issue changed EntityViewBuilder::resetCache() from just clearing the TYPE:ID tag to also clearing the TYPE_view_BUNDLE tag. However, what is the meaning of this tag? What does it mean to "view" a bundle? #1712456-50: How to leverage cache tags in Views says "Say you save settings for one node type, it would invalidate caches for every node on the system?!", so ok, if the intent of this tag is to clear caches upon a change to the settings of a bundle, then why is it being cleared upon the saving of a single entity, which is when EntityViewBuilder::resetCache() is called? OTOH, the same comment also says We need a way like this to invalidate 'lists' containing this entity type. This and 'view' could be similar though I guess.. Hm, if the 'view' here refers to lists (where Views module is the most common list generator) rather than to the 'view' in the sense of EntityViewBuilder, then let's rename this tag to 'list' instead. If we then also want a per-bundle 'view' tag for clearing when bundle settings change in a way that would affect their display, then that's what we should use 'view_BUNDLE' for, but then clear it only where bundle settings are managed (e.g., EntityDisplay) rather than in EntityViewBuilder::resetCache().
  2. If we do rename this to TYPE_list_BUNDLE, then how should we handle Views that are cross-bundle? For example, currently in HEAD, if I enable tag-based caching on the front page View, then visit the front page at a time when only Article nodes are promoted to the front page, then I add a Page node and promote it, it doesn't show up on my front page until I manually clear caches. Perhaps we need a non-bundle-specific TYPE_list tag as well, and add that to all cross-bundle Views, but not to Views and EFQ listings that we know are bundle-specific?
CommentFileSizeAuthor
#142 2145751-142-interdiff.txt861 bytesBerdir
#142 2145751-142.patch10.68 KBBerdir
#140 2145751-140-interdiff.txt5.01 KBBerdir
#140 2145751-140.patch11.52 KBBerdir
#135 2145751-interdiff-132-135.txt3.09 KBarpad.rozsa
#135 2145751-135.patch14.15 KBarpad.rozsa
#132 2145751-interdiff-122-132.txt1.2 KBarpad.rozsa
#132 2145751-132.patch16.16 KBarpad.rozsa
#122 2145751-122-interdiff.txt7.21 KBBerdir
#122 2145751-122.patch15.24 KBBerdir
#120 interdiff-2145751-120-115.txt4.26 KBmbovan
#120 2145751-120.patch11.85 KBmbovan
#115 2145751-115.patch11.65 KBNixou
#112 interdiff-2145751-110-112.txt1.22 KBjohnchque
#112 2145751-112.patch11.6 KBjohnchque
#110 interdiff-2145751-104-110.txt2.38 KBjohnchque
#110 2145751-110.patch11.63 KBjohnchque
#104 interdiff-2145751-102-104.txt2.77 KBjohnchque
#104 2145751-104.patch9.71 KBjohnchque
#102 interdiff-2145751-98-102.txt3.92 KBjohnchque
#102 2145751-102.patch9.75 KBjohnchque
#98 2145751-98.patch5.29 KBjibran
#98 interdiff-fb1002.txt1.03 KBjibran
#94 2145751-94.patch6.32 KBjibran
#94 interdiff-109953.txt6.18 KBjibran
#92 2145751-92.patch9.65 KBjibran
#92 interdiff-403f92.txt7.94 KBjibran
#86 2145751-86.patch4.26 KBjibran
#81 2145751-81.patch29.45 KBjibran
#81 interdiff-aea867.txt3.16 KBjibran
#79 2145751-79.patch28.67 KBjibran
#79 interdiff-213169.txt10.79 KBjibran
#75 2145751-75.patch21.1 KBjibran
#75 interdiff-a95d57.txt12.15 KBjibran
#9 entity_bundle_list_cache_tags-do-not-test.patch23.49 KBWim Leers
#12 entity_list_cache_tags_invalidation-2145751-12.patch10.61 KBDuaelFr
#22 entity_list_cache_tags_invalidation-2145751-12.patch10.61 KBDuaelFr
#33 entity_list_cache_tags_invalidation-2145751-33.patch0 bytesDuaelFr
#34 entity_list_cache_tags_invalidation-2145751-34.patch10.62 KBDuaelFr
#44 interdiff-2145751-34-44.txt8.96 KBpnagornyak
#44 entity_list_cache_tags_invalidation-2145751-44.patch13.29 KBpnagornyak
#45 interdiff-2145751-44-45.txt966 bytespnagornyak
#45 entity_list_cache_tags_invalidation-2145751-45.patch13.37 KBpnagornyak
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

effulgentsia’s picture

Issue summary: View changes
effulgentsia’s picture

Issue summary: View changes
effulgentsia’s picture

Issue tags: +VDC
damiankloip’s picture

So yes, I originally wanted to have list_ tags too, but got talked out of it. See from #1712456-22: How to leverage cache tags in Views onwards.

I will work on a patch to add back the stuff I was originally thinking a bit later today. I also like the idea of adding the generic ENTITY_TYPE_list tags.

moshe weitzman’s picture

I think we want to rename to LIST tag. I think it is OK for now to ignore cross bundle lists. We are getting rid of tag based views cache at #2158551: Views cache strategies are confusing. So folks will be using time based expiration. We should encourage folks to use a short duration of 5-10 mins and then items will be added to the list "shortly".

damiankloip’s picture

Just a rename does not achieve much here, we need these tags to be more generalised too. As well as fixing when these tags are invalidated. Yes, I think trying to reliably use the bundle is probably not feasible in views. As you could have any config for your filtering etc.. for bundles - and getting all the possible bundles in one hit is almost impossible.

I commented on the other issue, I do not want to get rid of the Tag cache plugin (at least no yet anyway).

We should encourage folks to use a short duration of 5-10 mins and then items will be added to the list "shortly".

Isn't that just pure time based? If that is what we tell people, we might as well just remove all traces of cache tags in views.

moshe weitzman’s picture

To me, cache tags are still useful. Many edits will result in instant View invalidation which is good. For the rest, we rely on time based expiration. These things work together.

Wim Leers’s picture

Wim Leers’s picture

While working on #2304987: Don't invalidate cache tags of referenced entities, use entity list cache tags correctly, add test coverage for entity list cache tags, I had at point added per-bundle list cache tags for entities (besides the cross-bundle cache tags that we already have). While my implementation was flawed, and it was out of scope (and hence removed from that patch), the general test coverage changes should still be the same. Therefore, uploading that part of the patch for posterity, in case we want to add per-bundle list cache tags again at some point.

DuaelFr’s picture

Currently the cache tags documentation indicate the availability of the "node_list" cache tag.
Searching the code it seems to be used by the node_title_list() function only.
The problem is that this tag is never invalidated.

I suggest to implement TYPE_list and TYPE_list:BUNDLE tags and to clear both on entities save. Views or custom code could then choose to implement the bundle tag or the generic one according to their needs.

Wim Leers’s picture

The node_list cache tag is used, available, invalidated and has test coverage.

See \Drupal\Core\Entity\EntityType::getListCacheTags(), the many invocations of that method, and the unit test coverage for it in \Drupal\Tests\Core\Entity\EntityUnitTest::testPostSave() and the integration test coverage for it in \Drupal\system\Tests\Entity\EntityCacheTagsTestBase::testReferencedEntity().

This issue is about adding a per-bundle list cache tag. i.e. not just "all nodes" but "all article nodes", "all page nodes", etc.

DuaelFr’s picture

You are too fast @Wim Leers!
I was writing that I was wrong and that #2304987: Don't invalidate cache tags of referenced entities, use entity list cache tags correctly, add test coverage for entity list cache tags has added that tag.
Thank you for your devotion :)

Here is my thin contribution to this issue.
Let's see what the testbot has to say.

Wim Leers’s picture

Title: Problems with ENTITY_TYPE_view_BUNDLE cache tag » Introduce ENTITY_TYPE_view_BUNDLE cache tag
Category: Bug report » Task
Issue tags: +API addition
Wim Leers’s picture

Nice work, but I think we don't want this just before RC1. We want this, but it's too late now, this belongs in 8.1.0 I'm afraid. Thanks for the great patch to get us started! :)

DuaelFr’s picture

You are too nice. It's basically a rewrite of your #9's patch ;)
I agree that this should be postponed. I was really scared that the TYPE_list tag were never invalidated but as you pointed, it is.
Do you think it's something we want in 8.0.x or more in 8.1.x?

Status: Needs review » Needs work

The last submitted patch, 12: entity_list_cache_tags_invalidation-2145751-12.patch, failed testing.

Wim Leers’s picture

Version: 8.0.x-dev » 8.1.x-dev
Issue tags: +minor version target

8.1.x, perhaps even 8.2.x. We need to learn from real sites during the 8.x cycle. Specifically, we need to learn from complex Views use cases. It's possible this will not be necessary, because a more optimal, more refined approach will emerge from real-world experience.

Moving to 8.1 — glad to have you on board!

The last submitted patch, 12: entity_list_cache_tags_invalidation-2145751-12.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Postponed

Postponing to 8.1 or later works for me. Changing status accordingly. When I opened this issue, I wrote:

That issue changed EntityViewBuilder::resetCache() from just clearing the TYPE:ID tag to also clearing the TYPE_view_BUNDLE tag.

And that is still in the issue summary, and most of the rest of that summary is about the consequences of that. But that tag no longer exists in HEAD, hence the issue title rename in #13, so I think we're in ok shape for 8.0.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

DuaelFr’s picture

Title: Introduce ENTITY_TYPE_view_BUNDLE cache tag » Introduce ENTITY_TYPE_list:BUNDLE cache tag
Status: Postponed » Needs review
Issue tags: -Needs beta evaluation +Performance
FileSize
10.61 KB

No reason to keep this postponed anymore.
Introducing the bundle for entities that have one would be a cheap but really good improvement for a lot of cases.

eg: when I have the list of the last 3 articles on my home page, I don't want it to be invalidated each time a standard page is created/updated.

The attached patch file is just a repost of #12's to be tested against 8.3.x.
It seems that it contains some out-of-scope changes. Shouldn't we add some follow-up to address the changes in Block, BookExport and Shortcut entities?

Status: Needs review » Needs work

The last submitted patch, 22: entity_list_cache_tags_invalidation-2145751-12.patch, failed testing.

Wim Leers’s picture

Wim Leers’s picture

Fabianx’s picture

  1. +++ b/core/lib/Drupal/Core/Entity/EntityInterface.php
    @@ -361,6 +361,16 @@ public function referencedEntities();
    +  public function getListCacheTags();
    

    Adding this function to the interface is a problem as its a BC break, so it needs to be its own interface that is implemented and used by the abstract base class.

  2. +++ b/core/modules/block/src/BlockViewBuilder.php
    @@ -87,6 +87,7 @@ public function viewMultiple(array $entities = array(), $view_mode = 'full', $la
    +      $cache_tags = Cache::mergeTags($cache_tags, $entity->getListCacheTags());
    

    Out of scope change, the the block_list cache tag is added elsewhere.

Fabianx’s picture

The biggest problem right now also is that this would still invalidate the node_list cache tag on entity creation, so we have not won anything, yet.

Berdir’s picture

1. We actually can add methods to EntityInterface if we provide a default implementation.

+++ b/core/lib/Drupal/Core/Entity/Entity.php
@@ -441,6 +441,17 @@ public function getCacheContexts() {
+    if ($this->bundle() != $this->entityTypeId) {
+      Cache::mergeTags($tags, [$this->entityTypeId . '_list:' . $this->bundle()]);
+    }

this check is not correct, the correct way to do this is to add an hasKey('bundle').

(Absolutely nothing prevents me from creating a node type called... "node")

And yes, for now, this just adds *more* cache tag invalidations. You only benefit from it when you use them an core doesn't give you a way yet to do this. You'd need something like my https://www.drupal.org/project/views_custom_cache_tag project.

So, just like we agreed before, we need to think this through and how we plan to use this (can we make views intelligent to automatically use this? how.. ? what if multiple bundles are used.. ? ) as at first, it would actually make things (very slightly) slower.

Other ideas would be to generate special cache tags for views and invalidate them when saving an entity that matches into a certain view, but then we'd need an API for that and would I guess require a lot of complexity, so sounds like contrib to me. If someone wants to experiment with that in my custom cache tag project, you're welcome.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

anavarre’s picture

Priority: Normal » Major
Issue tags: +scalability

I'm actually surprised it wasn't flagged as major before. In #17, Wim said:

We need to learn from real sites during the 8.x cycle. Specifically, we need to learn from complex Views use cases.

D8 has been out for a while now, and real-life sites I'm seeing are invalidating node_list like crazy, and even causing sites to be down and/or hit CDN throttling. Because IIUC each time you will update e.g. a node title for a node that is part of a view, then all nodes in that view but also all other entities in any other view will then be sent for invalidation since they all share the same very generic tag. This is very expensive.

mysql> SELECT * FROM cachetags ORDER BY invalidations DESC LIMIT 10;
+--------------------------------------------+---------------+
| tag                                        | invalidations |
+--------------------------------------------+---------------+
| 4xx-response                               |         31160 |
| node_list                                  |         23175 |
| route_match                                |         20121 |
| config:views.view.my_workflow              |         16677 |
| config:views.view.workbench_custom_content |         16666 |
| config:views.view.my_content               |         16660 |
| config:views.view.all_content              |         16657 |
| file_list                                  |         14090 |
| node_view                                  |          8761 |
| entity_field_info                          |          6697 |
+--------------------------------------------+---------------+
10 rows in set (0.02 sec)

Another site

mysql> SELECT * FROM cachetags ORDER BY invalidations DESC LIMIT 10;
+--------------+---------------+
| tag          | invalidations |
+--------------+---------------+
| 4xx-response |         45617 |
| node_list    |         45065 |
| node:31      |         42308 |
| node:6       |         42307 |
| node:41      |         42307 |
| node:56      |         42300 |
| node:66      |         42295 |
| node:111     |         42291 |
| node:101     |         42290 |
| node:96      |         42290 |
+--------------+---------------+
10 rows in set (0.00 sec)

And another one...

mysql> SELECT * FROM cachetags ORDER BY invalidations DESC LIMIT 10;
+----------------------------------+---------------+
| tag                              | invalidations |
+----------------------------------+---------------+
| 4xx-response                     |        110180 |
| file_list                        |         62677 |
| route_match                      |         59039 |
| paragraph_list                   |         58955 |
| node_list                        |         54740 |
| media_list                       |         34512 |
| advertising_product_list         |         12079 |
| crop_list                        |         11123 |
| config:acquia_connector.settings |         10788 |
| product_list                     |         10598 |
+----------------------------------+---------------+
10 rows in set (5.15 sec)

It's not always the worst offender, but it's almost always here and causes quite a lot of invalidation requests. Being able to be a little more specific with bundle-specific cache tags would seem like a great improvement already and would prevent leveraging contrib for something core should take care of by design it seems. This would then allow to not always attach node_list to views, right?

Not necessarily in scope for that issue but it's also interesting to see 4xx-response is always at the top of the list.

Berdir’s picture

https://www.drupal.org/project/views_custom_cache_tag is one possible solution for views but it requires a bit of custom code to tie it together but is as flexible as you need it (like promoted nodes based on a term reference).

anavarre’s picture

@Berdir, I know about this module and understand it does help, but the whole point is to make it something core would support OOTB or - worst case scenario - have contrib to transparently take care of things with sane defaults without writing custom code.

DuaelFr’s picture

DuaelFr’s picture

DuaelFr’s picture

For the record, I wonder if we shouldn't also add operation-dependant tags too. Something like TYPE_list.OP and TYPE_list.OP:BUNDLE.

Let's see an example: I build a block that shows the last 3 articles of my site. If we only have the node_list tag, the block is going to be invalidated each time a node is created, edited or deleted. If we have node_list:article the bloc if going to be invalidated each time an article is created, updated or deleted, what's better. The thing is that I only need to refresh this cache is a new article is created as I already have cache tags for the 3 articles shown so if one of them is edited or deleted, my cache is already going to be invalidated.

Berdir’s picture

There could be an update to another node that would result in it now being qualified for your list, for example by changing the create date. And there could also be cases where a delete on another node would also influence the list, for example when working with an offset (showing articles 2-5 then 1 is deleted).

DuaelFr’s picture

@Berdir You are right. It was just a simple example. As a developper I should have the choice to use the tags that I think are suiting my current use case, though.

Berdir’s picture

Right, but if we're talking about developers, then my module in #31 is a viable solution because then you can build whatever tag you need exactly and add that to the view, including dynamic things like based on a term reference or other field using views arguments.

Building something that works out of the box, automatically, as mentioned in #32, is so much harder and I was just pointing out why your suggestion IMHO doesn't work as an automated default solution.

Status: Needs review » Needs work

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

anavarre’s picture

Issue tags: +Needs reroll
GaëlG’s picture

Issue tags: -Needs reroll

I could apply the patch #34 to latest 8.5.x, so no reroll is needed, right?

pnagornyak’s picture

I propose:

  • clear cache ENTITY_TYPE_list and ENTITY_TYPE_list:BUNDLE on entity save/delete.
  • The $entity->getListCacheTags(); should return bundle related cache tags ONLY if it exists. We should not merge it with $entityType->getListCacheTags(); otherwise it will be difficult to split cache tags.
  • Add functionality to View`s CachePluginBase to check if there are Bundle filters then add bundle specific cache tag instead of global.

Also, I renamed $block->getBundleListCacheTag(); to $block->getListCacheTags(); cause it seems like this method was never used.

pnagornyak’s picture

pnagornyak’s picture

Issue summary: View changes
pnagornyak’s picture

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

webchick’s picture

Re-queued the test for #45 against 8.6.x just to see where we are at.

I do think that ideally, we would not require end users to both know of an arbitrary contrib module that only ~200 sites are using, plus writing code on top of that, to avoid this issue. 43 followers is pretty significant, as core issues go.

@Berdir, could you maybe clarify why it is not possible for Views to infer the correct cache tags based on what it knows about each views' filters, fields, etc?

Berdir’s picture

Issue tags: +Needs tests

Well, I'm not saying it's "not possible". But there are lots of special cases.. what about a node type condition that says "NOT article", it could be coming from the arguments, or custom filters that contain dynamic logic, or condition groups (published articles or pages)

At the same time, adding more cache tags that need to be checked and more cache tag invalidations is an overhead, so we should just throw in arbitrary cache tags that are then not used.

And it gets more fun when you mix multiple conditions, e.g. published articles of a certain category, based on an argument. The most efficient way is to create optimized cache tags for that and that more or less requires custom cache tag invalidation logic.

That doesn't mean that we can't work on it, we can start by implementing some simple cases, but we will need a lot of tests, so tagging for that.

Wim Leers’s picture

to avoid this issue

It won't avoid/solve this issue. It'll just make the problem more manageable in some cases. It'll only help if your say 1 million nodes are nicely spread across different bundles. If they're all the same bundle, this will not improve anything.

And it gets more fun when you mix multiple conditions, e.g. published articles of a certain category, based on an argument. The most efficient way is to create optimized cache tags for that and that more or less requires custom cache tag invalidation logic.

Exactly!

Josh Waihi’s picture

I recently worked on a sports event site that delivered 128 million page views over a 2 week period. We used a cache invalidation strategy in conjunction with a Varnish setup with the Purge module. The strategy was highly effective with only about 0.1% of the total traffic flowing through to Apache and Drupal.

< entity >_list is a real pain when it comes to an invalidation strategy as it is too liberal around cache invalidation. We can't do anything about that in Drupal's internal caching system but the Purge suite does allow you to blacklist tags which at least mean't node_list, taxonomy_term_list and paragraph_list wouldn't virtually drop out page cache every time an article was created or a game was updated.

< entity >_list:< bundle > tags wouldn't have any improved use in the case of this event site as content was mingled: articles appear on game pages, games feature in article pages. It would just mean more tags to black list.

We manually mapped views tags to nodes based on their criteria in a custom module and node presave hook. We used the Views Custom Cache Tags module to allow us to expand views tags to use arguments so we have the right granularity to accurately cache only pages applicable.

The implementation was advanced and meticulous - but we missed a bunch of mappings that led to stale caches that wouldn't expire for a month (longer than the duration of the event == not good). I don't hold a lot of confidence that the average site could implement such an effective invalidation strategy as we did without deep understanding of cache tags, purge suite and HTTP Cache-Control.

Given cache tags are a massive advancement in Drupal's maturity, the ability to purge the applicable lists (views) opposed to all in an entity or bundle realistically means site owners without the depth of expertise to implement an invalidation strategy will continue to use an expiry strategy as its completely managed in the UI.

I'd advocate for a direction where core makes it easier to implement an accurate invalidation strategy opposed to a less inaccurate one.

pnagornyak’s picture

I suppose that we should add new functionallity to Drupal maybe like contrib module or like part of Devel that can collect all the data about Cache tags. Right now we can only see how many times the cache tag was invalidated, so I propose to store the history about invalidation and than devs can decide what to do with it. Also would be nice to have an option to set a min cache time for content type or for view or block etc. and the module will compare this value with real statistics and if the time is less then log warning. (min cache time - is only for comparison, it should not change the cache logic)

Drupal must have the Cache, but also it must be sure that the Cache used in right way :)

P.S. if its good idea and nobody before worked on it then I will start, just somebody ping me.

DuaelFr’s picture

I just had a thought about this issue and Views integration for the most common cases:
Views can have complex fields and relationships, but we don't care because this is about finding entities that are linked to another entity which already carries all the needed cache tags. If a new relationship appears, the host entity field is going to be changed so the entity tag is going to be invalidated anyway. Same for the fields (custom handlers should declare their own accurate tags).
Finally, all that matters are the type filters that are applied to the source entities (ie. not related to any relationship).

Can someone find something stupid in my thought or could it be considered as a good start for this issue?

GoZ’s picture

I'm agree with @DuaelFr.
The only case that views should deal with is to add the ENTITY_TYPE_list:BUNDLE cache tag in case a filter or argument is made on bundle.

Otherwise, views should not care about this cache tag. The existing ENTITY_TYPE_list (and the other entities cache tags) already deal with the common cases.

dawehner’s picture

Views can have complex fields and relationships, but we don't care because this is about finding entities that are linked to another entity which already carries all the needed cache tags. If a new relationship appears, the host entity field is going to be changed so the entity tag is going to be invalidated anyway. Same for the fields (custom handlers should declare their own accurate tags).

I'm not sure about this analysis. Having changes in child entities can change the list of host entries which appear. For me at least I think a better approach is: If you have relationships, opt out and don't provide something magic out of the box.

DuaelFr’s picture

@dawehner What I meant is that if there is a change in child entities, the bubbled-up cache tags are going to handle it anyway. Am I wrong about that?

dawehner’s picture

Oh I see what you mean, yeah that is totally true. It'll have the generic test one.

Wim Leers’s picture

Yes, @DuaelFr, that would be a good start. For some sites, this issue is going to make a nice difference in performance. But for some other sites, this is not going to make any difference at all: if they use the same bundle everywhere, ENTITY_TYPE_list:BUNDLE will be invalidated just as often as ENTITY_TYPE_list. That doesn't mean we shouldn't do this though!

Fabianx’s picture

The big problem here (and in d8cache module) is:

You add any view listing to every page => all your pages are invalidated when any node is created or updated.

The big problem with lists is that you cannot know if the list changes or not, however there are potential ways out:

Views itself already had this nailed down quite early:

- There is a 'result' and an 'output' cache, which are independent.

The 'result' cache is only running the query. If results have not changed, then there is no need to invalidate the output cache. (Note: In views D7 (no idea of D8) the full $view->result array is hashed including loaded entities - which is not necessary for D8, because we have cache tags.

That means in general it is enough to re-run all cached queries to determine which views are outdated and which are not. (that is still not possible for all views [think about a dynamic argument with time()], but for many at least.)

To implement that a little bit:

- You can make the problematic on-every-page-view a lazy builder + placeholder, then at least when your caches expire, you hopefully only have to re-generate the view just once and can then rely on dynamic_page_cache hit + one cache hit to rebuild your pages.

e.g. I think we should open a follow-up/different issue to make node_list an auto-placeholdering cache tag => that would probably be an easy win for most sites out of the box - especially if the view is in a block.

The other thing we should try to evaluate is a minimum TTL, where cache tags do not expire, yet - especially for internal page cache and external proxies and then use re-create in background within KernelEvents::terminate() - that will ensure a fast experience, while ensuring that caches are correct the next time the page is visited.

e.g. I might not care that my recent content is not yet showing up in the sidebar for 10 min.
(The difference between max-age and min-age: Max-Age is "Do not ever cache this longer than 10 min. You need to re-evaluate things then."; Min-age is: "If the item is expired, it must not live for more than 10 minutes" - though a better name is probably stale-while-revalidate)

[Just leaving this here as else I forget: Cache Tags are in reality mapping If-Modifed-Since for many items, they should be implemented as timestamps instead of counters, where the max(timestamp) is used and compared to the timestamp of the cache item. Min-TTL and Max-Age only work with a correct last-modified implementation and max-age == remaining TTL on cache item on cache hits.]

catch’s picture

e.g. I think we should open a follow-up/different issue to make node_list an auto-placeholdering cache tag => that would probably be an easy win for most sites out of the box - especially if the view is in a block.

Let's definitely do that.

The other thing we should try to evaluate is a minimum TTL, where cache tags do not expire, yet - especially for internal page cache and external proxies and then use re-create in background within KernelEvents::terminate()

We have CacheBackendInterface::get($cid, $allow_invalid = FALSE) for this - so that should be possible to do without an API addition.

Berdir’s picture

We have #2498857: Support min-age in render caching for min-age support that is using $allow_invalid. And we've been talking there as well about auto-placeholdering those as an additional overlap with this issue, see explanation there in my last comment.

Berdir’s picture

> e.g. I think we should open a follow-up/different issue to make node_list an auto-placeholdering cache tag => that would probably be an easy win for most sites out of the box - especially if the view is in a block.

The problem with just putting things in the auto-placeholder list is that if it can *not* be autoplaceholdered then it kills dynamic page cache. So if it's in a block, then we could auto-placeholder it, but an actual views page or anything that's inlined, e.g. a views that's loaded and displayed through twig or preprocess code on a node would result in disabling DPC entirely.

See #2232375: Make language switcher block cacheable, #2935804: Add cache context exclusion list to RenderCache::set() and #2888838: Optimize render caching.

Fabianx’s picture

Oh, that is bad coupling that I did not know we had.

Yes, the blacklist of DPC should be independent of auto-placeholdering. Coupling that is a mistake - even if it seemed convenient at the time.

Fabianx’s picture

While disabling DPC is a real bummer, we can do something else (and hard-code that for now in DPC until the generic policy service for cache get / set is available):

All _list returning functions are changed to also return a `drupal_listing` or 'drupal-listing' cache tag. As we have the namespace of Drupal, the chance of conflicts is very low.

drupal_listing is a cache tag we never invalidate anywhere, but it is just an indicator that there is a listing on the page. (Unless we want to use 'control' tags instead of 'tags').

It is added to auto-placeholdering conditions, but blacklisted in DPC as not disabling it. (as we introduced it, it cannot be used anywhere already so hard-coding is fine).

And that gives us the functionality for now.

@Wim: Thoughts?

The only thing we need to see before introducing this cache tag is whether it might be desirable to have a generic 'control' or info field in #cache, which we use instead.

e.g.

$build['#cache']['control'][] = 'drupal_listing';
$build['#cache']['hint'][] = 'drupal_listing';
$build['#cache']['info'][] = 'drupal_listing';

In the worst case we could say that 'drupal_listing' is experimental and contrib must not yet rely on it for anything.

I btw. ended up doing something like that for d8cache / pantheon_advanced_page_cache in Drupal 7: Remove the _list tags and replace them with _list_disabled by default, so they won't get invalidated.

So the information was still there, but the tags would not be invalidated.

Edit:

If we decide to use tags, maybe:

$build['#cache']['tags'][] = 'drupal_cache_info:listing';

or even just

$build['#cache']['tags'][] = 'cache_info:listing';

is a better namespace and more flexible for other hints to the caching system and we could explicitly blacklist those tags from entering the low-level set() / get() calls.

(Other potential uses: cache_info:once:create_placeholder, cache_info:once:do-not-cache-me, cache_info:no_big_pipe, cache_info:esi, etc.)

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

kristiaanvandeneynde’s picture

Maybe this change could take a simpler approach?

We introduce Entity::getListCacheTags() as proposed in the patch, but do not automatically try to set bundle-specific list tags anywhere. More granular list cache tags is something we probably do not want to use by default. It should be a performance tool available to people who know what they're doing. Trying to determine which granular cache tags a view is using (by looking at the filters, for instance) is asking for trouble.

The downside of dynamically generated list cache tags would be that we have an entity type key which is potentially misleading: It usually lists either one or no list cache tags, but could potentially support a dozen list tags. We could start treating it as a last resort list cache tag, though: "If all else fails, i.e. no-one specified a specific list cache tag, we use this one to make sure we don't have access issues on our hands" When clearing tags upon save, you always clear the one from the entity type definition.

Here's another real life example where allowing us to dynamically determine the list cache tags would mean a lot: In the Group module there is this notion of a "group role" entity. These roles can belong to three audiences (consider them sub-entity types): anonymous, outsider and member.

I am generating some rather expensive hashes determining a user's permissions for each audience. Being able to have a anonymous_group_role_list on top of the generic group_role_list cache tag would improve the site performance a lot as, right now, saving a new group role of any audience clears all hashes. Ideally, altering an anonymous group role would only clear the anonymous hashes.

Having Entity::getListCacheTags() would be a huge lifesaver here. Currently, the only solution is to copy everything from Entity::invalidateTagsOnSave() and Entity::invalidateTagsOnDelete() and make changes inline. Copying that much code is a huge code smell that we could fix here.

effulgentsia’s picture

Having Entity::getListCacheTags() would be a huge lifesaver here.

What would that let you do that getCacheTagsToInvalidate() doesn't?

kristiaanvandeneynde’s picture

#68 that only gets called on entity update for content entities and not at all for config entities. Doing that on a config entity would even mean the list cache tag gets set on everything that has even a single config entity in it.

gabesullice’s picture

Coming to this issue from a JSON API perspective: #2996637-15: [META] Improve collection and include performance

All of JSON API's collection endpoints are bundle specific. IOW, one can't have a listing of both article and page nodes, for example.

That means that we could very easily improve our cache hit ratio on our collection resources by using only the ENTITY_TYPE_list:BUNDLE cache tag, vs. the more generic ENTITY_TYPE_list one. JSON API doesn't suffer from the same complexities as Views does in determining which bundle tags to apply because of that fact.

It seems to me that all the complexity here is no longer about whether the bundle specific tag should exist or how to invalidate it, but in the complexity of getting Views to use it.

Similar to what @kristiaanvandeneynde proposed, can we descope this issue by first introducing the bundle-specific list cache tags and invalidation and open a followup up for Views to actually begin using it instead of the more generic one, where possible?

jibran’s picture

+1 #70.
That means that we could very easily improve our cache hit ratio on our collection resources by using only the <code>ENTITY_TYPE_list:BUNDLE cache tag, vs. the more generic ENTITY_TYPE_list one. +10000

DuaelFr’s picture

+1 #70 too
I think that's what I suggested ages ago but I suppose I didn't explain myself very well.
What could be the problem of having some cache tags that would never be used by default but still gets invalidated? I can't see any real one.

ndobromirov’s picture

+1 #70!

Wim Leers’s picture

Assigned: Unassigned » jibran
Issue tags: +Needs followup

@jibran was on the API-First Initiative call earlier today (in the middle of the night for him! 😵), and he expressed a strong interest into pushing this forward. @gabesullice and I gave him pointers about how to do that, and he was excited to do so. So, assigning to him :) Thanks @jibran! 🎉

Tagging needs followup for the Views-related follow-up per #70.

jibran’s picture

Status: Needs review » Needs work

The last submitted patch, 75: 2145751-75.patch, failed testing. View results

Wim Leers’s picture

Only 15 failures, that's great! Go @jibran! 🤘

anavarre’s picture

jibran’s picture

Status: Needs work » Needs review
Issue tags: -minor version target
FileSize
10.79 KB
28.67 KB

This is becoming more and more confusing. At this point, the ENTITY_TYPE_list is becoming the empty list tag so invalidating ENTITY_TYPE_list on node save/delete doesn't make sense anymore and because of that we are seeing all the fails in \Drupal\Tests\system\Functional\Entity\EntityCacheTagsTestBase::testReferencedEntity

Berdir’s picture

  1. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
    @@ -487,7 +487,8 @@ public function onDependencyRemoval(array $dependencies) {
        */
       protected function invalidateTagsOnSave($update) {
    -    Cache::invalidateTags($this->getEntityType()->getListCacheTags());
    +    $tags = Cache::mergeTags($this->getEntityType()->getListCacheTags(), $this->getListCacheTags());
    +    Cache::invalidateTags($tags);
    

    getListCacheTags() already includes the entity type list cache tags, so we don't need to merge here?

  2. +++ b/core/modules/system/tests/modules/entity_test/src/Controller/EntityTestController.php
    @@ -78,11 +78,8 @@ public function listEntitiesAlphabetically($entity_type_id) {
           $cache_tags = Cache::mergeTags($cache_tags, $entity->getCacheTags());
    +      $cache_tags = Cache::mergeTags($cache_tags, $entity->getListCacheTags());
         }
    -    // Always associate the list cache tag, otherwise the cached empty result
    -    // wouldn't be invalidated. This would continue to show nothing matches the
    -    // query, even though a newly created entity might match the query.
    -    $cache_tags = Cache::mergeTags($cache_tags, $entity_type_definition->getListCacheTags());
     
    

    this doesn't seem valid.

    If a list is empty then we still need the regular list cache tag because otherwise we would have no way of updating the list.

    Also, adding the list cache tags of the entities in here doesn't help us. You can *only* use those if *you* are in control of the bundles that are being shown and that's not the case for tracker, so this is not helping, we just have more tags.

    Put otherwise, adding the bundle specific cache tag is *only* useful if you can replace the complete cache tag. Which also means that merging in the full cache tag into getListCacheTags() does *not* help, we only need that when invalidating, not when adding the tag.

    And considering all that, I think the method should be called getListCacheTagsToInvalidate() because it should never be used to add cache tags to a list, only as a helper when invalidating.

jibran’s picture

FileSize
3.16 KB
29.45 KB

Fixed #80.1 and 2. Thanks for the review.

Put otherwise, adding the bundle specific cache tag is *only* useful if you can replace the complete cache tag. Which also means that merging in the full cache tag into getListCacheTags() does *not* help, we only need that when invalidating, not when adding the tag.

Can you please elaborate on this?

The last submitted patch, 79: 2145751-79.patch, failed testing. View results

Berdir’s picture

What I mean is that we always invalidate both (for example) node_list and node_list:page if you save a page node.

That means each render-cached element should only have *either* node_list or node_list:page cache tag and not both. node_list:page only helps us if it allows us to *not* add the node_list cache tag so that we only have to invalidate our list of page nodes if a page is saved and not an article.

So if you have a block that just shows the last 10 nodes of all types, then you can only use the node_list cache. Lets say of those 10, 3 are page and 7 are article. With your change in tracker, it would result in adding node_list:page and node_list:article. And if you leave out node_list itself, then creating a new "event" node for example would *not* invalidate that list.

=> You never need to get the list cache tags from a single entity (except when invalidating that entity), the list cache tags that you need to use are based on the query conditions alone. So if you add a ->condition('type', 'page') then and only then it makes sense to add node_list:page.

Which is why I'm suggesting to name the method getListCacheTagsToInvalidate(). A getListCacheTags() method would just bemisused. It *would* make sense to have that on the query object, that could be be interesting, but we can do that later and just like in views, it would be surprisingly hard to get right)

jibran’s picture

Let's see if I get this right, you are suggesting:

  • If the node listing is empty then add node_list tag.
  • If the node listing is only showing single bundle(page) then add node_list:page tag.
  • If the node listing is showing more than one bundle then add node_list tag.

Also:

  • Rename getListCacheTags to getListCacheTagsToInvalidate.
  • Add getListCacheTags in the follow up where we determine bundle specific cache tag based on query object.

Status: Needs review » Needs work

The last submitted patch, 81: 2145751-81.patch, failed testing. View results

jibran’s picture

Title: Introduce ENTITY_TYPE_list:BUNDLE cache tag » Introduce ENTITY_TYPE_list:BUNDLE cache tag and add it to single bundle listing
Status: Needs work » Needs review
FileSize
4.26 KB

A new start after discussing it with @Berdir.

Status: Needs review » Needs work

The last submitted patch, 86: 2145751-86.patch, failed testing. View results

kristiaanvandeneynde’s picture

To clarify, in order to maintain BC:

  • We cannot change the generic list cache tag that is defined in core
  • We can introduce new (extra) list cache tags per bundle and make sure they are also cleared when an entity of that bundle changes
  • We should not start using this cache tag in core already, to keep the scope of this issue as narrow as possible

This allows module maintainers or site developers to choose to replace the very generic ENTITY_TYPE_ID_list with one or more bundle-specific one(s) so their list's cache object doesn't get flushed all the time.

The patch in #86 seems to be doing exactly that. So great!

Berdir’s picture

It's not even that much about BC, it's simply about actually being correct or not :)

*If* core has lists that really are limited to a specific bundle then I do believe we could optimize that, but I doubt that there are many cases of that. E.g. we could update the node_list cache tags to be include the node types that are enabled for book-functionality, but all of that should IMHO be a follow-up, for this, we should really just do the invalidation and a test implementation/usage here.

jibran’s picture

Still working on it. I'll post the patch once its green.

kim.pepper’s picture

Issue tags: +DrupalSouth 2018
jibran’s picture

Status: Needs work » Needs review
FileSize
7.94 KB
9.65 KB

Implemented #86 properly.

  1. +++ b/core/lib/Drupal/Core/Entity/Entity.php
    @@ -488,12 +488,25 @@ public function getCacheContexts() {
    +      $tags = Cache::mergeTags($tags, $bundle_tags);
    

    I think this is correct we'd always want to invalidate all the listing tags but after doing this we can't reliable use $entity->getCacheTagsToInvalidate() to set tags on listing pages. For example see the changes in \Drupal\entity_test\Controller\EntityTestController::listEntitiesAlphabetically().

  2. +++ b/core/modules/system/tests/src/Functional/Entity/EntityCacheTagsTestBase.php
    @@ -587,10 +587,10 @@ public function testReferencedEntity() {
    -    // there is a cache miss for both the empty entity listing and the non-empty
    -    // entity listing routes, but not for other routes.
    ...
    -    Cache::invalidateTags($this->entity->getEntityType()->getListCacheTags());
    

    I think we can expand this test for just entity_list tags.

I'm away until next year so feel free to pick up the patch.

Berdir’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/Entity/Entity.php
    @@ -488,12 +488,25 @@ public function getCacheContexts() {
    +    if ($this->bundle() != $this->entityTypeId) {
    

    the correct check for this $this->getEntityType()->hasKey('bundle'), it is absolutely possible and valid to create a node type with a "node" machine name and then this would break.

  2. +++ b/core/lib/Drupal/Core/Entity/EntityInterface.php
    @@ -424,6 +424,13 @@ public function referencedEntities();
    +  /**
    +   * The list cache tags to invalidate for this entity.
    +   *
    +   * @return string[]
    +   */
    +  public function getListCacheTagsToInvalidate();
    

    We need clear documentation on this. It's practically an internal method as it should *only* be called when you are invalidating tags and never when you are adding tags to a response.

  3. +++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
    @@ -391,17 +391,17 @@ public function resetCache(array $entities = NULL) {
           foreach ($entities as $entity) {
             $tags = Cache::mergeTags($tags, $entity->getCacheTags());
    -        $tags = Cache::mergeTags($tags, $entity->getEntityType()->getListCacheTags());
    +        $tags = Cache::mergeTags($tags, $entity->getListCacheTagsToInvalidate());
           }
    -      Cache::invalidateTags($tags);
         }
         else {
    -      Cache::invalidateTags($this->getCacheTags());
    +      $tags = Cache::mergeTags($this->getCacheTags(), $this->entityType->getListCacheTags());
         }
    +    Cache::invalidateTags($tags);
       }
    

    Like this for example. Just move the getListCacheTags() call out of the else and always do it, calling this per-entity was always unnecessary and means we have to merge the same cache tags together on each loop.

    and the list anyway always contains node_list. There is no benefit at all to also add node_list:article, that just means we have to check more cache tags on a cache hit.

  4. +++ b/core/modules/block/src/BlockViewBuilder.php
    @@ -83,6 +83,7 @@ public function viewMultiple(array $entities = [], $view_mode = 'full', $langcod
     
           $cache_tags = Cache::mergeTags($this->getCacheTags(), $entity->getCacheTags());
    +      $cache_tags = Cache::mergeTags($cache_tags, $entity->getListCacheTagsToInvalidate());
           $cache_tags = Cache::mergeTags($cache_tags, $plugin->getCacheTags());
    

    Same here I think, why do we need this now if it wasn't present before?

    We just get a bunch of block entities to render, we don't know based in which condition they have been selected.

  5. +++ b/core/modules/system/tests/modules/entity_test/src/Controller/EntityTestController.php
    @@ -75,14 +75,22 @@ public function listEntitiesAlphabetically($entity_type_id) {
         $labels = [];
    +    $list_tags = [];
         foreach ($entities as $entity) {
           $labels[] = $entity->label();
    +      $list_tags[$entity->bundle()] = $entity->getListCacheTagsToInvalidate();
           $cache_tags = Cache::mergeTags($cache_tags, $entity->getCacheTags());
         }
    -    // Always associate the list cache tag, otherwise the cached empty result
    -    // wouldn't be invalidated. This would continue to show nothing matches the
    -    // query, even though a newly created entity might match the query.
    -    $cache_tags = Cache::mergeTags($cache_tags, $entity_type_definition->getListCacheTags());
    +    if (empty($entities) || count($list_tags) > 1 ) {
    +      // Always associate the list cache tag with empty result, otherwise the
    +      // cached empty result wouldn't be invalidated.
    +      // If there are entities with more than one bundle on the page then don't
    +      // add bundle specific tags.
    

    This can all be reverted.

    What we need is a new method, something like listEntitiesAlphabeticallyByBundle(), and *only* then, with a specific, requested bundle, we can do a cache tag for that.

    And then a test that looks at the patch and verifies it only gets invalidated if an entity from the matching bundle is created/deleted/updated.

jibran’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests
FileSize
6.18 KB
6.32 KB

Addressed all 5 points from #93. Didn't fix the failing test. Bundle specfic tests are pending as well.

Status: Needs review » Needs work

The last submitted patch, 94: 2145751-94.patch, failed testing. View results

Wim Leers’s picture

Exciting progress! 🎉

Berdir’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
@@ -391,17 +391,14 @@ public function resetCache(array $entities = NULL) {
     // be able to optimize this further.
+    $tags = [];
     if (isset($entities)) {
-      $tags = [];
       foreach ($entities as $entity) {
         $tags = Cache::mergeTags($tags, $entity->getCacheTags());
-        $tags = Cache::mergeTags($tags, $entity->getEntityType()->getListCacheTags());
       }
-      Cache::invalidateTags($tags);
-    }
-    else {
-      Cache::invalidateTags($this->getCacheTags());
     }
+    $tags = Cache::mergeTags($tags, $this->entityType->getListCacheTags());
+    Cache::invalidateTags($tags);
   }

I think we lost the call to $this->getCacheTags() here.

While the logic here is a bit inefficient, we should probably just revert it completely as nothing here is actually required anymore for this issue.

jibran’s picture

Status: Needs work » Needs review
FileSize
1.03 KB
5.29 KB

Fixed #97.

jibran’s picture

Assigned: jibran » Unassigned
Issue summary: View changes
Status: Needs review » Needs work
Issue tags: -Needs issue summary update

Green! NW for bundle specfic tests as described in #93:

What we need is a new method, something like listEntitiesAlphabeticallyByBundle(), and *only* then, with a specific, requested bundle, we can do a cache tag for that.

And then a test that looks at the patch and verifies it only gets invalidated if an entity from the matching bundle is created/deleted/updated.

Also updated IS.

kristiaanvandeneynde’s picture

  1. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
    @@ -497,7 +497,11 @@ protected function invalidateTagsOnSave($update) {
       protected static function invalidateTagsOnDelete(EntityTypeInterface $entity_type, array $entities) {
    -    Cache::invalidateTags($entity_type->getListCacheTags());
    +    $tags = $entity_type->getListCacheTags();
    +    foreach ($entities as $entity) {
    +      $tags = Cache::mergeTags($tags, $entity->getListCacheTagsToInvalidate());
    +    }
    +    Cache::invalidateTags($tags);
       }
    

    Isn't it a bit redundant to ask for the entity type's list cache tags here? Those will be part of $entity->getListCacheTagsToInvalidate().

  2. +++ b/core/lib/Drupal/Core/Entity/Entity.php
    @@ -492,12 +492,25 @@ public function getCacheContexts() {
       public function getCacheTagsToInvalidate() {
    -    // @todo Add bundle-specific listing cache tag?
    -    //   https://www.drupal.org/node/2145751
         if ($this->isNew()) {
           return [];
         }
    

    Couldn't we move the list cache tags logic here? Both ::invalidateTagsOnSave() and ::invalidateTagsOnDelete() manually add the list cache tags and then the entity tags. The if ($update) part of ::invalidateTagsOnSave() isn't necessary because ::getCacheTagsToInvalidate() checks for $entity->isNew().

By calling ::getListCacheTagsToInvalidate() in ::getCacheTagsToInvalidate(), we can simplify ::invalidateTagsOnSave(), ::invalidateTagsOnDelete() and ConfigEntityBase::invalidateTagsOnDelete().

You could argue invalidating an entity's tags would always invalidate the list, so there's no change there. Our implementations actually do so already. And you'd still be able to get an entity's list cache tags by manually calling ::getListCacheTagsToInvalidate(), even though I don't see the need for that.

johnchque’s picture

Assigned: Unassigned » johnchque

I will work a bit on tests, which are also needed here. :)

johnchque’s picture

Status: Needs work » Needs review
FileSize
9.75 KB
3.92 KB

Adding tests for this. :) Wondering if we need to update the IS in the part on how the tags should behave.

We can always use the default-bundle cache tag doesn't matter if we have results or not, right? (If we have a condition for a specific bundle).

BTW thanks @Berdir for helping with the patch.

Berdir’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/system/tests/modules/cache_test/src/Controller/CacheTestController.php
    @@ -19,4 +19,23 @@ public function urlBubbling() {
    +    $page['#cache']['tags'] = [$entity_type . '_list:' . $bundle];
    

    Wondering what to do about this exactly.

    As I mentioned, a possible follow would be to make the entity query class generate this, if it sees a condition on the bundle field, then it would return this cache tag, otherwise the generic one, but that's not trivial and definitely a follow-up.

    Another option would be to have a buildBundleListCacheTag($entity_type_id, $bundle) method somewhere, possibly static. We already have \Drupal\Core\Cache\Cache::buildTags(), and we have \Drupal\Core\Cache\Cache::keyFromQuery(), so it *could* be there, but I think it would be weird to have an (implicit) dependency on the entity sstem in Drupal\Core\Cache. So maybe on the entity class?

  2. +++ b/core/tests/Drupal/FunctionalTests/Entity/EntityBundleListCacheTest.php
    @@ -0,0 +1,70 @@
    +    // Check that tags are not invalidated after creating an entity of a
    +    // different bundle than the current in the request.
    +    $url = Url::fromRoute('cache_test_list.bundle_tags', ['entity_type' => 'entity_test_with_bundle', 'bundle' => 'bundle_b']);
    +    $this->drupalGet($url);
    +    $this->assertSession()->responseHeaderEquals('X-Drupal-Cache', 'MISS');
    +    $this->drupalGet($url);
    +    $this->assertSession()->responseHeaderEquals('X-Drupal-Cache', 'HIT');
    +    $entity1 = EntityTestWithBundle::create(['type' => 'bundle_a', 'name' => 'entity2']);
    +    $entity1->save();
    +    $this->drupalGet($url);
    +    $this->assertSession()->responseHeaderEquals('X-Drupal-Cache', 'HIT');
    

    I would merge the two cases together, make $bundle_a_url and $bundle_b_url and then always check both urls directly after each other.

    Then you only have to create one new entity.

johnchque’s picture

Status: Needs work » Needs review
FileSize
9.71 KB
2.77 KB

Updating tests. Agree with 1. in #103. Open to discuss, I would imagine having a buildBundleListCacheTag method. Any other opinions?

Wim Leers’s picture

#100.1: +1 — getListCacheTags() is now called twice. Actually, no, this is correct. getListCacheTags() is called even if there are no entities to invalidate. And when there are entities to invalidate, the bundle-specific list tags are also added.

#100's By calling […] we can simplify […] — AFAICT that wouldn't work because ConfigEntityBase is a special beast. Look at ConfigEntityBase's overrides for invalidateTagsOnSave() and invalidateTagsOnDelete(). Look at the docblocks specifically. They explain why ::getCacheTagsToInvalidate() of the parent class is not called in their case. Which is why #100.2 would not work.

  1. +++ b/core/lib/Drupal/Core/Entity/Entity.php
    @@ -495,9 +495,22 @@ public function getCacheContexts() {
    +    $tags = $this->getEntityType()->getListCacheTags();
    ...
    +      foreach ($tags as $key => $tag) {
    +        $bundle_tags[$key] = $tag . ':' . $this->bundle();
    

    This is wrong; it makes too many assumptions. For example, for \Drupal\shortcut\Entity\Shortcut, the list cache tag is overridden to be config:shortcut_set_list. Appending :BUNDLE to it doesn't make sense. Similarly, some entity types might use rendered.

    What if … \Drupal\Core\Entity\EntityType::__construct() did not do:

        // Ensure a default list cache tag is set.
        if (empty($this->list_cache_tags)) {
          $this->list_cache_tags = [$definition['id'] . '_list'];
        }
    

    but:

        // Ensure a default list cache tag is set.
        if (empty($this->list_cache_tags)) {
          $this->list_cache_tags = [ 
            $definition['id'] . '_list',
            $definition['id'] . '_list:<BUNDLE>',
          ];
        }
    

    … and we'd make \Drupal\Core\Entity\EntityType::getListCacheTags() automatically omit any tag that includes the string <BUNDLE>, and then let this new ::getListCacheTagsToInvalidate() replace it with the appropriate bundle?

    That way we'd only be generating bundle list cache tags when necessary.

  2. +++ b/core/lib/Drupal/Core/Entity/EntityInterface.php
    @@ -425,6 +425,17 @@ public function referencedEntities();
    +   * @internal
    +   *   It's an internal method, should only be called when for the tags
    +   *   invalidation, and is not used for adding tags to a response.
    

    If it's internal, why are we adding it to the interface?

Wim Leers’s picture

#103.1: Yeah I'd also like this not to be manually generated. I don't like buildBundleListCacheTag(), because of that implicit dependency on the entity system in \Drupal\Core\Cache you mention. I would then prefer something like EntityType::getGranularListCacheTagsForEntities(EntityInterface[] $entities) or EntityType::getGranularListCacheTagsForQuery(\Drupal\Core\Entity\Query\QueryInterface $query).

mpp’s picture

Status: Needs review » Needs work

I applied the latest patch and I'm getting errors on search_api: /en/admin/config/search/search-api/index/solr

PHP Fatal error: Class Drupal\\search_api\\UnsavedIndexConfiguration contains 1 abstract method and must therefore be declared abstract or implement the remaining methods (Drupal\\Core\\Entity\\EntityInterface::getListCacheTagsToInvalidate) in search_api/src/UnsavedIndexConfiguration.php on line 22

Berdir’s picture

Yes, that is correct, search_api will need to be updated, nothing we can do about that. We are allowed to add new methods to EntityInterface, but search_api (just like views_ui in core) has an implementation that doesn't inherit from the base class.

Leaving at needs work for the other feedback.

mpp’s picture

Issue summary: View changes

Thanks @Berdir, adding a task to the todo list in this issue.

johnchque’s picture

Status: Needs work » Needs review
FileSize
11.63 KB
2.38 KB

Was this what you meant @Wim Leers? Adding a method for returning the bundle cache tags with the pattern. Because we previously called to getListCacheTags inside getListCacheTagsToInvalidate.

Status: Needs review » Needs work

The last submitted patch, 110: 2145751-110.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

johnchque’s picture

Status: Needs work » Needs review
FileSize
11.6 KB
1.22 KB

Making some code fixes.

Status: Needs review » Needs work

The last submitted patch, 112: 2145751-112.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Nixou’s picture

yogeshmpawar’s picture

Assigned: johnchque » Unassigned
Status: Needs work » Needs review

Setting back to Needs Review & Triggering bots.

Status: Needs review » Needs work

The last submitted patch, 115: 2145751-115.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

btully’s picture

Is there a recommended patch to use? Anyone have this working?

mbovan’s picture

Status: Needs work » Needs review
FileSize
11.85 KB
4.26 KB

Reroll and code style adjustments/fixes.

Status: Needs review » Needs work

The last submitted patch, 120: 2145751-120.patch, failed testing. View results

Berdir’s picture

Status: Needs work » Needs review
FileSize
15.24 KB
7.21 KB

Fixing tests and improving the implementation. because it merged and only did the replacement if there was a bundle entity key, we always had one extra cache tag that still had the placeholder on invalidate.

I improved the code to just return the list cache tags if an entity type doesn't have bundles and also actually replace the cache tag with the placeholder instead of merging it all together.

Also the route parameter and controller argument didn't match anymore because @mbovan renamed it :)

jibran’s picture

Issue tags: -Needs tests

Yay! green. Removing need tests tag as we have tests now.

  1. +++ b/core/modules/system/tests/modules/cache_test/src/Controller/CacheTestController.php
    @@ -19,4 +19,30 @@ public function urlBubbling() {
    +    $entities = $storage->loadMultiple($entity_ids);
    ...
    +    $page['#cache']['tags'] = [$entity_type_id . '_list:' . $bundle];
    

    We should add an empty listing condition here so that we can assert empty listing cache tags.

  2. +++ b/core/tests/Drupal/FunctionalTests/Entity/EntityBundleListCacheTest.php
    @@ -0,0 +1,80 @@
    +    // Check that tags are invalidated after creating an entity of the current
    +    // bundle.
    

    We should add asserts for entity1->delete() as well. We should also make sure after deleting all the entities of a bundle should give us a non-bundle list tag given we implement 1.

Berdir’s picture

Status: Needs review » Needs work

1. Not sure what you mean by this. We already test going from a list that has no entries to having entries, we first assert the empty list and only afterwards create an entity?

2. Agreed, we should test delete, but delete in this case is also covered by the cache tag of the specific entity. I think it might make more sense to test this explicitly in the unit test like I did save. For the functional test to be meaningful, we'd need a scenario with a pager, where we delete an entity that is not on the current page. Not sure if that's worth it? We could set pager to 2 per page (and sort alphabetically, for easy testing?), create 4 entities and then delete one one from the first page, updating should result in invalidating this page too. But this feels like we're more testing the test controller than the actual implementation.

jibran’s picture

#124.1: I meant something like

diff --git a/core/modules/system/tests/modules/cache_test/src/Controller/CacheTestController.php b/core/modules/system/tests/modules/cache_test/src/Controller/CacheTestController.php
index b773d5bb5a..cde84f9421 100644
--- a/core/modules/system/tests/modules/cache_test/src/Controller/CacheTestController.php
+++ b/core/modules/system/tests/modules/cache_test/src/Controller/CacheTestController.php
@@ -2,6 +2,7 @@
 
 namespace Drupal\cache_test\Controller;
 
+use Drupal\Core\Cache\CacheableMetadata;
 use Drupal\Core\Url;
 
 /**
@@ -40,8 +41,14 @@ public function bundleTags($entity_type_id, $bundle) {
       $page[$entity->id()] = [
         '#markup' => $entity->label(),
       ];
+      CacheableMetadata::createFromObject($entity)->applyTo($page[$entity->id()]);
+    }
+    if (empty($entities)) {
+      $page['#cache']['tags'] = [$entity_type_id . '_list'];
+    }
+    else {
+      $page['#cache']['tags'] = [$entity_type_id . '_list:' . $bundle];
     }
-    $page['#cache']['tags'] = [$entity_type_id . '_list:' . $bundle];
     return $page;
   }
 
Berdir’s picture

No, that's not necessary, why would we do that?

If you have a list of articles, then it should only have the article cache tag. Even if there are no articles right now.

The cache tag that you add has nothing to do with the entities that you might have, *only* with the conditions you define. You'd only need this logic if you would actually fall back to a secondary query if there's nothing to show that would not have that condition.

golddragon007’s picture

Guys, why was this decision made?: "If the node listing is showing more than one bundle then add node_list tag." Is there a specific reason?

Just because it's really doesn't feel good. For example, I have a site where there are 10 content types, if almost all of my views has two or three content types listed, then there's 70-80% chance almost all of my views will be invalidated, which is not good if your site is huge. In that case, we are in the same shoes as without these modifications which you guys did.

What I suggest:
If the node listing is showing more than one bundle then add all the listed bundle's tag. (i.e. node_list:page node_list:article)

Berdir’s picture

No such decision was made. This issue just adds those default cache tags, they are not yet used by default, you still have to actually use them in code or with https://www.drupal.org/project/views_custom_cache_tag/.

So yes, a better and more generic documentation will instead document that it's about cost of checking cache tags vs. too broad validations. It depends on your use case, if you have a list with 2-3 rarely used types, it's still beneficial to use them, but if they are the majority of your content, then at some point it's more efficient to just use node_list because it's fewer cache tags to check for invalidations.

The more important part that we need to document is that the cache tags need to be based on the *conditions* of the query, and not the result. If you list all nodes on a view, it doesn't matter if there are currently only articles showing up, the correct cache tag is node_list.

golddragon007’s picture

Yes, there is a point, but it's hard to tell where is it. It really depends on multiple variables, like:
- context: is it cached by the route and/or user and/or language?
- how complex is the view (incl. custom fields)
- how many data you have to work on
- how frequent you create some content types (diversity of the content)
- how many views you have
- how many visitors you have

Maybe for fast views, it's enough up to 2-3 cache tags on frequent content types.
Maybe for slow views, it's needed up to 4-7 cache tags on frequent content types.

And of course, it also depends on Drupal how it looks for the cache tags and how it stores.
And probably a test needed for this.

Berdir’s picture

> And probably a test needed for this.

No, that's not something we can test, that's something that people will have to decide on their own, based on good documentation. Only if we actually have coder that needs to consider something like that (e.g. an assumption in a node type filter that would use a different cache tag based on on whether you have one type selected or 10).

Nixou’s picture

Thanks you a lot for your work on this patch.

I created a patch for Views, based on yours in the issue : https://www.drupal.org/project/drupal/issues/3055371#comment-13372445.

It seems to work well.

arpad.rozsa’s picture

Status: Needs work » Needs review
FileSize
16.16 KB
1.2 KB

Added a testPostDeleteBundle() to the unit test to check for bundle entity keys in cache tags, similarly to the postSave tests.

sinn’s picture

There is an error in search_api after applying #122:

Fatal error: Class Drupal\search_api\UnsavedIndexConfiguration contains 1 abstract method and must therefore be declared abstract or implement the remaining methods (Drupal\Core\Entity\EntityInterface::getListCacheTagsToInvalidate) in /docroot/modules/contrib/search_api/src/UnsavedIndexConfiguration.php on line 22

Ticket has been created https://www.drupal.org/project/search_api/issues/3102151

Berdir’s picture

We are allowed to add methods to EntityInterface due to the 1:1 rule. Maybe there is a way to avoid that though. Could we add an optional $entity argument to getListCacheTags()? Might be a bit weird but worth a try.

arpad.rozsa’s picture

Instead of removing the getListCacheTagsToInvalidate() method, first trying if it works as a protected method, which also solves the problem for search api.

Berdir’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityType.php
@@ -335,7 +335,10 @@ public function __construct($definition) {
     // Ensure a default list cache tag is set.
     if (empty($this->list_cache_tags)) {
-      $this->list_cache_tags = [$definition['id'] . '_list'];
+      $this->list_cache_tags = [
+        $definition['id'] . '_list',
+        $definition['id'] . '_list:<BUNDLE>',
+      ];
     }
   }
 
@@ -863,6 +866,19 @@ public function getListCacheContexts() {

@@ -863,6 +866,19 @@ public function getListCacheContexts() {
    * {@inheritdoc}
    */
   public function getListCacheTags() {
+    $list_cache_tags = [];
+    foreach ($this->list_cache_tags as $tag) {
+      if (strpos($tag, '<BUNDLE>') === FALSE) {
+        $list_cache_tags[] = $tag;
+      }
+    }
+    return $list_cache_tags;
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function getBundleListCacheTagPattern() {
     return $this->list_cache_tags;

I'm still not sure about this pattern.

We add a default to the list cache tags, only to filter it out again in the existing method, and awkwardly replace the placeholder in a loop in the new method.

One effect of this is that someone who customized the list cache tag doesn't get the bundle, and whether or not that is a good thing.

The other question is whether we need this to be customizable per entity type like the list cache tag. Is there a use case for not having a bundle list cache tag if you have bundles or to have more than one?

We could have a separate new property for the pattern if we want to support that or just inline it into the method, if someone really doesn't want it, they could override the method?

The cost for having it is fairly small, it's adding one extra merge query on entity save (with the database backend). If we are worried about that then maybe we shouldn't add it by default and only opt-in for nodes, where per-bundle lists are common.

sinn’s picture

Removing getListCacheTagsToInvalidate() from EntityInterface.php in #135 helped for search_api.

geek-merlin’s picture

#134 @berdir:
> We are allowed to add methods to EntityInterface due to the 1:1 rule.

I could not find that by that name, can you give a pointer?

Berdir’s picture

https://www.drupal.org/core/d8-bc-policy:

> Where there is a 1-1 relationship between a class and an interface, inherit from the class. Where a base class or trait is provided, use those. This way your class should inherit new methods from the class or trait when they're added.

Berdir’s picture

Ok, here's an attempt at reducing this to a minimum. Instead of the new bundle cache tag placeholder system, method on EntityType and so on, this just hardcodes the bundle list cache tag logic directly into \Drupal\Core\Entity\EntityBase::getListCacheTagsToInvalidate().

Before, because we only set the bundle cache tag, it didn't affect entity types that had a customized list cache tags, now it does. In core, this specifically affects shortcuts. And I think that's actually useful, as a follow-up, we could likely use that in the shortcut module to further optimize it after #2914110: Shortcut hook_toolbar implementation makes all pages uncacheable lands.

In contrib, it affects also the webform_submission entity type, that too will get the list cache tag an it could be useful there as well as listing submissions of a specific webform is a valid use case.

One option to have a better API to use bundle list cache tags would be to add a getBundleListCacheTags($bundle) method to EntityType, then it would be easier to use in places like \Drupal\cache_test\Controller\CacheTestController::bundleTags().

Status: Needs review » Needs work

The last submitted patch, 140: 2145751-140.patch, failed testing. View results

Berdir’s picture

Status: Needs work » Needs review
FileSize
10.68 KB
861 bytes

Fixing that last test fail.

Wim Leers’s picture

Looks good, @Berdir! :)

I have only one question:

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
@@ -500,7 +500,11 @@ protected function invalidateTagsOnSave($update) {
-    Cache::invalidateTags($entity_type->getListCacheTags());
+    $tags = $entity_type->getListCacheTags();
+    foreach ($entities as $entity) {
+      $tags = Cache::mergeTags($tags, $entity->getListCacheTagsToInvalidate());
+    }
+    Cache::invalidateTags($tags);

🤔 Why can't this be just

Cache::invalidateTags($entity->getListCacheTagsToInvalidate());

?

Berdir’s picture

I'm not sure if I understand the question. Yes, we could skip getListCacheTags() as that's already included, and we could also call it directly on each entity and skip the merging, but only because we rely on the optimization for skipping to invalidating the same cache tag twice. It's the same implementation as the parent, just without the single-entity cache tag.

Wim Leers’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs change record

Yes, we could skip getListCacheTags() as that's already included

That's all I was pointing out.

It's the same implementation as the parent, just without the single-entity cache tag.

Unless I'm misreading, I don't think it is? Your patch in #142 is also doing
I was misreading! I didn't spot the difference between $this->getListCacheTagsToInvalidate() and $entity->getListCacheTagsToInvalidate() — my bad :)

In that case, I think this is ready! 🥳

catch’s picture

Issue summary: View changes
Issue tags: +8.9.0 release notes

Created a follow-up for Views #3103865: Use the bundle list cache tag in views.

Also removed the part of the issue summary on using them since that's all going to be handled in follow-ups.

Was going to take a stab at the change record, but couldn't really think of anything to say except the issue title, did that for the release notes snippet though.

maximpodorov’s picture

With the patch #142, sometimes I see such error in the logs:

Uncaught PHP Exception Drupal\Core\Entity\EntityStorageException: "SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry 'node_list:market' for key 'PRIMARY': INSERT INTO {cachetags} (invalidations, tag) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1); Array
(
[:db_insert_placeholder_0] => 1
[:db_insert_placeholder_1] => node_list:market
)
" at docroot/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php line 847

This problem appears when multiple entities (with several translations) are created programmatically during a single request.

Berdir’s picture

Sounds like a race condition when two nodes of the same time are created at the same time, after for some reason the cache tags have been emptied? that's not really something that we can do anything about here, not sure why it happens with that but not node_list?

maximpodorov’s picture

Is this possible if some code invalidates just 'node_list' tag (the hardcoded value) and not 'node_list:bundle' tag?

maximpodorov’s picture

More precisely, what if some code invalidates tags returned by getCacheTags(), not by getCacheTagsToInvalidate() ?

E.g. \Drupal\Core\Entity\EntityViewBuilder::resetCache() does this.
https://git.drupalcode.org/project/drupal/blob/8.8.x/core/lib/Drupal/Cor...

Berdir’s picture

That is problematic, but that method is a weird leftover anyway and is hardly ever used. It definitely wouldn't cause your issue, quite the opposite.

geek-merlin’s picture

@maximpodorov #147: I recently looke through the cachetag code and was convinced that it is buggy and something like this could happen... but tbh was distracted by something more pressing. If you open a separate issue (it's probably not related to this but the odds this happens might be increased) and crosslink it here, i'll add my 5 cents there.

  • catch committed e68ca93 on 9.0.x
    Issue #2145751 by jibran, yongt9412, DuaelFr, Berdir, pnagornyak, arpad....
catch’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs change record

OK I found something to say in a change record: https://www.drupal.org/node/3107058

Committed e68ca93 and pushed to 9.0.x (and cherry-picked to 8.9.x). Thanks!

  • catch committed a7eacc8 on 8.9.x
    Issue #2145751 by jibran, yongt9412, DuaelFr, Berdir, pnagornyak, arpad....
init90’s picture

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

xjm’s picture

Status: Closed (fixed) » Needs work

This is tagged for the 8.9.0 release notes, but the release note in the IS of:

Added an ENTITY_TYPE_list:BUNDLE cache tag.

...Doesn't sound like anything remotely close to important update information about a disruptive change.

Was this supposed to be tagged as a highlight instead, or is there a disruptive impact we should be documenting?

jibran’s picture

Doesn't sound like anything remotely close to important update information about a disruptive change.

It is not a disruptive change. It is just a new feature. It is there if you want to use it. I think that might be the idea behind tagging it?

catch’s picture

Re-tagging as a highlight.

Berdir’s picture

Issue summary: View changes
Status: Needs work » Fixed

I've tried to add a short highlight snippet. There's also the change notice and I've now also updated the documentation page to mention bundle cache tags as well as how to define custom, more specific ones: https://www.drupal.org/docs/8/api/cache-api/cache-tags#s-common-cache-tags

I've also added a reference to my Views Custom Cache Tag module to the documentation and release note. I do think that using that is *very* beneficial for sites specific content lists and a lot of changes/new content. Feel free to ignore that, but it's not like I'm benefitting from it being there :)

geek-merlin’s picture

🎉!

>I've also added a reference to my Views Custom Cache Tag module to the documentation and release note.

This module is a huge gift 🤩 for optimizing any more complex site. If you would not have done it, i'd now add it.

laurent.lacote’s picture

Hi all!

First of all, thank you very much for bringing this. It was a long-awaited feature for the project(s) I'm working on.

However, while the Drupal.org page describing cache tags makes mention of this ENTITY_TYPE_list:bundle being usable (which triggered my pulse to search and find the ticket relating to it to see when it arrived), I don't find any mention of it being actually available already: didn't see this ticket mentioned in 8.8.x releases notes, and I don't see the related code either in the EntityType class.

So unless I did not look hard enough (possible ^^), my understanding is that while doc has been updated, code is currently only available on 8.9-dev.
Is there any chance that specific improvement would be included in a potential next-to-come 8.8.x release?

Otherwise, what would be the best way for us to "pre-integrate" that in our current code? Would a diff-patch just targeting the EntityType class from current 8.9 be enough (sorry if question seems trivial, not quite familiar yet with the heart of Drupal ^^)? I guess it wouldn't be enough, I'd have to also patch the files taking care of the invalidation?

Thanks in advance for your feedback.

Berdir’s picture

Yes, this will only be available starting with 8.9.0, it will not be backported to 8.8.

(Note: 8.9 *is* the next and last minor version for Drupal 8, what you probably meant is a patch release for 8.8, that won't happen).

To use this earlier, you can apply it as a patch, not sure if the most recent one will apply against Drupal 8.8, you might need to use an older one maybe. See https://www.mediacurrent.com/blog/composer-patches-dependency-your-proje....

laurent.lacote’s picture

Thank you @berdir for the confirmation and lead on patching.

It would be nice if we could just wait for 8.9 to be released to upgrade & adjust, but our timeframe does not give us this freedom. ^^
Doesn't prevent me to be hyped on when it will be published. Keep up the good work and stay healthy everyone ! ;)

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.