Default file entities are not exportable by features (Media File Entity Overridden)

I just wanted to chime in here and add that I am experiencing the same issue. I'll add these related issues here:
#1345174: Alter hook implementations lead to false overridden status for features components
#1042088: Feature stuck in overridden state due to buggy hook detection

The patch in https://drupal.org/node/1042088#comment-7796697 does not fix this. So far I had been reluctant to try out the patch by das-peter, but I can confirm it does solve the issue after applying it.

#1 Patch worked for me.

#24 in #1042088: Feature stuck in overridden state due to buggy hook detection did not work for me which was committed recently.

My original comment is https://drupal.org/node/2071073#comment-7929987

Why was this posted with a "do not test"? I can't accept any patch that doesn't pass the automated tests. So please post an updated patch and let the tester do it's work, then we can evaluate it.

My guess is that this is a very bad idea and will break code that adds alter hooks (such as Features Override, or code-based alters to views, etc). It sounds more like the features_export_render function used in Media needs to add the stuff being added by the alter hook to the export.

It doesn't seem like Media is using (hook_)features_export_render.

Maybe they are using alter_hooks to change the File Entity data. In which case, marking the File Entity feature as "overridden" would be correct behavior.

To detect Overrides, what Features does is:

1) (get_features_normal) Calls the hook_features_export_render to convert the current DB values into code, which it then evals.
2) (get_features_default) Calls the default_hook function to return the data represented by code, including any alter hooks.
3) compares md5() of 1 and 2. If different, feature is marked as Overridden.

So, if a module just implements an alter hook, that is modifying (1) without changing (2), it's marked as overridden. In other words, it is telling you that the code exported by Features does not represent the full state of the data on your site, which is correct in this case...Media is changing the data that was stored in your File Entity feature. You'd somehow need to get Media to export/render the stuff that it adds to the feature.

@mpotter I can't look into this issue much today because of other obligations, but before anything else I just want to say how much I appreciate the time you take to explain things in such a clear way.

As far as I understand this should be fixed in the Media and File entity modules respectively. I suppose (new) issues should be made there, but I'd appreciate if this issue could stay open for just a moment when I gather my thoughts and can actually find the time to try and do something about it (hopefully tomorrow). As a side-note, here the alters that cause the behaviour:

media.module, line 1074

/**
 * Implements hook_file_default_displays_alter().
 */
function media_file_default_displays_alter(&$file_displays) {
  // Image previews should be displayed using the media image style.
  if (isset($file_displays['image__preview__file_field_image'])) {
    $file_displays['image__preview__file_field_image']->settings['image_style'] = 'media_thumbnail';
  }

  // Video previews should be displayed using a large filetype icon.
  if (isset($file_displays['video__preview__file_field_file_default'])) {
    $file_displays['video__preview__file_field_file_default']->status = FALSE;
  }

  $file_display = new stdClass();
  $file_display->api_version = 1;
  $file_display->name = 'video__preview__file_field_media_large_icon';
  $file_display->weight = 0;
  $file_display->status = TRUE;
  $file_display->settings = '';
  $file_displays['video__preview__file_field_media_large_icon'] = $file_display;

  // Audio previews should be displayed using a large filetype icon.
  if (isset($file_displays['audio__preview__file_field_file_default'])) {
    $file_displays['audio__preview__file_field_file_default']->status = FALSE;
  }

  $file_display = new stdClass();
  $file_display->api_version = 1;
  $file_display->name = 'audio__preview__file_field_media_large_icon';
  $file_display->weight = 0;
  $file_display->status = TRUE;
  $file_display->settings = '';
  $file_displays['audio__preview__file_field_media_large_icon'] = $file_display;

  // Document previews should be displayed using a large filetype icon.
  if (isset($file_displays['document__preview__file_field_file_default'])) {
    $file_displays['document__preview__file_field_file_default']->status = FALSE;
  }

  $file_display = new stdClass();
  $file_display->api_version = 1;
  $file_display->name = 'document__preview__file_field_media_large_icon';
  $file_display->weight = 0;
  $file_display->status = TRUE;
  $file_display->settings = '';
  $file_displays['document__preview__file_field_media_large_icon'] = $file_display;
}

