field_attach_delete_bundle() calls hook_field_attach_delete_bundle before calling field_delete_instance(), thus before the related instances are actually deleted.

CommentFileSizeAuthor
#8 fields-period-441180-8.patch1.09 KBbjaspan
#4 field-delete-bundle-441180-4.patch2.26 KBbjaspan
#1 field-delete-bundle-441180-1.patch1.42 KBbjaspan

Comments

bjaspan’s picture

bjaspan commented
Status: Active » Needs review
StatusFileSize
new field-delete-bundle-441180-1.patch1.42 KB

Note that this is the last hook_field_attach_* hook that is not a pre-hook that is called before the end of its related function.

yched’s picture

yched commented

I think the idea was that if you delete (mark for deletion) the instances first, then the code in hook_field_attach_delete_bundle() cannot use the usual $instances = field_info_instances($bundle); to get the instances, because they won't show up.

Why is 'call hook first, then mark for delete' problematic ?

yched’s picture

yched commented

OK, got it looking at pbs.module. It does not need to actually do something about the instances in the bundle, just resynchronize according to the *new* state.

What node_type_delete() does is :
- get the type $info
- delete the type
- call the delete hook with the saved info

Maybe we should do the same here ?
- get $instances = field_info_instances($bundle);
- do the delete
- call hook_field_attach_delete_bundle with the saved $instances as a param ?

bjaspan’s picture

bjaspan commented
StatusFileSize
new field-delete-bundle-441180-4.patch2.26 KB

Yes, you got the point exactly. :-) By the way, I just noticed that the PHP docs for hook_field_attach_delete_bundle() specifies that the hook is invoked after the delete operation is performed.

Your proposal looks like it would work, and it turns out we already got $instances earlier in the function. Here's a new patch.

yched’s picture

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

Ready, then :-)

dries’s picture

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

Committed to CVS HEAD. Thanks.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Fixed » Needs work 
+ * @param $bundle
+ *   The bundle that was just deleted.

It would be nice to know what "the $bundle" actually is. A string?

+  // Let other modules act on deleting the bundle

Comments should form a proper sentence - with a trailing full-stop (period).

bjaspan’s picture

bjaspan commented
Status: Needs work » Reviewed & tested by the community
StatusFileSize
new fields-period-441180-8.patch1.09 KB

I fixed the missing period in the comment.

$bundle is a string. It is one of the basic data types of Field API, documented elsewhere. We do not declare the types of function arguments in Drupal (though we should) so doing something different for this one function would be inconsistent.

I only added a period, so RTBC.

dries’s picture

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

Committed to CVS HEAD. Thanks!

Status: Fixed » Closed (fixed)

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