Not even sure if it ever did, but hook_field_delete() is no longer being invoked when deleting a field.
_field_invoke() uses field_info_field() for info, among which $field['module'] to construct the hook function name.
When a field is deleted, field_info_field() is no longer useable:
field_info_field() {
$info = _field_info_collate_fields();
if (isset($info['field_ids'][$field_name])) { // <-- this is no longer available
| Comment | File | Size | Author |
|---|---|---|---|
| #23 | field_bulk_delete_hooks-d7-1371938-23.patch | 12.26 KB | yched |
| #20 | field_bulk_delete_hooks-1371938-20.patch | 12.34 KB | yched |
| #17 | field_bulk_delete_hooks-1371938-17.d7.patch | 12.26 KB | yched |
| #15 | field_bulk_delete_hooks-1371938-15.patch | 12.34 KB | yched |
| #15 | field_bulk_delete_hooks-1371938-15-test-only.patch | 11 KB | yched |
Comments
Comment #1
xjmYes, the docs for field_info_field() state:
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.
Comment #2
xjmI 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().Comment #3
xjmAttached 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 usefield_read_fields()instead.Comment #4
xjmLooking at the docs for field_info_field_by_id(), it says:
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..
Comment #5
Everett Zufelt commentedTested patch from issue description, working. Re-rolled so that it is easier to apply.
Comment #6
xjmEverett 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.
Comment #7
Everett Zufelt commented@xjm
I might be missing something, but the patch you posted only contains tests, it doesn't incorporate the original patch.
Comment #8
xjmEverett -- 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!
Comment #9
Everett Zufelt commentedIncludes tests from #3
Comment #10
Everett Zufelt commented@xjm :)
I woke up early :) Tested, working, if the patch I up'd in #9 passes the bots then RTBC.
Comment #11
xjm@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.Comment #12
sunlooks good; only that field_test_memorize() function (not introduced here) makes me run away screaming
Comment #13
xjmOh, and it has tests, so untagging.
Comment #14
yched commentedGosh. 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.
Comment #15
yched commentedAttached 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
Comment #16
yched commentedCNR for the new patch.
Comment #17
yched commentedAnd the d7 patch.
Comment #19
xjmIf 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.
Comment #20
yched commentedYup, 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 ";"
Comment #21
xjmAlright, reviewed #20 again and the refactoring in the tests looks good as well. I think this is RTBC.
Comment #22
catchLooks 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?
Comment #23
yched commentedD7 patch.
Comment #24
webchickHeh, 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!
Comment #25
yched commented@webchick : load and delete are the only ops involved during purge - load existing values, purge them.
Thks for the commit :-).