Problem/Motivation

In #2949964: Add an EntityOwnerTrait to standardize the base field needed by EntityOwnerInterface @bedir discovered an inconsistency with how the entity keys cache on ContentEntityBase is purged. This revealed itself through a newly introduced entity key for "owner" however the same circumstances would be exploitable for the existing "status"/"published" keys on Node.

The problem is lies in \Drupal\Core\Entity\ContentEntityBase::onChange which only searches for one entity key matching a field name when attempting to purge the cache.

Proposed resolution

Fix this to search for multiple keys.

Remaining tasks

User interface changes

API changes

Data model changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Sam152 created an issue. See original summary.

Sam152’s picture

Status: Active » Needs review
FileSize
5.58 KB

Failing test case for this. The usage of getEntityKey seems a bit sparse. I wonder why for example EntityPublishedTrait doesn't use it, the following would fail if it did:

    $node->set('status', TRUE);
    $this->assertEquals(TRUE, $node->isPublished());

    $node->set('status', FALSE);
    $this->assertEquals(FALSE, $node->isPublished());

Given ContentEntityBase is a base class, I do think getEntityKey forms part of the public API despite it being protected. This also exposes that the order of keys is important for the behavior of getEntityKey. If we added an additional key and deprecated an old one, unless it appeared first in the list of entity keys, calls to getEntityKey('new_key') would fail.

Sam152’s picture

Title: Entity keys are cached incorrectly when two keys exists for one field. » The ContentEntityBase entity key cache is purged incorrectly when two keys exists for one field.
Sam152’s picture

Title: The ContentEntityBase entity key cache is purged incorrectly when two keys exists for one field. » The ContentEntityBase entity key cache is purged incorrectly when two keys exist for one field.

Status: Needs review » Needs work

The last submitted patch, 2: 2961986-2.patch, failed testing. View results

Sam152’s picture

Status: Needs work » Needs review
FileSize
2.37 KB
7.96 KB

And the fix. I was wondering why we do this caching in the first place and it's origins are in #2498919: Node::isPublished() and Node::getOwnerId() are expensive.

So the reason is, $entity->field->property is expensive and we use the owner and publish status a lot during access control. So with that in mind #2962304: EntityPublishedTrait should use getEntityKey for better performance.

tstoeckler’s picture

Wow, fix is very nice. Never new array_keys() had a second argument. PHP is just... ...so much fun! ;-)

Test looks great as well and fails as expected. I've seen people before complain about adding new test entity types, because that means a performance hit for any test enabling the entity_test module. I'm not sure whether think it would be possible to re-use an existing entity type while still retaining the same amount of coverage? Otherwise this is RTBC from my point of view but leaving at Needs review for that point.

tstoeckler’s picture

Looking at http://php.net/array_keys again, it would probably to make sense to pass $strict = TRUE as the third parameter as well. I don't think anyone is going to name a field '0' (which as far as I can tell would be the only case where it actually makes a difference) but still it's best practice to always use strict comparisons wherever possible.

Sam152’s picture

Agree re: strictness, adding that.

With the test entity type, I've extracted all the entity key configuration out into the test case, so EntityTestKeys could be used as a "programmable" entity type with regards to keys. Hopefully that saves us some additional entity types in the future. We could however add this functionality straight into EntityTest itself, with the only caveat that it increases the responsibilities and therefore complexity of EntityTest, EntityTestKeys clearly has only one purpose.

Happy to roll a patch for that second approach and see what folks think.

Sam152’s picture

Adding the ability to set entity keys to EntityTest. I personally prefer #9, but I'm not sure how significant the speed impact of adding another entity type is.

