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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch’s picture

Subscribing.

chx’s picture

Note how text tries to update deleted fields -- it has no chance in 7.0 because _update_7000_field_read_fields hardwired deleted 0.

chx’s picture

More 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.

chx’s picture

Another note, a list_update_7002 is certainly in order which does nothing but calls list_update_7001.

pwolanin’s picture

should 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.

chx’s picture

There is no list_update_7000 in list.install

pwolanin’s picture

Ah, I see now - it's _update_7000_field_read_fields() which is called by list_update_7001()

carlos8f’s picture

subscribing

effulgentsia’s picture

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

What 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?

chx’s picture

Hrm, 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.

yched’s picture

I'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++.

chx’s picture

No, 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.

yched’s picture

Ah right. list_update_7001() renamed field types as well. Deleted fields remain with the old, now unknown field type names.

Taxoman’s picture

Title: All field updates are broken » All field updates are broken (on update, not upgrade)

Less scary title...

KarenS’s picture

Title: All field updates are broken (on update, not upgrade) » Updates are broken for deleted fields

Maybe a better title. This is just about deleted fields.

yched’s picture

Looking 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)

Status: Needs review » Needs work

The last submitted patch, field_update_deleted-1022924-16.patch, failed testing.

bfroehle’s picture

yched’s picture

Status: Needs work » Needs review
FileSize
4.26 KB

Ok, 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().

yched’s picture

Note : 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).

chx’s picture

Assigned: Unassigned » chx

Working on a test.

chx’s picture

FileSize
195.6 KB

Someone wants to lend a hand with this testing thing? Doesnt log in.

webchick’s picture

Status: Needs review » Fixed

We 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!

Status: Fixed » Closed (fixed)

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