Alter hooks causing status to always be overridden

My opinion that is is not working as intended. Alters are not part of the export by definition.

The 'correct' semantic solution, in my opinion, is to not have the 'normal' signature be altered. But since that is ctools and other modules job (which they do correctly), the solution is to also allow alters to the default items.

Well that's certainly the path. I feel there may be dragons, i.e. the default drupal_alters are not named the same as the hooks. But its worth some investigation.

Subscribing. Marked #810158: if hook_views_default_views_alter() is implemented, features always detects view in an overridden state as duplicate.

#766264: Alter hooks causing status to always be overridden

Re-opening this because I think it was mistakenly closed as a duplicate of itself.

Never mind, it is a duplicate of #838612: Remove Alterifications from export.

I don't consider this a duplicate of #838612: Remove Alterifications from export, but it is worth noting that they are very closely related.

Here's a patch that solves this problem for me. When doing 'overridden-ness' checks, I make sure no alter hooks run. This makes sense to me, because if you're going to call alter hooks on one, you should do it on both. The opposite is easier to implement, and will probably make the #838612: Remove Alterifications from export people happier.

Thoughts?

@tarmstrong - this does indeed seem to stop default alterations from being re-exported, however the features still appear as 'overridden' when using drush fl

Working on tests in http://drupal.org/node/1402312#comment-5472904 comment that test for altering, and the tests for true exportables (views, image) are failing, whereas the ones for faux exportables (features provided, user_permission, filter, field) are passing.

It seems the alter hooks remain "Overridden" when they add a new item to the exportable array. An example is hook_content_default_fields_alter() - you can alter an existing field all you want but adding a new one will prevent the "Overridden" message from disappearing. Same for hook_user_default_roles_alter().

For these two examples, if you have added roles or fields to the database your feature will appear overridden. If you add the new roles/fields into the alter hook reverting the component will include your alters but the "Overridden" message remains.

I think adding an item during alter stage isn't necessarily supported; alters are for altering existing, not adding new. It's possible cause it's possible, but it breaks how features knows about components and don't think getting features to work with it is a good effort. (there's a patch in views issue queue that is moving views to that (views added during alter won't work).

The patch in #8 doesn't work with faux exportables. If I add an alter to add new perms using hook_user_default_permissions_alter(), it never actually pulls my new perms into the DB.

It almost seems like Features is simply detecting overrides where none exists as far as it is (or should be) concerned.

My take on it is that Features shouldn't care about components it isn't aware of, i.e. exported in the info file. Someone please correct me if this has already been decided otherwise somewhere else, but Features itself should only compare observed exports.

In this line of reasoning, it no longer matters whether CTools or some other module is altering the normal/default on its own. Features couldn't care less about added components when determining the signature. However, this would still allow faux exportables to be rebuilt using the entirety of the altered defaults.