Berdir’s picture

  1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
    @@ -772,23 +772,25 @@ protected function updateFieldLangcodes($langcode) {
    +      foreach ($keys as $key) {
    

    array_keys() always returns an array, also when empty. is this really easier to read than just foreach (array_keys($this->getEntityType()->getKeys(), $name, TRUE) as $key) ?

    saves us one level of indendation and it's like two characters longer than the current if.

  2. +++ b/core/modules/system/tests/modules/entity_test/entity_test.module
    @@ -101,6 +102,9 @@ function entity_test_entity_type_alter(array &$entity_types) {
    +  $entity_test_definition->set('entity_keys', $state->get('entity_test.entity_keys', EntityTest::defaultEntityKeys()));
    
    +++ b/core/modules/system/tests/modules/entity_test/src/Entity/EntityTest.php
    @@ -33,13 +33,7 @@
      *   list_cache_contexts = { "entity_test_view_grants" },
    - *   entity_keys = {
    - *     "id" = "id",
    - *     "uuid" = "uuid",
    - *     "bundle" = "type",
    - *     "label" = "name",
    - *     "langcode" = "langcode",
    - *   },
    + *   entity_keys = {},
    

    I don't quite understand why we need this. it's not like we merge in the defaults, if anything is set in state we replace. Can't we just make the set() here conditional on actually having any keys to override (having no keys is not valid anyway), then we can skip all the other changes

  3. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityKeysTest.php
    @@ -0,0 +1,66 @@
    +  protected function setupTestEntityKeys($keys) {
    +    $this->state->set('entity_test.entity_keys', EntityTest::defaultEntityKeys() + $keys);
    +  }
    

    for this we could either duplicate them or we could read the current keys (which shouldn't be a problem as long as we don't all it multiple times in a single test)

Sam152’s picture

Status: Needs review » Needs work

I'll have to look into point 2 and 3, but point 1 is enough for NW. Good catch!

Sam152’s picture

Status: Needs work » Needs review
FileSize
5.51 KB
4.65 KB

1. This looks much better. Diff is a lot less disruptive.
2 + 3. I think I was trying to avoid duplicating the keys here. I think we can get away with not duplicating the keys, we simply lose the ability to remove entity keys. Since we don't need that for this test case, we can do that at some point in the future if we need it.

Berdir’s picture

Status: Needs review » Needs work

Yes, that looks a lot easier to read in the diff :)

+++ b/core/modules/system/tests/modules/entity_test/src/Entity/EntityTest.php
@@ -164,4 +164,11 @@ public function getName() {
+  /**
+   * {@inheritdoc}
+   */
+  public function getEntityKey($key) {
+    return parent::getEntityKey($key);

There have been some discussions in the past on whether this should be done or if we should use introspection.

I do prefer this, but lets at least add a comment to explain why this is done. See #2920168: Document why SqlBaseTest::getHighWaterStorage() is needed

Sam152’s picture

Good idea, it does look a bit confusing at a first glance. How about the following?

Berdir’s picture

Status: Needs work » Reviewed & tested by the community

Works for me.

alexpott’s picture

Giving @Berdir and @tstoeckler review credit.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed 62fb120619 to 8.6.x and cf907fb3ae to 8.5.x. Thanks!

diff --git a/core/modules/system/tests/modules/entity_test/entity_test.module b/core/modules/system/tests/modules/entity_test/entity_test.module
index b36510cfac..d7dc635146 100644
--- a/core/modules/system/tests/modules/entity_test/entity_test.module
+++ b/core/modules/system/tests/modules/entity_test/entity_test.module
@@ -19,7 +19,6 @@
 use Drupal\Core\Session\AccountInterface;
 use Drupal\Core\Entity\Entity\EntityFormDisplay;
 use Drupal\Core\Url;
-use Drupal\entity_test\Entity\EntityTest;
 
 /**
  * Filter that limits test entity list to revisable ones.

Removed unused use on commit.

  • alexpott committed 62fb120 on 8.6.x
    Issue #2961986 by Sam152, Berdir, tstoeckler: The ContentEntityBase...

  • alexpott committed cf907fb on 8.5.x
    Issue #2961986 by Sam152, Berdir, tstoeckler: The ContentEntityBase...

Status: Fixed » Closed (fixed)

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