doh ! Looking at #457080: Add node_delete_multiple() made me realize node module never calls field_attach_delete() nor field_attach_delete_revision().
user module never calls field_attach_delete() either - although I think we have an excuse (?) for that one: user deletion didn't exist when we started on this...
I'm not where I can easily roll a patch for the next 2-3 days. Easy job for a beginner Field API contributor :-)
If/when the patch above and #362024: Field Attach: Make hook_field_load() be 'multiple' like field_attach_load() both land, we'll have an opportunity to make field_attach_delete() multiple as well...
| Comment | File | Size | Author |
|---|---|---|---|
| #2 | node_field_delete.patch | 1.66 KB | quicksketch |
Comments
Comment #1
quicksketchUser module is missing these too. This problem makes cleaning up files in a File field impossible and will become a blocker for #391330: File Field for Core.
Comment #2
quicksketchSome tests would obviously help prevent this problem in the future. I'm including tests for this in File module since I need to make sure files are properly deleted on update/delete/delete revision, but probably some dedicated tests would be a good idea too. This patch solves the immediate need at least.
Comment #3
yched commentedYes, they miss in user.module too because there was no user delete when the initial Field API landed - not sure what's our excuse for nodes, though ;-)
The patch here fixes nodes and is RTBC.
About tests: yes, Field API features are tested in an entity-agnostic way (using a dummy 'test' entity type and a a 'test' field type). The 'being fieldable' behavior of specific core entities (node, users, terms..) are not currently tested. We didn't really find a good approach for this so far.
Comment #4
quicksketchNote that the "delete revision" hook has a second bug that prevents it from working even after this patch: #520620: Delete revision and prepare translation field hooks are called incorrectly (or not at all).
Comment #5
quicksketchOops, but I'd be happy to take this RTBC. :-)
Comment #6
webchickI really dislike committing such bug fixes without tests, but quicksketch has informed me that once committed, the FileField tests will cover this, since that's what uncovered the bug in the first place. :) Woohoo!
Committed to HEAD!