Problem/Motivation

ConfigEntityBase::disable() does an unnecessary cache tag invalidation that can be just removed.

Item.php in aggregator.module should only be called for a new entity, so !$update like e.g. Block.php

Proposed resolution

Remove and add the check.

Remaining tasks

User interface changes

API changes

Data model changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers’s picture

Issue tags: +D8 cacheability
jespermb’s picture

Status: Active » Needs review
FileSize
1.17 KB

Hi, I have removed the mentioned cache check and added the if statement to aggregator.

Status: Needs review » Needs work
Berdir’s picture

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
@@ -193,7 +193,6 @@ public function enable() {
   public function disable() {
     // An entity was disabled, invalidate its own cache tag.
-    Cache::invalidateTags($this->getCacheTags());
     return $this->setStatus(FALSE);

The comment also needs to be removed.

And apparently there is a unit test for this, so the assertions in that method there need to be updated/removed.

jespermb’s picture

Status: Needs work » Needs review
FileSize
1.97 KB

Comment removed and test updated. I hope it has been correctly changed, my experience with automated tests is limited unfortunetly.

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me.

For the record, the performance impact is close to zero since the default cache checksum implementation has a static cache to avoid repeated invalidations. But especially the disable() call is confusing and others might use that as a bad example.

Status: Reviewed & tested by the community » Needs work
joshi.rohit100’s picture

Status: Needs work » Needs review
FileSize
2.01 KB

re-rolled the patch as previous was unable to apply.

borisson_’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC as the patch now does apply and tests pass.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
@@ -192,8 +192,6 @@ public function enable() {
-    // An entity was disabled, invalidate its own cache tag.
-    Cache::invalidateTags($this->getCacheTagsToInvalidate());

Are we sure about this?

Berdir’s picture

Status: Needs work » Reviewed & tested by the community

Yes, very sure.

I don't see how that ever made any sense. enable() itself just changes a property on the entity. nothing happens until you actually save it. And then we invalidate again. Always.

So if something actually is called between enable() and save() it would be built again without seeing that the config entity is now disabled, and then it would be invalidated again when we save.

It really makes no sense :)

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

@Berdir that's a good point :) thank you.

Committed 58a6dbb and pushed to 8.0.x. Thanks!

  • alexpott committed 58a6dbb on 8.0.x
    Issue #2525884 by jesperjb, joshi.rohit100, Berdir: Avoid unecessary...

Status: Fixed » Closed (fixed)

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