Saving 'Display Fields' wipes widget settings [#279190]

Has no one else noticed this? I define a Filefield in a custom content type. When I change something in Display Fields, like hiding the label, and then go back to Mange Fields, the following data are gone:
- Permitted upload file extensions
- File path
- File size restrictions (HEAD, but I tried this with beta2 first)
- File widgets

The Global settings (File display etc.) are not deleted. The same happens with Filefield Image widget, but it looks like this is a Filefield issue.
I also suspect this is the same issue as #276652: Minor problem during update of module but I'm not bold enough to move and change a whole issue like that.

Comment	File	Size	Author
#6	cck_field_update_reentrant-279190-6.patch	895 bytes	yched

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

jpetso CreditAttribution: jpetso commented 7 July 2008 at 09:14

Status:

Active

» Postponed (maintainer needs more info)

Er, FileField doesn't mess at all with the display settings page. We just don't control that code or hook into it in any way. So if changing the formatter settings affects the field configuration in any way, I would suspect this to be a CCK bug. Could you check if this kind of stuff happens with any other CCK field's settings as well?

Comment #2

ar-jan CreditAttribution: ar-jan commented 7 July 2008 at 11:16

Strange. It only happens with Filefield and Filefield_Image settings, i.e. those specific settings I mentioned. Changing a display setting for a CCK Link field or a CCK Date field does not clear any settings for those fields.

Comment #3

jpetso CreditAttribution: jpetso commented 7 July 2008 at 23:03

Priority:	Normal	» Critical
Status:	Postponed (maintainer needs more info)	» Active

Thanks for the info. I could reproduce it here, and this is indeed happening when hitting "Save" on the "Display fields" form. Might also be triggered by other circumstances, as indicated by #276652: Minor problem during update of module, which by now I'm pretty sure is the same issue. This is a grave bug, no matter if it's caused by filefield or CCK. Release blocker, should really be fixed before we're going beta3.

Comment #4

ar-jan CreditAttribution: ar-jan commented 7 July 2008 at 23:55

Looks like it's CCK. I was playing around with Embedded Media Field (which is still in -dev fro 6.x), and noticed the same thing happening:
emfield > Embedded Image Field has some settings like:
- Providers Supported
- Full size display settings
- Image Preview Settings

These are all reset when updating the display of the field.
Like with Filefield, the Global Settings and help text are not reset.

I guess we should move the issue to CCK, but I don't know which component...

Comment #5

yched CreditAttribution: yched commented 8 July 2008 at 01:22

Title:	Saving 'Display Fields' clears 'Manage Fields' settings	» Saving 'Display Fields' wipes widget settings
Project:	FileField	» Content Construction Kit (CCK)
Version:	6.x-1.0-beta2	» 6.x-2.x-dev
Component:	Code	» General

This is a CCK bug.
Something's wrong in the save sequence. Investigating.

Comment #6

yched CreditAttribution: yched commented 8 July 2008 at 02:50

Status:

Active

» Needs review

File	Size
cck_field_update_reentrant-279190-6.patch	895 bytes

Can be reproduced by simply reading a field and saving it as is :

module_load_include('inc', 'content', 'includes/content.crud');
$fields = content_field_instance_read(array('field_name' => 'field_foo', 'type_name' => 'story'));
$field = array_pop($fields);
// or $field = content_fields('field_foo', 'story'), for that matter
content_field_instance_update($field);

wipes the widgets settings for 'field_foo'.

From what I can tell, once we get in _content_field_instance_write(), there's a loop that tries to assemble $field['widget_settings'], except the correct values are already there and the code wipes them out instead.

Attached patch fixes that (and doesn't break fields settings submission). For now I have no certainty that this is the correct place to fix, though.
Side note : there's a similar bunch of code in _content_field_write(), that doesn't seem to cause any problem.