Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
text, list, you name it: they do not update deleted fields. Hilarity ensues when _field_info_collate_fields fires the next page load and tries to get deleted fields too -- list_schema won't have anything to say about the left behind fields.
Comment | File | Size | Author |
---|---|---|---|
#22 | list_test.patch | 195.6 KB | chx |
#19 | field_update_deleted-1022924-19.patch | 4.26 KB | yched |
#16 | field_update_deleted-1022924-16.patch | 4.62 KB | yched |
#16 | interdiff.txt | 3.63 KB | yched |
delete_update.patch | 2.59 KB | chx | |
Comments
Comment #1
catchSubscribing.
Comment #2
chx CreditAttribution: chx commentedNote how text tries to update deleted fields -- it has no chance in 7.0 because _update_7000_field_read_fields hardwired deleted 0.
Comment #3
chx CreditAttribution: chx commentedMore clarification: this is an update not an upgrade issue. It only affects those who had deleted list (or text but that's a minor affair) fields prior to list_update_7001.
Comment #4
chx CreditAttribution: chx commentedAnother note, a list_update_7002 is certainly in order which does nothing but calls list_update_7001.
Comment #5
pwolanin CreditAttribution: pwolanin commentedshould call 7001 or 7000? I'm not sure what the change to 7001 is doing - it just seems to eliminate the key in the foreach setup.
Comment #6
chx CreditAttribution: chx commentedThere is no list_update_7000 in list.install
Comment #7
pwolanin CreditAttribution: pwolanin commentedAh, I see now - it's _update_7000_field_read_fields() which is called by list_update_7001()
Comment #8
carlos8f CreditAttribution: carlos8f commentedsubscribing
Comment #9
effulgentsia CreditAttribution: effulgentsia commentedWhat are the steps to reproduce, and what is the symptom of said hilarity? I tried installing a D7RC3 site, created a list field on the Page type, deleted it, updated to 7.0, and am not noticing any problems, at least no errors reported in the UI. When would a problem due to non-updated deleted fields surface?
Comment #10
chx CreditAttribution: chx commentedHrm, i we were seeing some level of PHP errors immediately. Are you sure the list change went in after RC3? I am pretty sure the issue is valid :)
Edit: Please check you indeed have the deleted field in the field_config table. Maybe if you immediately delete it gets rid of it? There might be a need to create a record with said field.
Comment #11
yched CreditAttribution: yched commentedI'm not sure either why the specific 'list update' would be problematic. If the field is deleted, the only thing that will happen to it is values deletion on cron, and unless I'm mistaken that shouldn't involve dealing with its 'allowed values'.
But +1 on principle about letting _update_7000_field_read_fields() retrieve deleted fields as well.
I'd only suggest we use the same 'include deleted' param pattern as field_read_fields(). Dealing with conditions only lets you say 'give me all deleted fields matching X' or 'all non-deleted fields matching X', not 'all fields matching X, deleted or not', which is what we want here. Also consistency++.
Comment #12
chx CreditAttribution: chx commentedNo, the problem is the page after the deletion. field info collate tries to gather column for everything inc deleted and does not get an array from list_field_schema and gets very unhappy over that fact.
Comment #13
yched CreditAttribution: yched commentedAh right. list_update_7001() renamed field types as well. Deleted fields remain with the old, now unknown field type names.
Comment #14
Taxoman CreditAttribution: Taxoman commentedLess scary title...
Comment #15
KarenS CreditAttribution: KarenS commentedMaybe a better title. This is just about deleted fields.
Comment #16
yched CreditAttribution: yched commentedLooking back at this, I take back what I wrote in #11 about reusing field_read_field()'s 'include deleted' pattern. Having _update_7000_field_read_fields() return all fields by default makes more sense for the function use cases.
Updated patch :
- removed _update_7000_field_read_fields() $key param, added in the original patch. Overkill IMO, plus we'd need to explain than keying by anything else than 'field id' is not safe unless you explicitly filter out deleted fields. Patch changes taxonomy_update_7005() accordingly.
- while I'm there, removes a useless !empty($conditions) check. We can foreach empty arrays.
- adds list_update_7002(), which simply replays list_update_7001(), to fix the original bug (list_update_7001() overlooked deleted-but-not-purge-yet list fields). 7001 is reentrant, so it's harmless to replay it on a clean db.
(interdiff with patch from the OP attached as well)
Comment #18
bfroehle CreditAttribution: bfroehle commentedFYI: The latest patch in #1003308: Forum module cannot decide if it wants 'taxonomy_forums' hardcoded in it leading to faulty functioning is also using _update_7000_field_read_fields().
Comment #19
yched CreditAttribution: yched commentedOk, chx was right, the taxonomy D6 upgrade function really benefits from the option to key by field name rather than field id.
Updated patch. Perfect circle back to chx's original patch, with a couple additional comments and an update for deleted list fields messed up by list_update_7001().
Comment #20
yched CreditAttribution: yched commentedNote : since we're about to re-run list_update_7001(), we might want to get #1059184: warnings in list_update_7001 (edge cases) in first (minor).
Comment #21
chx CreditAttribution: chx commentedWorking on a test.
Comment #22
chx CreditAttribution: chx commentedSomeone wants to lend a hand with this testing thing? Doesnt log in.
Comment #23
webchickWe discussed this, and decided that writing tests isn't really worth it on this patch.
First, because it was a weird quirk, and is unlikely to ever surface again.
Second, because writing such a test would be tremendously expensive and bloat the core download by about 1MB unless we made special arrangements like excluding upgrade tests from the tarball (which we could still do, but hopefully not holding up a critical bug fix).
Committed to 7.x! Thanks!