In file_entity.module, line 2154

/**
 * 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($items['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($items['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($items['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;
}

So if I understand correctly, Media and File entity should have an features_export_render function, so that the stuff added in the alter can be added to the export, correct?

Yep the work needs to be done in Media & File Entity.

Updated title to better represent the issue.

I'm gonna be bold and put this thing on duplicate of #766264: Alter hooks causing status to always be overridden

The patch in this issue fixed it for me:
https://drupal.org/comment/7975839#comment-7975839

@dagomar, I can't get our feature to revert to default, with or without the patch in #766264: Alter hooks causing status to always be overridden. But (again with or without that patch) if I define hook_file_default_displays_alter() in a module and set my image__default__file_field_image file display default image_style, then the module is no longer overridden. This isn't a solution though, because then the settings are defined in the feature export as well as an alter function.

Looking back through file_display's commit history, image_file_default_displays_alter() was added with very little testing: #2067909: Default file display configuration depends on Standard profile image styles. I've commented over there, where the problem for file_entity, at least, should be resolved.

alexkb you are completely right. I'll reopen.

We are also having this problem in Open Atrium.

This is not something to be fixed in Features. The problem is that the Media module is using the *_alter to override the file display settings. So when you have a site that wants to have different settings and that site uses Features to save the defaults, you end up with overridden Features.

To make this worse, because of the *_alter in Media, exporting the file display settings prevents Features from importing the correct value.

For example, if I change the Document Preview to use "Generic File" instead of "Large File Icon" and then export it to Features, the export actually contains the correct data (Generic File). But when I move this Feature to a new site and try to revert my feature, the File Display will still say "Large File Icon" in the UI. (And this is correct behavior for how alter hooks work).

In general, using the media_file_default_displays_alter() method is just the wrong way to handle this. The Media module should not be dictating what the file display modes should be. The Media module should set the *default* display modes, perhaps when the module is enabled.

The fact that File Display settings can't be properly deployed from dev->prod via Features still makes this a pretty serious issue.

I don't expect this to be committed, but here is a simple patch that removes the media_file_default_displays_alter() function for sites or distributions that already export their file type display settings via features and don't want them overridden.

The real solution as far as I can understand are the comments in #6 and #8 by the features guys. Media module should "It sounds more like the features_export_render function used in Media needs to add the stuff being added by the alter hook to the export.".

I talked with @mpotter about this a bit yesterday - we both agree that the real solution is for Media to add these default displays to the database via hook_install() so that they can be modified and or replaced entirely via the default hooks (probably via Features). That was most other modules do in similar situations.

In the interim, I'm probably going to use a patch similar to @mpotter's in Panopoly.

I agree that the correct fix should be for File Entity to create these settings in the database on install, and for media module to update those settings when it gets installed, rather than providing them in _default and _default_alter hooks. This is very similar in nature the the media issue that I reported and fixed in #1702700: Implementation of hook_file_default_displays makes it impossible to export image display settings via features by doing the same.

The latest patches resolve the issue with a feature being overridden; however, Media thumbnails are now broken.

I overlooked the schema update. Running update.php corrects the thumbnails issue. The patches work for me. Thanks for getting the ball rolling on this issue again.

Is there a sibling issue in the File Entity queue that the second patch from #23 can live in? We're clogging this issue with failed tests and I assume people aren't testing the patch because they see the failed tests at the bottom and move on.

Patch media @#23 works for me too. Thanks. I take a look at the file_entity patch now.

Could we get it updates to people that want to join know, what patches, how to test and what issue it fixes, version (1.x or 2.x) of Features. Does it req. patch of file_entity also and so on?

#38 might help. I tried applying both the File Entity patch (from the sibling issue) and the Media patch from #23 and then did a fresh install. My first action on the new site was to create a Feature to export all the default file entity settings. After enabling the Feature, I am still seeing the overridden state. Reverting does not fix anything, nor does clearing caches. Maybe I am missing a step.

@sylus - it looks like the WetKit versions of File Entity is from October?

Shouldn't you be testing your patches against the latest HEAD for File Entity?

This might be the reason you are seeing different results. Or maybe not...

It also seems the *.file_default_displays.inc you are linking to does not deal in the default entity types that ship with File Entity.

ie: wetkit_video__default__file_image vs image__default__file_field_image

Perhaps I'm just confused - and this might totally be the case, so I apologize in advance - but I was hoping these patches would fix the issues with default file settings.

Okay - I am not sure then, but I'll go ahead and attach a Feature I created and also a screenshot of the overrides page.

We have been using panopoly_images and panopoly_widgets too - but I removed those for this test so that no other Features on the site were dealing in File Entity defaults.

Ah gotcha. I've tried another test removing Media Youtube and Media Vimeo and we're in default. Good times. Looks like a patch for those modules will need to follow.

Re #47 - yeah, I removed the panopoly modules for all tests and did a fresh install.

@sylus / @brantwynn: Thanks for rocking out these issues! We're planning to use these patches for the next version of Panopoly. :-)

FYI, there is a patch for media_youtube that was already committed upstream in the latest -dev: #1863788: File display settings not exported

I believe a recent change to the module broke the patch in comment 23
https://drupal.org/comment/8396877#comment-8396877

17: media_remove_file_display_alter-2104193-17.patch queued for re-testing.

23: media_remove_file_display_alter-2104193-23.patch queued for re-testing.

Yup, patch is not applying, marking as needs work...

Needs a re-roll since update function 'media_update_7226()' now exists and is taken.

Here is a re-rolled patch for the latest current dev
The only change is to the update function version number.

This is super useful for distributions! However, I don't feel safe using this patch until it gets committed upstream because it's chasing the latest hook_update_N(). If we were to use this patch in our distribution (before it's committed) and upstream added a new hook_update_N(), we'd have problems with upgrades, because our users' schema version would already be at the highest N in the upstream version. :-/

However, the patch works great! I can't wait until it gets committed. :-)

65: media_remove_file_display_alter-2104193-65.patch queued for re-testing.

Woohoo! Thanks, @aaron. :-)

Since the commit was reverted, I assume it's 'Needs work'.

Here's the info about why it was reverted: #2271909: Default file display for file entities no longer set

Pff. I guess we have to test and figure out what was going wrong that caused the displays not to get setup correctly! I was really looking forward to integrating the latest versions of Media/File Entity into Panopoly. :-/

I tested the file_entity patch, it worked for me and I made a few minor changes: #2192391: Default file entities are not exportable by features (Sibling Issue)

However, this patch wasn't working for me. :-/ There was a bug in it's logic: it was only adding the new display if file_displays_load($type, $view_mode) returned nothing! This means it would only add the display if there were no displays on that view_mode, which was obviously not the case, because file_entity added several displays in it's hook_install().

I've updated the patch to check for the specific display.

Please let me know what you think!

I tried to test this patch along with its counterpart from #2192391: Default file entities are not exportable by features (Sibling Issue).

They apply cleanly.

I exported a test feature and was able to export all the file_entity displays.

I then reinstalled using the standard profile. Strangely, the only thing that I see for overrides is that the image style for 'Preview' is set to 'media_thumbnail' in my Feature but the default after a reinstall is 'thumbnail'

This might be a product of the environment I set up. I am not sure.

Overall, I am not sure if this is correct for ongoing updates but it seems to be a good fix for distributions during install.

Thanks, Brant! :-)

I then reinstalled using the standard profile. Strangely, the only thing that I see for overrides is that the image style for 'Preview' is set to 'media_thumbnail' in my Feature but the default after a reinstall is 'thumbnail'

Hrm, this is from Media's media_file_default_displays_alter():

http://cgit.drupalcode.org/media/tree/media.file_default_displays.inc#n56

I'm really not sure what to do about that. :-/ We could have Media update the Preview display one-time, but that could be weird if a user updated that in their configuration, installing Media will suddenly overrwrite it. Any thoughts?

I can confirm the two patches (2104193 & 2192391) apply cleanly on 2.x-dev and allow my File Entity + Media feature to correctly install the exported file displays without adding any default ones.

Here's my feature's drush makefile:

; Media
projects[file_entity][version] = 2.x-dev
projects[file_entity][patch][] = https://drupal.org/files/issues/fix-pdfpreview-formatter.patch
projects[file_entity][patch][] = https://www.drupal.org/files/issues/file_entity_remove_file_display-2192391-16.patch

projects[imagemagick][type] = module

projects[media][version] = 2.x-dev
projects[media][patch][] = https://www.drupal.org/files/issues/media_remove_file_display_alter-2104193-76.patch

projects[mediaelement][type] = module
projects[mediaelement][patch][] = https://drupal.org/files/responsive_width-1348816-7.patch
libraries[mediaelement][download][type] = file
libraries[mediaelement][download][url] = http://github.com/johndyer/mediaelement/zipball/master
projects[pdfpreview][type] = module
projects[pdfpreview][patch][] = https://drupal.org/files/pdfpreview-black.patch

My recovered sanity thanks you so much!

@PascalAnimateur: If they work for you, can you mark both issues as "Reviewed & tested by the community"? Thanks!

Actually, sorry, I think I'm going to mark this as "Needs work". :-) The issues in comment #77 and #78 need to be handled somehow. :-/ But thanks for testing!

@dsnopek

How about moving the setting of image__preview__file_field_image to media_thumbnail in media.file_default_displays.inc
to the install file as:

$display = array(
    'api_version' => 1,
    'name' => 'image__preview__file_field_image',
    'status' => 1,
    'weight' => 50,
    'settings' => array(
      'settings' => array(
        'image_style' => 'media_thumbnail',
        )
      ),
    'export_type' => NULL,
  );
  file_display_save((object) $display);

Then when the feature is set it will over ride what was loaded at install.

@bpleduc: That's pretty much exactly what this patch does. Give it a try! :-)

@dsnopek - Thanks for the super fast reply! :) I'll explain what I'm understanding below:

#78

I then reinstalled using the standard profile. Strangely, the only thing that I see for overrides is that the image style for 'Preview' is set to 'media_thumbnail' in my Feature but the default after a reinstall is 'thumbnail'

#78

this is from Media's media_file_default_displays_alter():

So could we remove this code from file: media.file_default_displays.inc

function media_file_default_displays_alter(&$file_displays) {
  // Image previews should be displayed using the media image style.
  if (isset($file_displays['image__preview__file_field_image'])) {
    $file_displays['image__preview__file_field_image']->settings['image_style'] = 'media_thumbnail';
  }
}

And add this code to file: media.install

 $display = array(
    'api_version' => 1,
    'name' => 'image__preview__file_field_image',
    'status' => 1,
    'weight' => 50,
    'settings' => array(
      'settings' => array(
        'image_style' => 'media_thumbnail',
        )
      ),
    'export_type' => NULL,
  );
  file_display_save((object) $display);

Maybe there is a reason that media_file_default_displays_alter function is needed in file media.file_default_displays.inc?

I tried this out and it seemed to get over the problem of image__preview__file_field_image being set to media_thumbnail and the feature not showing default.

Thanks in advance :)

Oh, ok, sorry, I think understand now!

So you're saying that File Entity creates the original display (in the database), and then Media could just overwrite it (again, in the database)?

The problem with that, is that if the user customized the display after installing File Entity, but before installing Media, those customizations would be overwritten by Media. And when Media is disabled, the display would still refer to an image style that Media defines (but is no longer present).

Those are both problems that the existing implementation (before the patches) doesn't have. Since the existing implementation updates the default from code, it won't affect the user settings if they have customized them (in the database). And, if the user hasn't customized the settings, Media's alterations to the defaults will be removed on uninstall.

So... Actually, I think we probably could make it work! We'd just have to check that user hasn't customized the File Entity defaults before overwritting them, and add a hook_disable() to Media to restore the File Entity defaults if 'media_thumbnail' is still set as the image style. This isn't an idea I had previously considered! :-)

This is what I'm understanding:
FLOW

1) Install File Entity
2) User Makes Changes - Customizing display
3) Install Media 
	a) Has user changed defaults?
		i) Yes - Save defaults
		ii) No - Continue
4) Uninstall Media
	a) hook_disable()
		i) Restore defaults

Questions:
1) Where is best place to store the defaults 3(a)(ii) ?
2) This requires movement of the code #85 from file: media.file_default_displays.inc

function media_file_default_displays_alter(&$file_displays) {
  // Image previews should be displayed using the media image style.
  if (isset($file_displays['image__preview__file_field_image'])) {
    $file_displays['image__preview__file_field_image']->settings['image_style'] = 'media_thumbnail';
  }
}

The image__preview__file_field_image = media_thumbnail would then be stored with the other defaults which are restored on uninstall correct?

Answers to questions:

1) In 3.a.ii, I don't think you actually need to store anything, you just don't touch what the user has done. Maybe set a warning message, so the user knows that they would have to change the image style by hand if they want media_thumbnail. Also, in 4.a.i, I think you just have to check if media_thumbnail is being used, and if so, restore the image style that file_entity normally uses.

2) Sorry, I don't understand this question.

@dsnopek

File: media.install

  foreach ($default_types as $types) {
    foreach ($default_image_styles as $view_mode) {
      $display_name = $types . '__' . $view_mode . '__file_field_media_large_icon';
      $existing_display = file_displays_load($types, $view_mode);
      if (empty($existing_display[$display_name])) {
        $display = array(
          'api_version' => 1,
          'name' => $display_name,
          'status' => 1,
          'weight' => 50,
          'settings' => NULL,
          'export_type' => NULL,
        );
        file_display_save((object) $display);
      }
    }    
  }

Doesn't this line ensure we don't touch anything set?
if (empty($existing_display[$display_name])) {

Well, first of all, that's a different display: "file_field_media_large_icon". But also, it's only checking if it exists. In the possible implementation we're discussing, it WILL exist, and we're going to change it, but only if it hasn't been changed from what file_entity set it to when it was install. I hope that makes sense!

Patch here seems to address the concerns in #77 & #78.

@heddn: Looking at the interdiff, that looks perfect! However, I haven't actually tested it, so marking as "Needs review".

patch in #91 works great for me - thanks so much! That helped resolve lots of frustration!

Waiting on Issue #2192391: Default file entities are not exportable by features (Sibling Issue).

Wouldn't postponed be a better status?

Can anyone comment on Devin Carlson's objection to the patch for the sibling issue #2192391-27: Default file entities are not exportable by features (Sibling Issue)? I'm not sure I understand what the original problem with the module was or why the defaults need to be stored in the database. The referenced issue #1702700: Implementation of hook_file_default_displays makes it impossible to export image display settings via features mentions something about Features having a "competing" implementation of hook_file_default_displays(), but this did not appear to be the case (apologies if I'm not seeing something). Any insights would be really appreciated.

Let's get a statement from the features maintainers on this.

Ok I think it's good to ping this issue too.

I've just added a patch to #2192391: Default file entities are not exportable by features (Sibling Issue) that might solve that issue once and for all. It simply means that media should provide it's default display settings in hook_file_default_displays() (without alter!) as it is right now. Together with the patch for file_entity this should "just work".

Please check if the patch in #2192391: Default file entities are not exportable by features (Sibling Issue) fixes the issues with media too.

Re-rolled #91 to apply cleanly on 7.x-2.0-alpha4+30-dev

This is a shot at re-rolling for beta1.

Here's another approach similar to #2192391-59: Default file entities are not exportable by features (Sibling Issue) that relies on ctools API to force evaluation of non-features first, allowing features to override file default displays.

#101 works really we
+1 RTBC

#101 works well

I've created a more generic version of #101 in #2446485: Proper way to define module defaults. that will solve this for file_entity and media at the same time. This would also make the patch in #101 obsolete.

Handling module defaults vs. features is simply something that is not yet included in the design of features. So in the end the issue has to be solved there - everything else is just a workaround.

For anyone interested in patch #100 re-rolled against media-7.x-2.0-beta13 ...

looking into #104 as possible replacement for #105, not sure yet which solution would be best.
#2446485: Proper way to define module defaults.

Postponing, waiting for #2446485: Proper way to define module defaults.

I suggest those looking into this try the patch in #2446485 instead before using patch 105 (as a last resort only), as it could put your schema off by interfering with your system record for this module (could put you out of sync with dev /release).

I'd prefer to wait on #2446485

Altered for the new 2.0-beta14

Updated #108 patch to add missing semi-colon.

Still waiting for #2446485: Proper way to define module defaults.

Updated patch from #109 to work with versions 2.5 and 2.6 (update hook collision)

Re-roll of patch with latest 7.x-2.x 6331458 (7.x-2.9)

Been a while, but I could be convinced to take patch #113 instead of #2446485: Proper way to define module defaults.

Looks like a lot of work went into this. The schema changes (hook_update changed) are few and far between now so this might be worthwhile making the change.

Can someone please confirm success of both a fresh install with this patch and a dirty upgrade after applying this patch?

Thanks. I am willing to put this into 7.x-3.x right away and take a bit of time before pushing it into 7.x-2.x as generally we can take more risk in 7.x-3.x and use it as a buffer to confirm successful changes.

2446485 still has an issue and is maybe too complicated dealing with media and file_entity at one instead of just media. No reported issues with latest patches here and a long time reviewed. Probably safer going with patch 113 here than #2446485

Like I said, bake time in media 7.x-3.x branch, we'll see what happens I guess.

Any last words?

I think we should try and get more tester on #2446485: Proper way to define module defaults. and get it committed to the Features module.

There was security update 25.4.18 and I tested possibility to upgrade media 2.x (from jan 2015) to 2.19. Our site is using this patch but it fails the build. Instead I patched the old version to match security issues with (https://cgit.drupalcode.org/media/rawdiff/?id=1cd77ffa9c2cf96d80b76d4731...).

So heads up if sometime this is going into 2.x

Are there any plans to make this work compliant with latest stable version?

Recent versions of media have resolved most of peoples concerns and is compatible with entity translation, multilingual and various advanced configurations. Due to the high volume of inactive and most often irrelevant issues we are Closing this as (outdated). If for whatever reason this issue is important to you AND you still have issues after checking the media recipe documentation, then let us know and we will review your concerns.

Otherwise, see the recipe documentation for how to configure media and for troubleshooting tips OR refer to the media_dev distribution if you want to see a working media setup.

As mentioned, feel free to make some noise in this issue if you still feel it is important to you or someone else.

Thanks,

Media team

As we have new git auth model at git.drupalcode.org, this patch url:
https://cgit.drupalcode.org/media/rawdiff/?id=1cd77ffa9c2cf96d80b76d47318179a8a82f0d46
should be changed to this:
https://git.drupalcode.org/project/media/commit/1cd77ffa9c2cf96d80b76d47318179a8a82f0d46.diff