Introduce a pre-delete hook like in D8

Not sure if this is what you meant, but I took a stab at it.

I'd say leave the invocation outside the try block, so that everyone knows who threw the exception.

Also, #1007830: Nested transactions throw exceptions when they got out of scope (from the comments in that function) is fixed, but another one started: #1803886: PDOException: Syntax error or access violation: 1305 SAVEPOINT savepoint_1 does not exist I'd say transactions are unneeded here, though they might be cool and all.

Just ran into this as well.

When trying to use entity metadata wrappers that reference the old entity in a hook_entity_delete(), you can get exceptions because the EMW tries to load an entity that no longer exists.

New patch attached, marking as critical because this is unexpected and potentially site breaking behavior.

Bump

Can we get this in please? It's breaking part of the Group module.

In this case, there is a parent-child relationship. When the parent is deleted, the children are retrieved from the database by a conditional entity load (by 'parent' property). When Group tries to decide whether the children need to be deleted along with the parent, it tries to access the parent through an EMW.

Result: WSOD because the parent has already been deleted and the EMW can no longer access it.

That's just one use case, I can see plenty situations where this behavior causes WSODs or otherwise unexpected results.

Patch in #3 still applies. Let's see what the testbot says.

Testbot is happy green.

What else needs to happen here for a fix?

A commit.

Bump... will this be committed?

If I remember it correctly, this wasn't unified in core and entity.module does it the same way with d8 now which has another pre-delete hook. Moreover, I do not see any reasony why we should do an API break like this now?

Thus marking as won't fix. If you want, you can re-open it to introduce the pre-delete hook in d7 also?

Yes please! This has been causing WSODs in the Group module for quite a long time now.

I didn't put anything in entity.api.php because the other hook_ENTITY_TYPE_action hooks aren't in there either and the hook_entity_ACTION hooks are generally documented in system.api.php.

If this gets in, would you mind releasing a new version soon? Then I can state that version as the minimum dependency for my module.

thanks for the patch. I am trying to get this to work with commerce_order. Apparently the commerce orders get deleted like so:

entity_get_controller('commerce_order')->delete($order_ids);

Wouldnt the predelete hooks get invoked using the supplied patch from above?

It would get invoked, yes. However, you'd need to move your code from:
* hook_entity_delete() to hook_entity_predelete() or
* hook_ENTITY_TYPE_delete() to hook_ENTITY_TYPE_predelete()

So I am trying to understand what's the difference between 'delete' and 'predelete'. cannot we not do the same operation in hook_delete?

No, the difference is huge.

Predelete is ran when the entities still exist, much like 'delete' for some core entities. However, as fago mentioned in #11: Not all core entities behave this way; some run their 'delete' after the entity has been deleted.

In Drupal 8, we have 'predelete' and 'delete' properly implemented for all entities. Therefore, the patch in #12 adds a 'predelete' hook to mimic D8 and to make sure no existing hook_entity_delete() implementations break all of the sudden.

A very important consequence of adding hook_entity_predelete is that people may now clean up using an EntityMetadataWrapper (EMW) without getting a white screen of death (WSOD). Previously, the EMW would try to load the entity again, which would fail because it had already been deleted.

@fago: If I build a branch of Group that uses this new hook and it works, would that be enough to set this issue to RTBC myself?

This has already been fixed:
public static function ConfigEntityBase::preDelete.

Facepalm!

should this be closed, as it's been already done somewhere else?

feature already done