This is obviously going to be confusing to users. Why is this harcoded:

 if ($file->type == 'image') {
    if ($view_mode == 'media_preview') {
      $image_style = 'square_thumbnail';
    }
    elseif ($view_mode == 'media_large') {
      $image_style = 'large';
    }
    elseif ($view_mode == 'media_original') {
      $image_style = '';
    }

into the function as opposed to just setting the default settings upon install?

CommentFileSizeAuthor
#9 media-exportables-1141082-9.patch13.22 KBeffulgentsia
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

JacobSingh’s picture

Category: bug » feature

Media Gallery will also need this to get working again since it sets default displays for the view modes it creates.

JacobSingh’s picture

Title: Use of media_file_type_media_default_view causes file display config screens to seem like it is unconfigured. » Provide an exportables format for declaring defaults via ctools
Issue tags: +configuration

So here's how I see this going:

Currently the configuration is stored in a variable. This is fine, but it doesn't really allow configuration in code. There is an alter function, but this only applies on the display side, not the admin side. If we used the alter function on the admin side, you could have code which was overriding what the administrator was setting... Ctools exportables was made to handle this mess if I understand correctly.
Right now, the configuration is a multi-dim array of:
$type
- $view_mode
-- $display
--- status
--- weight
--- settings (which is also an array, but I suppose could be anything)

It looks like this:

Array
(
    [image] => Array
        (
            [default] => Array
                (
                    [file_image] => Array
                        (
                            [status] => 1
                            [weight] => 0
                            [settings] => Array
                                (
                                    [image_style] => medium
                                )

                        )

                )

        )

)

From what I can tell, ctools works on DB tables, so we'd need to move away from the serialized blob method here which is convenient, but not very extendable.

If we transform it into a schema, I imagine it would look like:
file_display
- machine_name (autoset to CONCAT(file_type, "::", view_mode);
- file_type
- view_mode
(pkey machine_name)

file_display_settings
- file_display_key FK to file_displays.machine_name
- display_name
- status (is this required?)
- weight
- settings (serialized blob).
pkey(machine_name, display_name)

David_Rothstein’s picture

Subscribing. Yes, I think it sounds reasonable to use ctools exportables for this.

JacobSingh’s picture

Okay, I got this working. Woot!

I made a branch for it too:
ctools_file_entity_1141082

which is pushed up. I'm not posting a patch here since it's not done yet (still needs docs and may want to refactor).

The part I'm not sure about is whether we want a normalized table or not. The primary key of $type::$view_mode::$display_name is unfortunate, but baked into ctools. See #924236: Primary key auto-detection, support for compound primary keys for more on this.

So given that we have that, and we generally can just load all display defs and cache them everytime, it might be simpler and better to have a two column table:
$name | $definition

Where $definition is a big ol' blob. This probably makes migration and update a little trickier, but reduces some array confusion...

Thoughts? Anyway, what I've got in the branch works and will allow us to eliminate mode of the media_file_type_media_default_view() function and allow us to fix media_gallery. I think as a next step, we should take media_file_type_media_default_view() and change it to be an alter since we can supply the defaults with ctools.

Best,
Jacob

JacobSingh’s picture

Added support for the default import / export UI.

effulgentsia’s picture

Subscribe. No time to look right now, but very interested when I get a chance.

neurojavi’s picture

I'm trying to export some file_display setting into a feature and I've found that they are stored in the file_displays array. I think this is what I'm looking for.
I'm going to do it by hand by now but I'm willing to test your patch or git branch when you thinks it'e ready for testing.

Thanks.-

RobW’s picture

Subscribing.

effulgentsia’s picture

Status: Active » Needs review
FileSize
13.22 KB

Here's an evolution of what Jacob had in the ctools_file_entity_1141082 branch. I did not commit any of this to that branch. I think it's small enough to be reviewable as a patch instead. This is a patch against 7.x-1.x, so represents the complete work on this issue: Jacob's plus my refinements.

This removes the field redundancy mentioned in #4, adds some code comments, and cleans up a few other things.

@JacobSingh: It also removes the export_ui plugin. I don't think file entity module needs to provide an export ui. We don't need to support all the clone and add features, since these config objects are finitely determined as the union of file type / view mode / formatter. Instead we can manage saving configuration entirely via the existing "manage file displays" ui (unchanged by this patch). Let's leave it to other modules to provide any additional fanciness with respect to importing and exporting. CTools comes with a bulk export module, and Features or custom modules can handle the rest of the use-cases.

I think this is ready to fly. @neurojavi: want to take it for a spin?

effulgentsia’s picture

I think as a next step, we should take media_file_type_media_default_view() and change it to be an alter since we can supply the defaults with ctools.

If you still think this, let's take it up in another issue. All that remains in the function now (after above patch) is just making sure something visible is returned, even if no other display configuration grabbed it. Yeah, we can move it to a hook_file_displays_alter() implementation, but I'm not clear on why that's better. But if you have something in mind for why that would be better, let's discuss it in a new issue.

JacobSingh’s picture

w/o the export UI, how would someone know they had overriden a default or revert?

effulgentsia’s picture

IMO, that's something that can be added later to the "manage file displays" UI (for example, "revert" links next to the top set of checkboxes). But I don't think that needs to hold this issue up. I think what's here now is enough to clear the path for working on #1152972: Media Gallery incompatible with Media dev snapshots post beta4.

effulgentsia’s picture

Title: Provide an exportables format for declaring defaults via ctools » Provide a UI for identifying and reverting CTools Exportables overrides of file display configurations
Status: Needs review » Active

Since this is blocking Media Gallery work and other work, I committed #9. Setting issue to active and changing title to address #11. I don't think the UI portion is blocking other issues, so can be done whenever someone has motivation to do it.

Dave Reid’s picture

Project: D7 Media » File Entity (fieldable files)
Version: 7.x-1.x-dev » 7.x-2.x-dev

I believe this would now be File entity's responsibility.

Devin Carlson’s picture

Devin Carlson’s picture

Status: Closed (duplicate) » Active

On second though, they're related but different requests.