Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comments
Comment #1
Wim LeersComment #2
jespermb CreditAttribution: jespermb as a volunteer commentedHi, I have removed the mentioned cache check and added the if statement to aggregator.
Comment #4
BerdirThe 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.
Comment #5
jespermb CreditAttribution: jespermb as a volunteer commentedComment removed and test updated. I hope it has been correctly changed, my experience with automated tests is limited unfortunetly.
Comment #6
BerdirLooks 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.
Comment #8
joshi.rohit100re-rolled the patch as previous was unable to apply.
Comment #9
borisson_Back to RTBC as the patch now does apply and tests pass.
Comment #10
alexpottAre we sure about this?
Comment #11
BerdirYes, 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 :)
Comment #12
alexpott@Berdir that's a good point :) thank you.
Committed 58a6dbb and pushed to 8.0.x. Thanks!