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.

Comments

ericduran’s picture

Status: Active » Needs review
StatusFileSize
new1.15 KB

Here's a patch for the issue.

This is a big problem currently.

jhedstrom’s picture

Priority: Normal » Major

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

hefox’s picture

Status: Needs review » Needs work

Think 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 :/

ericduran’s picture

Issue tags: +Needs tests

Yea, this definitely needs test and drupal_array_diff_assoc_recursive() does make more sense.

elijah lynn’s picture

Title: Updating field base even thought they're no changes » Updating field base even though there no changes
Issue summary: View changes
elijah lynn’s picture

Issue tags: +Performance
jhedstrom’s picture

StatusFileSize
new1.22 KB
new1.2 KB

Here'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.

RavindraSingh’s picture

Status: Needs work » Needs review

Pushing for needs review. As I have tested #8 is working fine.

mpotter’s picture

Priority: Major » Normal
Status: Needs review » Reviewed & tested by the community

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

  • mpotter committed 30a4247 on 7.x-2.x authored by jhedstrom
    Issue #2270619 by jhedstrom, ericduran: Updating field base even though...
mpotter’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 30a4247.

Status: Fixed » Closed (fixed)

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