hook_field_delete() no longer invoked during field_purge_data()

Yes, the docs for field_info_field() state:

$field_name can only refer to a non-deleted, active field. Use field_read_fields() to retrieve information on deleted or inactive fields.

However, _field_invoke() uses it, as in the patch above. (For some reason the patch does not show the function name, but the patch applies to lines in _field_invoke()).

Should we instead use field_read_fields() as suggested by the docblock?

Edit: Writing a test for this.

I think I've also confirmed the bug. The field API unit tests include coverage for hook invocation, including a whole API for it, but use of the API is conspicuously absent from FieldBulkDeleteTestCase::testPurgeField().

Attached test adds hook test coverage to FieldBulkDeleteTestCase::testPurgeField(). @casey's patch from the original post resolves the issue, though I'm still unsure if we should try to use field_read_fields() instead.

Looking at the docs for field_info_field_by_id(), it says:

$field_id The id of the field to retrieve. $field_id can refer to a deleted field.

So, since this is explicitly documented, I think we are fine using the approach in the original post and #3. We might want to open a documentation followup referencing this information.

Edit: Opened #1372280: Document that field_info_field_by_id() may also be used for deleted fields..

Tested patch from issue description, working. Re-rolled so that it is easier to apply.

Everett Zufelt, I posted a more recent version of the patch in #3 above. (It is the same except that it adds an automated test.) So could you test that patch? Thank you.

@xjm

I might be missing something, but the patch you posted only contains tests, it doesn't incorporate the original patch.

Everett -- My apologies, there are two patches attached to my earlier comment; one with tests only, and the second with the fix included. This is the combined patch. Thanks!

Includes tests from #3

@xjm :)

I woke up early :) Tested, working, if the patch I up'd in #9 passes the bots then RTBC.

@Everett -- No problem. :) In reading again I realized it would be good to add an inline comment explaining the use of field_info_field_by_id(), so this patch adds that to #9. The interdiff shows the added comment.

looks good; only that field_test_memorize() function (not introduced here) makes me run away screaming

Oh, and it has tests, so untagging.

Gosh. We go through so many hoops with delayed bulk deletion so that we can call hook_field_delete(), and the hook is actually not executed. I guess this worked at some point (?). What a fail.

Patch is fine, but reveals another bug: hook_field_load() is not called either during purge, which issues warnings when purging a file/image field.

That's because field_attach_load() does an incorrect call to _field_invoke_multiple('load') - the $options argument (that specifies "yes, I want you to run on deleted fields too") is not in the correct position and gets discarded. Code fix is ready, I'm working on the tests. Meanwhile, this doesn't prevent the current patch from getting in, so leaving RTBC.

Attached patch
- fixes the additional bug of hook_field_load() not being invoked ether in field_bulk_delete().
- adds new tests for that bug (and cleaned / abstracted a little the code that tests hook invocations)

+ a test-only patch to prove the fix
+ an interdiff with patch #11

CNR for the new patch.

And the d7 patch.

If you attach the test-only first and the combined second then testbot doesn't change the status. ;) Edit: Note that there's a smidge of trailing whitespace in there. I haven't read through the whole thing yet.

Yup, fixed trailing spaces on one line. I recently switched to NetBeans, which is supposed to clear trailing spaces on save, but occasionally misses some on empty lines.
Also fixed a double trailing ";"

Alright, reviewed #20 again and the refactoring in the tests looks good as well. I think this is RTBC.

Looks fine here as well. Only test refactoring since it was RTBC in December, so committed/pushed to 8.x.

Assuming the patch needs minor whitespace fixes for D7 so is this the right status?

D7 patch.

Heh, I confess I'm not totally sure what all of this is doing, but I didn't see anything that stuck out to me as terribly awful (other than it's weird to only check load/delete and not others, but that's not a critical bug).

Committed and pushed to 7.x. Thanks for all the work on this!

@webchick : load and delete are the only ops involved during purge - load existing values, purge them.
Thks for the commit :-).