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.I just ran across this while writing a patch for D8 profile2 module. The problem was that the helper function I used to load the profile objects for a user returned them keyed by their profile type. During bulk deletion in hook_user_delete() I just passed that on to entity_get_controller()->delete() which failed because the wrong entity id's where used. This happened because the storage controllers currently assume that the array keys are always the entity ids. While this is probably expected in most cases it is a somewhat fragile behavior - And it's not even documented by the interface:
/**
* Deletes permanently saved entities.
*
* @param array $entities
* An array of entity objects to delete.
*
* @throws Drupal\Core\Entity\EntityStorageException
* In case of failures, an exception is thrown.
*/
public function delete(array $entities);
It's very easy (and most likely doesn't have any performance impact) to just use array_map() to retrieve the id's from the entity objects.
It's not _really_ a bug considering that it works everywhere in Core currently but considering that it's not documented anywhere and somewhat inconsistent with the interface I am marking it as a bug report anyhow.
Fixing this is as easy as this:
// We shouldn't assume that the array keys are the entity ids.
$ids = array_map(function ($entity) {
return $entity->id();
}, $entities);
| Comment | File | Size | Author |
|---|---|---|---|
| #37 | entity_delete_1854912-37.patch | 3.94 KB | bojanz |
| #33 | entity-delete-1854912-33-TEST-ONLY.patch | 1.92 KB | jhedstrom |
| #30 | entity_delete_1854912-30.patch | 3.95 KB | seanB |
| #14 | entity_delete_1854912-14.patch | 2.02 KB | seanB |
| #8 | entity_delete_1854912-8.patch | 1.36 KB | dermario |
Comments
Comment #1
fubhy CreditAttribution: fubhy commentedComment #2
sunYou're right that it is not enforced or validated in any way currently, but right now, we expect all callers of
delete()to pass a proper array that we expect.This was recently changed/improved in #1822150: EntityStorageControllerInterface::delete() should accept an array of entities, not IDs
Comment #3
fubhy CreditAttribution: fubhy commentedI see, so there are two things we can do then:
a) Enforce ids as keys in the ->delete() by extracting them from the entities array as shown here:
or b) Document it in the interface and leave the code as it is (always expect a properly formatted array of entities)
Comment #3.0
fubhy CreditAttribution: fubhy commentedAdding example of how to fix this.
Comment #6
BerdirAn up to date fix for this is currently part of #1885830: Enable static caching for config entities, tagging as Novice and re-roll, instead of trying to re-roll the patch here, just create a new on that contains the code from over there in EntityStorageBase::delete().
Comment #7
dermarioComment #8
dermarioI tried to fix that issues with the comment of Berdir in #8. Rerolled the patch and took over the changes in EntityStorageBase::delete(). I also replaced the $entities variable with the $keyed_entities variable - otherwise the patch would not make sense since $keyed_entities would not be used.
Comment #9
seanBI will review this!
Comment #10
dermarioComment #11
seanBFirst thing that I noticed:
One of these blank lines need to be removed.
I just tested the following:
This all seemed to work (without the patch the manual delete didn't actually delete the items. No errors though!).
I will now do the following:
Comment #12
seanBAfter checking some more, the patch actually doesn't feel like the right way to go.
The actual delete is done in the doDelete() method. In the implementation of the doDelete() method for the SqlContentEntityStorage class, we should get the IDs from the entities, not from the array keys. Basically just like the KeyValueEntityStorage class already does in it's doDelete() implementation.
Attached is a patch. Not sure if we need extra tests for this.
Comment #13
BerdirSure, we can fix it differently, but the thing is that the common expectation when getting a list of entities is that they are keyed by the entity ID. that is the case when you do an entity load.
If we do it like #8, we could simplify KeyValueEntityStorage instead of having to duplicate this logic in all doDelete().
Comment #14
seanBOk, that actually does sound easier.
Attached is an updated patch with the following changes:
- Removed extra newline from #8
- Simplified the doDelete() for the KeyValueEntityStorage
Comment #18
seanBTestbot is failing 'Too many connections'. I'll try again later.
Comment #22
dermarioUnassigning myself from that issue.
Comment #23
chx CreditAttribution: chx commentedIt's delete so a) the overhead is miniscule anyways especially on such a super slow operation as a database delete b) making sure the right things get deleted is kinda important :)
Comment #24
alexpottThis looks completely sensible - nice code hardening. It would be fantastic to have a test so we don't unexpected revert this. I guess the right place to do this will be
Drupal\system\Tests\Entity\EntityApiTestComment #25
seanBI don't have much experience writing tests, but here is a first attempt to do this. Please let me know if there is stuff I could improve for this patch.
Comment #26
seanBComment #27
seanBSorry, I changed a comment since the test is not just for nodes. Here's a new patch.
Comment #30
seanBArgh, off course I need to check that the delete works for the storage controller. The entity_delete_multiple() does a loadMultiple() that fails when we pass an array not keyed by the IDs.
New patch is attached...
Comment #31
seanBComment #32
star-szr@seanB, can you please post a test-only patch to ensure the test would catch regressions? Thanks!
Comment #33
jhedstromHere's the test only part of #30. Don't attribute credit for this please.
Comment #35
jhedstromFail was expected in #33.
Comment #37
bojanz CreditAttribution: bojanz at Centarro commentedOpened a duplicate in #2560433: Entity storage assumes entities passed for deletion are keyed by id.
Patch still applies, though with offsets. Here's a fresh one.
I also took the opportunity to swap the added 'array()' with '[]' in EntityStorageBase::delete().
RTBC if green :)
Comment #38
bojanz CreditAttribution: bojanz at Centarro commentedRetitling.
Comment #39
bojanz CreditAttribution: bojanz at Centarro commentedWe're good to go.
Comment #42
bojanz CreditAttribution: bojanz at Centarro commentedGreen again.
Comment #43
alexpottCommitted 40ad8ee and pushed to 8.0.x. Thanks!
This change makes lots of sense and does not introduce any disruption.