Problem/Motivation
BlockContent::delete is not guaranteed to be called as its just a wrapper to Storage::delete
Steps to reproduce
Call \Drupal::entityTypeManager()->getStorage('block_content')->delete($some_entities)
and notice that BlockContent::delete is not called.
Proposed resolution
Move the logic from BlockContent::delete to BlockContent::preDelete
Remaining tasks
Move the logic
User interface changes
API changes
Data model changes
Release notes snippet
entity_delete_multiple() calls $storage_controller->delete($entities) directly.
So any code present in the EntityClass::delete() method is never executed. Thus those two lines are not equivalent :
entity_delete_multiple('my_entity_type', array(1));
entity_load('my_entity_type', 1)->delete();
Problem : entity_delete_multiple() is marked deprecated, with the official recommendation being "use $storage_controller->delete($entities)", i.e we promote bypassing the $entity->delete() method, and deleting entities through the controller directly.
Right now, this means that entity-type-specific code for delete() should not be added in the entity type class, but in the storage controller.
However:
- nothing documents that, if that's how we want things to be, Entity::delete() should be final.
- there are definitely cases where the code would be storage-controller agnostic.
Note : The problem doesn't seem to arise for save, since we never added a generic entity_save() function.
Comment | File | Size | Author |
---|---|---|---|
#43 | 1937266-43.patch | 2.84 KB | ravi.shankar |
#43 | interdiff_40-43.txt | 787 bytes | ravi.shankar |
#43 | 1937266-43-test-only.patch | 1.72 KB | ravi.shankar |
#40 | 1937266-40.patch | 2.82 KB | smustgrave |
#40 | 1937266-40-tests-only.patch | 1.71 KB | smustgrave |
Issue fork drupal-1937266
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #1
yched CreditAttribution: yched commentedAlso, note that while no entity class in core currently overrides Entity::delete() (although we have a case in the "Field API / CMI" in-progress patch), there are a couple already that do override Entity::save().
So "overriding Entity::save() is fine but overriding Entity::delete() will lead to inconsistencies, put the code in StorageController::delete() instead" would not be great DX-wise :-/
Comment #2
msonnabaum CreditAttribution: msonnabaum commentedWhat if we moved preDelete and postDelete to EntityInterface as empty hook methods, so that any custom business logic can go there? Then there would never be a usecase to override EntityInterface::delete, it would simply delegate to the storage controller's delete method.
If EntityStorageControllerInterface::delete already has an array of entities, it can simply call those hook methods on each entity.
Comment #3
BerdirOverriding Entity::save() is not possible either. $storage_controller->save($entity) will not call Entity:save(). That method shouldn't be overridden. It's just a shortcut for the latter.
Whether that is good or bad design is another question, but right now, doing it is wrong :) And making it final is problematic because that prevents us from mocking it.
Not sure about this being a bug. As said above, it's not an exception, it's by design right now, so changing the design would be a task?
Comment #4
yched CreditAttribution: yched commentedI think there are a couple config entities that override save right now. At least Field and FieldInstance do.
And yeah, putting every logic in the controller kind of sucks :-/.
Comment #5
fagoSo are you suggesting to go with static methods on the entity class? If we end up passing on $entity_type and $entity_info I don't think this ends up as a good DX, but if we are not and use that for entity-type specific customisations I think it should work and I agree that it improves DX.
Comment #6
fagoMeanwhile we have the easy way to delete-customizations in the entity class via the static pre/post-delete methods. Thus any entity type customizing delete() does it wrong, and needs to be fixed.
Comment #7
fagoCustomBlock, Field and FieldInstance entities do so - i.e. we should fix them.
Comment #8
yched CreditAttribution: yched commentedField & FieldInstance are being worked on in #2020895: Move save() / delete() logic in Field / FieldInstance to [pre|post]Save(), [pre|post]Delete()
Comment #11
tim.plunkettComment #21
larowlanLooks like this got resolved Elsewhere
https://git.drupalcode.org/project/drupal/-/blob/9.5.x/core/modules/bloc...
Comment #22
BerdirThat's new code, there's still a delete method below
Comment #23
larowlanSo just to confirm, we need to move that logic into a custom storage controller and out of the entity?
I don't understand why that's an issue, is it in cases where someone changes the entity-class?
Comment #24
BerdirNo, just to pre/post Delete. delete is not guaranteed to be called, it's a wrapper for $storage->delete($entity)
Comment #25
larowlanThanks, feels like a novice task then.
Updated issue summary
Comment #27
smustgrave CreditAttribution: smustgrave at Mobomo commentedThis along the lines of what you were thinking?
Comment #28
BerdirIt should probably be a a new preDelete() method as the current code runs before delete, but yeah, basically.
Comment #29
smustgrave CreditAttribution: smustgrave at Mobomo commentedSo this? Or do we not need that first loop?
Comment #30
BerdirYes, like this.
This code was added in [##1994146], the test there is functional, so we can't easily change that. We could maybe extend \Drupal\Tests\block_content\Kernel\BlockContentDeletionTest::testDeletingBlockContentShouldClearPluginCache() to also create a block config entity and verify that gets deleted also when using $storage->delete([$block_content]);
Comment #31
smustgrave CreditAttribution: smustgrave at Mobomo commentedNot sure I follow. I see in that test a block entity does get deleted already.
Comment #32
BerdirYes, but only because the code calls $block_content->delete(). The bug here is that if you call $storage->delete([$block_content]) then that does not happen which is a valid thing to do and for example happens when using the views bulk delete action or directly through the API in custom code.
So we either need a kernel test calling it like that or I suppose we could also test through the bulk delete action, but bulk actions are by default not enabled on the block content view, so we'd need to do that. That's why I'd suggest a kernel test, but the views approach could be useful for example for a manual test.
Comment #33
smustgrave CreditAttribution: smustgrave at Mobomo commentedSo like this?
Comment #34
BerdirThe existing kernel test is only testing block *plugins*, the code that we are changing is about block config entities/instances. To test it in the existing kernel test, you also need to create a block for the given plugin and then verify that it was deleted.
Comment #35
smustgrave CreditAttribution: smustgrave at Mobomo commentedThis closer?
Comment #36
smustgrave CreditAttribution: smustgrave at Mobomo commentedComment #37
andypostNot sure parent pre-delete should be executed before instance removal
Needs clarification
I wonder why parent now called before instances delete?
Maybe it does not have effect but looks weird, probably some code comment about why parent called first could help
Comment #38
smustgrave CreditAttribution: smustgrave at Mobomo commented@andypost Was following how I saw other preDeletes written.
Comment #39
BerdirWith delete, the order was relevant, because it essentially means preDelete or postDelete. with preDelete(), it does not because it's either we preDelete and it's IMHO perfectly fine to call the parent first and doesn't need an explanation.
Test looks fine to me, a failing test (a patch with just the test) would be helpful to prove that this indeed covers this bug.
Comment #40
smustgrave CreditAttribution: smustgrave at Mobomo commentedComment #42
andypostI wonder why 2 different approaches used to access entity type manager, please unify within this testing method to \Drupal::etm
Comment #43
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedMade changes as per comment #42.
Comment #45
BerdirI don't like using $this->container but there's an existing example in the test, so fine to stay consistent.
Comment #53
larowlanAdding credit from #2509552: BlockContent::delete() should be postDelete()
Comment #64
alexpottNice fix.
Committed and pushed 9e67c21100 to 10.1.x and 9c93ae1486 to 10.0.x and ba44f6430e to 9.5.x. Thanks!
Comment #68
alexpottI backported this to 9.5.x as this is kinda like moving a hook implementation and has no API implications.