BlockContent entities uses delete method instead of pre/postdelete

What 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.

Overriding 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?

If EntityStorageControllerInterface::delete already has an array of entities, it can simply call those hook methods on each entity.

So 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.

Meanwhile 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.

CustomBlock, Field and FieldInstance entities do so - i.e. we should fix them.

Looks like this got resolved Elsewhere

https://git.drupalcode.org/project/drupal/-/blob/9.5.x/core/modules/bloc...

That's new code, there's still a delete method below

So 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?

No, just to pre/post Delete. delete is not guaranteed to be called, it's a wrapper for $storage->delete($entity)

Thanks, feels like a novice task then.

Updated issue summary

This along the lines of what you were thinking?

It should probably be a a new preDelete() method as the current code runs before delete, but yeah, basically.

So this? Or do we not need that first loop?

Yes, 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]);

Not sure I follow. I see in that test a block entity does get deleted already.

Yes, 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.

So like this?

The 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.

This closer?

Not sure parent pre-delete should be executed before instance removal
Needs clarification

+++ b/core/modules/block_content/src/Entity/BlockContent.php
@@ -126,6 +126,20 @@ public function postSave(EntityStorageInterface $storage, $update = TRUE) {
+  public static function preDelete(EntityStorageInterface $storage, array $entities) {
+    parent::preDelete($storage, $entities);
...
+    /** @var \Drupal\block_content\BlockContentInterface $block */
+    foreach ($entities as $block) {
+      foreach ($block->getInstances() as $instance) {
+        $instance->delete();

@@ -162,16 +176,6 @@ public function preSaveRevision(EntityStorageInterface $storage, \stdClass $reco
-  public function delete() {
-    foreach ($this->getInstances() as $instance) {
-      $instance->delete();
-    }
-    parent::delete();

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

@andypost Was following how I saw other preDeletes written.

With 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.

+++ b/core/modules/block_content/tests/src/Kernel/BlockContentDeletionTest.php
@@ -57,6 +60,24 @@ public function testDeletingBlockContentShouldClearPluginCache() {
+    $storage = $this->container->get('entity_type.manager')->getStorage('block_content');
...
+    $this->assertNull(\Drupal::entityTypeManager()->getStorage('block')->loadUnchanged($block->id()));

I wonder why 2 different approaches used to access entity type manager, please unify within this testing method to \Drupal::etm

Made changes as per comment #42.

I don't like using $this->container but there's an existing example in the test, so fine to stay consistent.

Adding credit from #2509552: BlockContent::delete() should be postDelete()

Nice fix.

Committed and pushed 9e67c21100 to 10.1.x and 9c93ae1486 to 10.0.x and ba44f6430e to 9.5.x. Thanks!

I backported this to 9.5.x as this is kinda like moving a hook implementation and has no API implications.