A potential caveat is that Features will no longer detect changes to faux exports added via an alter hook. Whenever extra faux exports are modified, the module update or the administrator shall have to rebuild (now possible in Drush with #1721366: Drush features-revert should detect/revert FEATURES_REBUILDABLE components (features_revert() does)).

EDIT: It should also be possible to force Features to pick it up via hook_system_info_alter().

Let's do it on 7.x-1.x first.

Forgot to change status.

Declaring the dynamic exports hook_system_info_alter() wouldn't work for components like image styles. Feature modules providing image styles without exporting them as features, i.e. by implementing hook_image_default_styles() in the module file, are shown as overridden if the hook doesn't return the full info, i.e. with all the effect info filled in.

Seems like excluding everything not included in features[...] in the info file is still the way to go.

Update: Completely suppress empty exports.

This works pretty well. Image styles (hook_image_default_styles) still report as overridden though when a module implements hook_image_styles_alter().

Yes, I've run into that too, though I thought I had fixed it in #18.

Can you tell me if the hook_image_styles_alter() implementation adds/removes anything?

@zhangtaihao - no, it just alters the width and height of existing styles.

Alterations added with hook_strongarm_alter also still appear in the features exports.

Just wanted to chime in to say that #18 fixes the always overridden problem that gets introduced with the latest file entity and media_youtube dev versions where defaults are introduced via hook_file_default_displays_alter().

Someone will need to resubmit a patch rolled against the 2.x branch for this to be considered. The 1.x branch is frozen.

A straight reroll of #18 against the 2.x branch for testing.

This is also going to need some testing with the Features Override module. Can somebody let me know if this patch changes any interaction with that module? Got my hands full with releases right now.

I plan to wait a couple of weeks to make sure the Features 2.0 release is solid before embarking on any new changes for 2.1-dev. So we have some time to work on this one.

So far looking good. Will need to test with Features Override though.

With alpha3 (25th Oct) of file_entity, which implements hook_file_default_displays_alter(), and #25 patch for features, we're still seeing the image_style of image__default__file_field_image as overridden, after a drush cc all, and a re-export of the feature.

I just killed a couple hours trying to figure out why the feature module with my media module configuration in it kept being listed as overridden, no matter what I did. I'm using:

• media-7.x-2.0-alpha3
• file_entity-7.x-2.0-alpha3
• features-7.x-2.0

I installed the patch from #25 and suddenly Features was happy. No overrides reported.

@alexkb, I seem to be using the same version of File Entity as you and I'm not seeing that error. Are you still having problems with this patch, or can we mark it RTBC?

@jstoller are you making changes to the default view modes of default file entity types like the image?

I think this patch needs test added for it, specially one that reproduces the bug and show that it is fixed via this.

@alexkb: I added a couple new fields to the default Image, Audio and Video file types. I went through each of the display modes (default, teaser, preview) on both the "Manage Display" and "Manage File Display" tabs for all four default media types. For each one I at a minimum hit the save button, so Drupal thought I was overriding it, but in several cases I made changes, particularly with my new fields. I then added all relevant field_base, field_instance, file_display, and Strongarm variable instances to a feature. In all, I think I have 55 file_display values in there from file entities. Before applying the patch I was getting a number of different "overridden" errors across the file entities. After applying the patch, there are no errors.

I'm on features 2.0 and the patch in #25 worked partially, I now can export file entity image defaults but it keeps overridden for one style. I have changed the display of the image > teaser with a custom image style. Although the style is in the code of the feature, it reports overridden .

'image__teaser__file_field_image' => array( 	
    'api_version' => 1 	
    'name' => 'image__teaser__file_field_image',
    'weight' => 50,
    'status' => TRUE, 	
    'settings' => array(
      'image_style' => 'medium',
      'image_link' => 'content',
    ), 	
  ),

VS override

'image__teaser__file_field_image' => array( 	
    'api_version' => 1 	
    'name' => 'image__teaser__file_field_image',
    'weight' => 50,
    'status' => TRUE, 	
    'settings' => array(
      'image_style' => 'default-thumbail',
      'image_link' => '',
    ), 	
  ),

I can confirm that the patch in #25 works for me.

The issue that dreamlabs is encountering in #34 is because the File Entity module is providing a polyfill for the Image module by introducing this function in file_entity.module:

/**
 * Implements hook_file_default_displays_alter() on behalf of image.module.
 */
function image_file_default_displays_alter(&$file_displays) {
  // Images should be displayed as unstyled images by default.
  if (isset($file_displays['image__default__file_field_file_default'])) {
    $file_displays['image__default__file_field_file_default']->status = FALSE;
  }

  $file_display = new stdClass();
  $file_display->api_version = 1;
  $file_display->name = 'image__default__file_field_image';
  $file_display->weight = 50;
  $file_display->status = TRUE;
  $file_display->settings = array(
    'image_style' => '',
    'image_link' => '',
  );
  $file_displays['image__default__file_field_image'] = $file_display;

  // Image previews should be displayed as image thumbnails by default.
  if (isset($file_displays['image__preview__file_field_file_default'])) {
    $file_displays['image__preview__file_field_file_default']->status = FALSE;
  }

  $file_display = new stdClass();
  $file_display->api_version = 1;
  $file_display->name = 'image__preview__file_field_image';
  $file_display->weight = 50;
  $file_display->status = TRUE;
  $file_display->settings = array(
    'image_style' => 'thumbnail',
    'image_link' => '',
  );
  $file_displays['image__preview__file_field_image'] = $file_display;

  // Image teasers should be displayed as medium images by default.
  if (isset($file_displays['image__teaser__file_field_file_default'])) {
    $file_displays['image__teaser__file_field_file_default']->status = FALSE;
  }

  $file_display = new stdClass();
  $file_display->api_version = 1;
  $file_display->name = 'image__teaser__file_field_image';
  $file_display->weight = 50;
  $file_display->status = TRUE;
  $file_display->settings = array(
    'image_style' => 'medium',
    'image_link' => 'content',
  );
  $file_displays['image__teaser__file_field_image'] = $file_display;
}

This is actually an _alter() function so should not interfere with the calculation of overridden states after applying the patch from #25.

Updated the above answer and wanted to shed some more light on this issue.

My recent findings show that a lot of related issues are about people not being able to export Media, File Entity and the like entities. This is because they are trying to export entities already provided by a defaults hook.

Media used to provide its defaults through hook_file_default_displays() but moved away from that approach in favor of hook_install(). See: #1702700: Implementation of hook_file_default_displays makes it impossible to export image display settings via features

File Entity and Media Vimeo (AFAIK) still provide default displays through that hook instead of installing them when the module is installed. This leads people to editing the 'provided defaults', thinking they can still export them afterwards.

Solution: Clone the 'default' entities, disable the 'defaults' and work with the clones. After applying the patch in #25, nothing can stop you from having clean, working features with Media, File Entity and the likes :)

Solution: use features overrides (or fix whatever is preventing features override to work in this case). Works for default views, tmk.

@hefox

Features override does not work for me in this case. Should it be in a different feature then the one in which I am trying to set the default file setttings?

[edit] I just tried to put everything in a second feature, that is still not working.

[edit-2] I think I misunderstood the comment from @hefox as it was in reply to another problem. #25 (still) works for me.

Does Features Override work when the original exportable is not provided through Features, but the code of a 'regular module'?

(As is the case for File Entity, Media Vimeo, etc.)

@37 not that I can see. I have the catalog view provided by uc_catalog and when I customize it none of the changes show up in FEATURE OVERRIDES or FEATURE OVERRIDES (INDIVIDUAL -- ADVANCED).

I believe it's because the view is not provided by a feature but hook_views_default_views.

I wish I was able to export this override! I'm guessing I'm going to need to hook_views_default_views_alter() to get it in code.

@leex (#40)
Yes, that is exactly how it can work:

• Does it come from a feature? Then you can:
  • Use features override
  • Clone the 'defaults', then disable them, then export the clone along with your changes
• Does it come from a defaults hook? Then you can:
  • hook_x_alter() the default
  • Clone the 'defaults', then disable them, then export the clone along with your changes

I like this patch. It makes me warm and fuzzy. +1

However, this does not take affect on a drush fu.

I'm attempting to use patch #25 to get around a hook_default_rules_configuration_alter() causing one of my features to always show as overridden. The rule is provided by the commerce module(s) and I'm simply trying to disable it.

When I applied the patch I was no longer seeing the issue overridden because of that alter hook. But now I'm getting a different override against defaultconfig.

Any ideas?

Could you provide an interdiff.txt? Makes reviewing the changes between #45 and #25 easier.

@rsmylski seems to have only modified this section of the patch - original patch (#25):

+    $module = features_get_features($module_name);
+    if (empty($module->info['features'][$component])) {
+      $objects = array();
+    }
+    else {
+      $objects = array_intersect_key($objects, drupal_map_assoc($module->info['features'][$component]));
+    }

New patch (#46):

+    $module = features_get_features($module_name);
+    if (empty($module->info['features'][$component])) {
+      $objects = array();
+    }
+    else {
+      // Keep track of version if it was added to default feature definition.
+      $version = '';
+      if (isset($objects['version'])) {
+        $version = $objects['version'];
+      }
+      $objects = array_intersect_key($objects, drupal_map_assoc($module->info['features'][$component]));
+      // Add back version if it was stripped.
+      if (!isset($objects['version']) && !empty($version)) {
+        $objects['version'] = $version;
+      }
+    }

Marking Needs Review to trigger the testbot. I just tested the patch from #45 and it worked for me.

#45 worked great for me.

#45 also solved the problem for me!

#45 works. Even applies to 2.2. (With some offsets)
RTBC imo.

I tried to use patch #45, and while it resolved the problem I had with features exporting file_displays, it caused problems with another feature that exported styles (from the 'styles' module). The 'styles_style' component always registered as overridden, with the 'diff' claiming that there was no 'standard' content, in spite of it being present in the 'mymodule.features.inc' file.

I investigated, and this appears to be due to the structure of $objects for the styles_style component. For instance, the style definition for 'file:article_half' can be found in $objects["file"]["styles"]["article_half"] rather than $objects["file:article_half"].

If I replace the 'array_intersect_key' line of patch 45 with

      if ($component=="styles_style") {
        foreach(array_keys($objects) as $type) {
          if (!is_array($objects[$type]) || empty($objects[$type]["styles"])) continue;
          $objects[$type]["styles"] = array_intersect_key($objects[$type]["styles"], drupal_map_assoc(preg_replace("%^$type:%", "", preg_grep("%^$type:%", $module->info['features'][$component]))));
        }
      } else {
        $objects = array_intersect_key($objects, drupal_map_assoc($module->info['features'][$component]));
      }

everything works.

@MRV, can you post a patch of your changes, please ?

Patch #45 + last comment of @MRV.

This patch (or #45) produces a curious side effect with some featured taxonomy terms. It needs more investigations.

To summarize quickly: a Feature, whose status is 'Default' without this patch, is marked as 'Overridden' with this patch for some taxonomy terms.

needs review based on above comment, also simple code style issue/s ( } else {)

Fixed code style.

AFAIK taxonomy terms can not be exported with solely this module.

Maybe you meant taxonomies but I don't experience any issue with this.

For me the patch is ready to go and I use it daily.

The patch applies to features-7.x-2.4 without problems.
I didn't have any issues before applying the patch and everything still seems fine with the patch.
Sorry, i'm not much help having not encountered the problem.

Maybe it needs reassuring that the bug is still there, trying the steps in #35 or #44

There's 46 people subscribed, maybe it's not features fault.
I kinda apply the patch in every project like the monkeys in this experiment http://skeptics.stackexchange.com/questions/6828/was-the-experiment-with...

@rodrigoaguilera Thanks for the laugh :-)

@rodrigoaguilera What are you trying to export? As I stated in #36, the problem is usually not with Features, but modules providing defaults incorrectly.

OK

I stopped being a monkey and removed the patch. No feature is overriden.
Anyone is free to open it again with some steps to reproduce it.

ok, I'll also stop being a monkey see monkey do and remove the patch until the problem is clarified. :-))

Don't mind me still monkeying around "eek eek". Re-roll b/c it was easier than tracking down the culprit for this older project.