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 :

<?php
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.

Comments

yched’s picture

Also, 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 :-/

msonnabaum’s picture

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.

Berdir’s picture

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?

yched’s picture

I 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 :-/.

fago’s picture

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.

fago’s picture

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.

fago’s picture

Title:entity_delete_multiple() bypasses Entity::delete()» CustomBlock, Field and FieldInstance entities do not use pre/postdelete methods

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

yched’s picture

Title:CustomBlock, Field and FieldInstance entities do not use pre/postdelete methods» CustomBlock entities do not use pre/postdelete methods
Component:entity system» block.module