Introduce EntityBase::getCacheTagsToInvalidateOnSave()

The Needs Review Queue Bot → tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Reroll and removed unused use statement.

Added a draft change record.

Change record looks super clear, thanks @lauriii!

Committed/pushed to 10.1.x, thanks!

Is there a reason for the naming inconsistency between getCacheTagsToInvalidateOnSave and getTagsToInvalidateOnDelete? Why not getCacheTagsToInvalidateOnDelete?

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
@@ -490,12 +489,8 @@ protected function invalidateTagsOnSave($update) {
    */
-  protected static function invalidateTagsOnDelete(EntityTypeInterface $entity_type, array $entities) {
-    $tags = $entity_type->getListCacheTags();
-    foreach ($entities as $entity) {
-      $tags = Cache::mergeTags($tags, $entity->getListCacheTagsToInvalidate());
-    }
-    Cache::invalidateTags($tags);
+  protected static function getTagsToInvalidateOnDelete(EntityTypeInterface $entity_type, array $entities) {
+    return $entity_type->getListCacheTags();
   }

this dropped the call to $entity->getListCacheTagsToInvalidate(). This happened between #21 and #26, where that call was introduced in HEAD, but the reroll dropped it. I don't think it was deliberate.

Since there are no config entities with bundles, there are no failing tests, but there's still a possibility that someone implemented that method and that no longer works correctly now?

Tags vs CacheTags is a bit inconsistent indeed, but the existing/replaced methods also used just Tags and not CacheTags, so I think that's fine.

Agreed with both points from #39 (although config entities with bundles is a terrifying idea). Rolling back so we can re-commit with the extra getListCacheTagsToInvalidate() and I think if we want to address the method naming mismatch that could happen in a follow-up to deprecate the old methods and add new ones.

Just made a new reroll and now the getListCacheTagsToInvalidate is back. Moving back to NR.

That looks great. Since I already committed without this, moving back to RTBC, and I'll commit in a couple of days if no further comments.

FWIW, I don't think this implements what the original proposal was, which said a single shared method between the two? The fact that it's still two separate methods *and* that there doesn't seem to be a use case in core to override this makes me question a bit how useful this really is?

The last submitted patch, 42: 3070022-42.patch, failed testing. View results →

Change CR back again to draft.