A Feature is NOT marked as Overridden when the changes to code only add arrays that contain values that evaluate to FALSE. I encountered this specifically with field_bundle_settings_node__NODE_TYPE where the Feature was adding display settings to hide a new extra field on a particular view mode. This adds a section to the variable which looks like:

      // ...
      'display' => array(
        // This is the bit added.
        'field_name' => array(
          'default' => array(
            'weight' => '0',
            'visible' => FALSE,
          ),
        ),
      // ...

However, it possible to reproduce with a reduced set of steps:

  1. Install Drupal 7.38, Features 2.6 and Strongarm 2.0
  2. Run this Drush command to set a variable with an array that contains other arrays:
    drush php-eval 'variable_set("features_testing", array("key" => array("stuff" => array())));'
    
  3. Create a new Feature which contains the 'features_testing' variable
  4. Enable the new module
  5. Edit the MODULE.strongarm.inc and add a new array under "key" that's a sibling to "stuff", which contains all values that evaluate to FALSE. So, for example this what it looked like originally:
      $strongarm = new stdClass();
      $strongarm->disabled = FALSE; /* Edit this to true to make a default strongarm disabled initially */
      $strongarm->api_version = 1;
      $strongarm->name = 'features_testing';
      $strongarm->value = array(
        'key' => array(
          'stuff' => array(),
        ),
      );
      $export['features_testing'] = $strongarm;
    

    And then change it to:

      $strongarm = new stdClass();
      $strongarm->disabled = FALSE; /* Edit this to true to make a default strongarm disabled initially */
      $strongarm->api_version = 1;
      $strongarm->name = 'features_testing';
      $strongarm->value = array(
        'key' => array(
          'stuff' => array(),
          // Added this!
          'otherstuff' => array(
            'visible' => FALSE,
            'weight' => 0,
          ),
        ),
      );
      $export['features_testing'] = $strongarm;
    
  6. Clear all caches
  7. Run 'drush fr' on your Feature and it'll say "Current state already matches defaults, aborting."
  8. But if you do 'drush vget features_testing' you'll see that the variable still has the old value!
  9. If you edit the MODULE.strongarm.inc and make any of the values to something that doesn't evaluate to FALSE, for example, changing 'weight' to 1, then Features will see the changes!

When I downgrade Features to 2.5, it correctly marks the Feature as "Needs review" and running 'drush fr' will change the variable to have the value specified in code.

Using 'git bisect', I was able to find the exact revision that introduces this problem:

http://cgit.drupalcode.org/features/commit/?id=19057448a5960f7d0561711b6...

... which is from the first patch committed on #2497139-4: Alter hooks causing overrides due to object properties not sorted

I'm still not entirely sure what it is about those changes that causes this problem, but at least we know where to start. :-)

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dsnopek’s picture

Issue summary: View changes
dsnopek’s picture

Status: Active » Needs review
FileSize
1.43 KB

Here is a patch that fixes it by changing the default value for $remove_empty parameter on features_serialize() and _features_serialize() from TRUE to FALSE.

I don't know what other implications there are for this change, but it matches the behavior that Features 2.5 and lower had, so I'm hoping that it won't break anything important (since Features 2.6 has only been out for a short while).

Please let me know what you think!

dsnopek’s picture

Actually, looking at the changes in:

http://cgit.drupalcode.org/features/commit/?id=19057448a5960f7d0561711b6...

... it appears that Features used to sanitize the array's ALWAYS and there just was no argument to control it. :-/

So, I'm not sure why it failed to remove empty data from the strongarm in the past, but does now. Maybe it's because it couldn't descend into objects until after those changes?

In any case, I'm not entirely sure why Features wants to remove empty values in the first place, since an "empty" value like 0 or FALSE could still need to be reverted...

dsnopek’s picture

Chatted with @mpotter a bit on IRC and he made a case for removing empty values from items in the database when comparing overrides.

Here's an alternative patch that doesn't remove empty values from the items in code when comparing overrides, only from the values in the database. This doesn't seem quite right either, but it also solves the problem as shown in the steps described in the issue summary, but I'm pretty sure it'll have colateral damage. It's more of a conversation starter than a solution. :-)

Status: Needs review » Needs work

The last submitted patch, 4: features-remove-empty-2542150-3.patch, failed testing.

mpotter’s picture

The commit mentioned in #3 was designed to ignore empty arrays and keys when comparing code with DB for determining what is overridden. The whole idea was to prevent a module that just adds some empty settings (like to a field) from causing a feature to be marked as overridden.

For example, the references_dialog module adds a bunch of settings to field instances. So just turning on this module can cause any entity-reference field feature to be marked as overridden. If you export the feature, these empty settings get added to the export. Then you move the feature to a site without references_dialog and the feature is permanently marked as overridden until you enable that module. We are trying to prevent this case.

So in this regard to the OP, Features sounds like it's working "as designed" now. The fundamental question we need to ask is how much change dictates marking something as overridden. Maybe instead of just using empty() we need to check only for empty arrays, so that FALSE and 0 values do not trigger the change. In the OP, using the --force option like "drush fr --force myfeature" would allow you to update the variable correctly.

There probably isn't going to be a clean solution or answer to this. In some case you might want it marked as an override, and in some cases not. The purpose was to prevent those extraneous overrides that cannot be reverted. The side effect as you've seen is to have some hidden overrides that require using --force.

cboyden’s picture

@mpotter: How do you pass in the force option when you're reverting the feature in an update hook, as opposed to via drush?

bleedev’s picture

Check only for empty arrays, so that FALSE and 0 values do not trigger removal.

bleedev’s picture

Update previous patches formatting

cboyden’s picture

Status: Needs work » Needs review
nayanalok’s picture

Here is the updated patch.

xaa’s picture

Patch from #11 is working.

My case:
- creation of basic rule related to Registration (check registration status, send notif if updated).
- make a change in the rule UI or via code (eg fix a typo in the mail subject & body).
>> features don't see the changes. Status stuck in 'default'.

Once the patch applied the status is "overriden" instead of "default".

This issue seems critical as you can't know in some cases if a feature has changed or not without, right?