Problem/Motivation

The method does a $bundle_of !== FALSE check. But it actually returns NULL, not FALSE if the bundle does not exist. That results in a $entity_manager->clearCachedFieldDefinitions() call for every node/user/comment that is saved.

Proposed resolution

Thought of different ways to fix this. Quick fix is to remove the type safe check and just do if ($bundle_of), because we're not interested that it is not FALSE that seems like an unnecessary negative. We just want to know if it has a $bundle_of.

However, I think this logic is better off in ConfigEntityBundleBase, where all other bundle_of logic lives too. Refactored the existing postSave() there to cover this part too.

I don't think this needs a regression test, for example by saving a node and checking if the caches were cleared as the problematic code path simply doesn't exist anymore.

Remaining tasks

User interface changes

API changes

CommentFileSizeAuthor
#10 entity-field-cache-clears-2359509-10-interdiff.txt1.13 KBBerdir
#10 entity-field-cache-clears-2359509-10.patch7.97 KBBerdir
#8 entity-field-cache-clears-2359509-8-interdiff.txt2.69 KBBerdir
#8 entity-field-cache-clears-2359509-8.patch8.12 KBBerdir
#6 entity-field-cache-clears-2359509-6-interdiff.txt3.99 KBBerdir
#6 entity-field-cache-clears-2359509-6.patch8.61 KBBerdir
#1 entity-field-cache-clears-2359509-1.patch4.64 KBBerdir
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Active » Needs review
FileSize
entity-field-cache-clears-2359509-1.patch4.64 KB

Here is a patch.

Status: Needs review » Needs work

The last submitted patch, 1: entity-field-cache-clears-2359509-1.patch, failed testing.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented

If it's technically impossible to create a content entity type that is the bundle entity type for another entity type, then this looks fine.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Issue tags: +D8 cacheability
Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

It is not technically impossible, but you would have a lot of weird problems (like your fields would depend on the bundle, which would be content, not config). Which we can not safely recover from, which is one of the requirements from the whole config depending on content topic.

In case you do not have a config entity/do not depend on ConfigEntityBundleBase, (it could theoretically be a simple config), then it is the same as for all the other logic that is already in ConfigEntityBundleBase like calling $entity_manager->onBundleDelete() and the other methods: You are responsible to do it manually.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs work » Needs review
FileSize
entity-field-cache-clears-2359509-6.patch8.61 KB
entity-field-cache-clears-2359509-6-interdiff.txt3.99 KB

Oh, fun with the cached language list again. Related to #2303877: Remove cached languages from ContentEntityBase.

Implemented it so we can unset on __sleep() and lazy-load.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented 
  1. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBundleBase.php
@@ -67,12 +67,25 @@ protected function deleteDisplays() {
+      $entity_manager->onBundleCreate($this->id(), $this->getEntityType()->getBundleOf());
...
+      $bundle_of = $this->getEntityType()->getBundleOf();

    Given that we need it both times we could just store it as well ... 

  2. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBundleBase.php
@@ -67,12 +67,25 @@ protected function deleteDisplays() {
+      // Invalidate the render cache of entities for which this entity
+      // is a bundle.
+      if ($entity_manager->hasHandler($bundle_of, 'view_builder')) {
+        $entity_manager->getViewBuilder($bundle_of)->resetCache();
+      }
+      // Entity bundle field definitions may depend on bundle settings.
+      $entity_manager->clearCachedFieldDefinitions();
+
+      if ($this->getOriginalId() != $this->id()) {
+        // If the entity was renamed, update the displays.
+        $this->renameDisplays();
+        $entity_manager->onBundleRename($this->getOriginalId(), $this->id(), $bundle_of);
+      }

    It feels wrong to hardcode all that special bits inside the base entity class ... but rather we should have a simple event which different code cares about. 

  3. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -135,7 +135,6 @@
-    $this->languages = $this->languageManager()->getLanguages(LanguageInterface::STATE_ALL);

    +1 do less in the constructor 

  4. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -454,7 +464,7 @@ public function language() {
       if (!isset($this->languages[$this->activeLangcode])) {
-        $this->languages += $this->languageManager()->getLanguages(LanguageInterface::STATE_ALL);
+        $this->getLanguages();
       }
       $language = $this->languages[$this->activeLangcode];

@@ -462,7 +472,7 @@ public function language() {
       if (!isset($this->languages[$this->defaultLangcode])) {
-        $this->languages += $this->languageManager()->getLanguages(LanguageInterface::STATE_ALL);
+        $this->getLanguages();
       }
       $language = $this->languages[$this->defaultLangcode];

@@ -558,6 +568,7 @@ public function getTranslation($langcode) {
+        $this->getLanguages();
         if (isset($this->languages[$langcode])) {

    I don't get why we not just do 

    
$language = $this->getLanguages()[$this->activeLangcode] instead.

    now that you have introduce it 

  5. +++ b/core/modules/path/src/Tests/PathLanguageTest.php
@@ -97,7 +97,7 @@ function testAliasTranslation() {
+    //$this->rebuildContainer();

    Ups

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
FileSize
entity-field-cache-clears-2359509-8.patch8.12 KB
entity-field-cache-clears-2359509-8-interdiff.txt2.69 KB

1. Changed.
2. Not sure I agree but I think it doesn't really matter for this issue :)
3. I'm not sure if it will work, but it is possible that the fix that I'm applying here will also fix why we had to add those checks before.

Status: Needs review » Needs work

The last submitted patch, 8: entity-field-cache-clears-2359509-8.patch, failed testing.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs work » Needs review
FileSize
entity-field-cache-clears-2359509-10.patch7.97 KB
entity-field-cache-clears-2359509-10-interdiff.txt1.13 KB

3. Yeah, no luck :)

Berdir queued 10: entity-field-cache-clears-2359509-10.patch for re-testing.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented

I didn't review the language-related changes in this patch that were imported from #2303877: Remove cached languages from ContentEntityBase, but I did review the key changes in this issue. It basically moves Entity::onUpdateBundleEntity() which is called by Entity::invalidateTagsOnSave() which is called by Entity::postSave() into ConfigEntityBundleBase::postSave().
This is okay for the reasons explained in #5.

So: if plach approves the language parts of this patch, then this is RTBC IMHO.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Status: Needs review » Reviewed & tested by the community

It is :)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

Committed 9e8a523 and pushed to 8.0.x. Thanks!

  • alexpott committed 9e8a523 on 8.0.x 
    Issue #2359509 by Berdir: Incorrect type safe check in Entity::...

Status: Fixed » Closed (fixed)

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