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
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
Comment | File | Size | Author |
---|---|---|---|
#15 | 2961986-15.patch | 4.92 KB | Sam152 |
#15 | interdiff.txt | 793 bytes | Sam152 |
#13 | 2961986-13.patch | 4.65 KB | Sam152 |
#13 | interdiff.txt | 5.51 KB | Sam152 |
#9 | interdiff.txt | 773 bytes | Sam152 |
Comments
Comment #2
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedFailing test case for this. The usage of
getEntityKey
seems a bit sparse. I wonder why for exampleEntityPublishedTrait
doesn't use it, the following would fail if it did:Given
ContentEntityBase
is a base class, I do thinkgetEntityKey
forms part of the public API despite it being protected. This also exposes that the order of keys is important for the behavior ofgetEntityKey
. If we added an additional key and deprecated an old one, unless it appeared first in the list of entity keys, calls togetEntityKey('new_key')
would fail.Comment #3
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedComment #4
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedComment #6
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedAnd 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.
Comment #7
tstoecklerWow, 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.Comment #8
tstoecklerLooking 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.Comment #9
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedAgree 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 ofEntityTest
,EntityTestKeys
clearly has only one purpose.Happy to roll a patch for that second approach and see what folks think.
Comment #10
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedAdding 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.Comment #11
Berdirarray_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.
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
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)
Comment #12
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedI'll have to look into point 2 and 3, but point 1 is enough for NW. Good catch!
Comment #13
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commented1. 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.
Comment #14
BerdirYes, that looks a lot easier to read in the diff :)
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
Comment #15
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedGood idea, it does look a bit confusing at a first glance. How about the following?
Comment #16
BerdirWorks for me.
Comment #17
alexpottGiving @Berdir and @tstoeckler review credit.
Comment #18
alexpottCommitted and pushed 62fb120619 to 8.6.x and cf907fb3ae to 8.5.x. Thanks!
Removed unused use on commit.