field_delete_{field,instance}() does not invoke hook_field_delete_{field,instance}().

Also, field_delete_field() marks all instances as deleted without even calling hook_field_storage_delete_instance() for them. ISTM that field_delete_field() should call field_delete_instance() for all instances before deleting the field.

These fixes are required for per-bundle storage and other non-default storage engines to work.

CommentFileSizeAuthor
#5 field-delete-hooks-571934-5.patch3.28 KBbjaspan
#1 field-delete-hooks-571934-1.patch2.76 KBbjaspan

Comments

bjaspan’s picture

bjaspan commented
Status: Active » Needs review
StatusFileSize
new field-delete-hooks-571934-1.patch2.76 KB

Patch attached.

yched’s picture

yched commented
Status: Needs review » Reviewed & tested by the community

Makes sense, code is fine. As the TODO: not implemented notes in field.api.php show, we always had the intention to have those hooks...

Kind of beats me why we chose field_delete_field($field_name) and field_delete_instance($field_name, $bundle_name) instead of $field and $instance param arrays like for the create and update functions, but that's probably not something to be changed in this thread, and the patch stays consistent with the current API.

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

bjaspan’s picture

bjaspan commented

Actually, I'm not sure this patch is right.

Should hook_field_delete_{field,instance} be called before or after the Field API cache is updated to reflect the deletion? If we invoke it before, then calls to field_info_* functions will still return the "deleted" field/instance, which is a problem if (like pbs.module) you want to act on all existing fields/instances. If we invoke it after, then the field/instance is already deleted, and the hooks have no way to access the deleted instance at all (though it can access the deleted field by id).

bjaspan’s picture

bjaspan commented
Status: Needs work » Needs review
StatusFileSize
new field-delete-hooks-571934-5.patch3.28 KB

yched reminded me of the general strategy: Call deletion hooks after the object in question is deleted and the cached cleared, and pass the deleted object (which may no longer be available from the Field Info API) as an argument to the hook.

New patch.

yched’s picture

yched commented
Status: Needs review » Reviewed & tested by the community

Green tests, RTBC.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Looks good to me. Committed to CVS HEAD. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.