Multigroup fields with empty values loose their ordering when saved via node_save [#577580]

Short version:
If a field in a multigroup has empty values, the ordering is reset on node_save().
Example:

Multigroup with two fields: field_users (required), field_nodes (not required).
It initially has the following values
UID 1, NID
UID 2, NID 2

The node is edited and (UID 3, NID 3) is added via the node edit page. As expected, the result is
UID 1, NID
UID 2, NID 2
UID 3, NID 3

Using a custom module, I execute (where # is this node's nid)
$node = node_load(#);
node_save($node);

The values are now
UID 1, NID 2
UID 2, NID 3
UD 3, NID

Marking critical: As this occurs on node_save with NO changes to the node, this causes any manipulation of nodes by other modules to corrupt cck multigroup data

Comment	File	Size	Author
#6	content_multigroup_node_load-577580-6.patch	1.15 KB	markus_petrux

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

markus_petrux CreditAttribution: markus_petrux commented 15 September 2009 at 07:12

Assigned:

Unassigned

» markus_petrux

Confirmed.

Too bad because I already tried to cover node_load+node_save at some point (see comments #46 and #47 in Deleting unwanted multiple values / multiple values delta issues), but it seems that was broken with the next changes to CCK core. :(

Well, aside from fixing this issue, we need to change the behavior of multigroups to store NULL values in the database for empty items (see comment #38 in Synchronized deltas Views integration: Filter on equal deltas in multigroups), and this should make things easier here.

Both issues are related, and I think we don't have an issue opened in the queue for the second one, so we may deal with both here.

Assigning to myself so I don't miss this. I'll work on it as soon as I can.

Comment #2

danielb CreditAttribution: danielb commented 29 October 2009 at 03:37

Having this problem too! Didn't discover this bug till I created hundreds of nodes by hand assuming this was working :(

Any clues how I can quickly fix this myself? I will look into it after lunch :( argh

Comment #3

danielb CreditAttribution: danielb commented 29 October 2009 at 04:53

I don't see your 'keep_empty' hack in content.module (from comment #47 in the issue you linked)

Comment #4

danielb CreditAttribution: danielb commented 29 October 2009 at 05:16

I notice you keep mentioned CCK 6.x-3.x but no such version exists on the project page. I will need this fixed in 6.x-2.x.
edit: ah found it on the 'all releases' page.
I'd consider an upgrade, but if it doesn't work in 3.x either, then perhaps it isn't worth it.

Comment #5

danielb CreditAttribution: danielb commented 29 October 2009 at 06:33

Hmm OK, I've upgraded to CCK 3.x

Via the GUI - the ordering is preserved.

I don't think that was made clear in the original post of this issue, as node_save() is indeed executed during a node form submission.

In that case - for my usage purposes, there is no apparent bug.

Comment #6

markus_petrux CreditAttribution: markus_petrux commented 31 October 2009 at 20:56

Status:

Active

» Needs review

File	Size
content_multigroup_node_load-577580-6.patch	1.15 KB

Well, I've been thinking about this a bit... this is not so easy as it seems because there are things in the implementation of multigroups that may change. I mean, we are still using separate tables for each field, however we may implement tables at multigroup level some day (see #520956: Add a new storage method for fields in multigroups), and that means we'll have rows for all fields, including those that are empty (NULL in the database).

Even if that happens, during node_load processing we may need to ensure that all deltas are consecutive, so we do not generate holes that are destroyed when node_load/node_save is executed, which is what triggers this issue where we are now.

So, here's a patch that tries to address this issue in that way. What we do here is ensure deltas are consecutive during node_load(). It should be safe enough and cause no trouble with other field implementations, but it could happen, so I won't commit this until some feedback is provided. Please, test with as much fields as possible. Specially critical are fields shipped with CCK package, Date and FieldField/ImageField. Thanks in advance!

Comment #7

markus_petrux CreditAttribution: markus_petrux commented 3 November 2009 at 19:42

Title:	Multigroup fields with empty values loose their ordering when saved via node_save.	» Multigroup fields with empty values loose their ordering when saved via node_save
Status:	Needs review	» Fixed

Committed to CVS.

http://drupal.org/cvs?commit=283572