This is similar to #2088771: Unable to update a field/field_instance/field_base when equality operator things not-same arrays are the same or maybe it was introduced in that issue.
The problem is that (array(1,2) == array(2,1)) === false;
So this line of code:
$existing_field = $existing_fields[$field['field_name']];
if ($field + $existing_field !== $existing_field) {
field_update_field($field);
}
Is always going to run because of the order after $field + $existing_field is going to be off those the extra keys that need to be merged in.
This is normally ok but it gets pretty bad when the field in question has a index on it. In this case the update will try to recreate the index, causing the revert to blow up.
One thing we could do is change it to "empty(array_diff($field+$existing_field, $existing_field))" or if we want to be extra picky "array_diff($field+$existing_field, $existing_field) === array_diff($existing_field, $field+$existing_field)"
Those both will skip the updates for fields that don't need to be updated.
| Comment | File | Size | Author |
|---|---|---|---|
| #8 | features-field-update-2270619-08.patch | 1.2 KB | jhedstrom |
Comments
Comment #1
ericduran commentedHere's a patch for the issue.
This is a big problem currently.
Comment #2
jhedstromJust ran into this issue. The very odd thing was this just started occurring...no update to Features, just the addition of a field to an unrelated entity type triggered this.
I wonder if we need
drupal_array_diff_assoc_recursive()since field definitions can be multidimensional?Also, while debugging, I wondered why in some places in Features, calls to
field_update_field()are wrapped in exception handling, while here they are not.Regardless of that, patch in #1 fixed the immediate problem.
Comment #3
hefox commentedThink this needs tests. Haven't seen the issue in person myself Can you provide a full example of what is happening?
Need works based on jhedstrom comment -- array_diff alone probably won't do it.
Exception handling has been added on an as needed basis as errors have come up :/
Comment #4
ericduran commentedYea, this definitely needs test and drupal_array_diff_assoc_recursive() does make more sense.
Comment #5
elijah lynnComment #6
elijah lynnComment #7
elijah lynnComment #8
jhedstromHere's a patch that switches to use drupal_array_diff_assoc_recursive. I have not had time to write tests so leaving at needs work.
Comment #9
RavindraSingh commentedPushing for needs review. As I have tested #8 is working fine.
Comment #10
mpotter commentedThis is working well for me also. I'm going to mark this for the next release even without a specific test because it's pretty clear to me that the original implementation is a problem and the new version is a more correct solution to the same logic.
Comment #12
mpotter commentedCommitted to 30a4247.