Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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?
Comment | File | Size | Author |
---|---|---|---|
#9 | media-exportables-1141082-9.patch | 13.22 KB | effulgentsia |
Comments
Comment #1
JacobSingh CreditAttribution: JacobSingh commentedMedia Gallery will also need this to get working again since it sets default displays for the view modes it creates.
Comment #2
JacobSingh CreditAttribution: JacobSingh commentedSo 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:
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)
Comment #3
David_Rothstein CreditAttribution: David_Rothstein commentedSubscribing. Yes, I think it sounds reasonable to use ctools exportables for this.
Comment #4
JacobSingh CreditAttribution: JacobSingh commentedOkay, 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
Comment #5
JacobSingh CreditAttribution: JacobSingh commentedAdded support for the default import / export UI.
Comment #6
effulgentsia CreditAttribution: effulgentsia commentedSubscribe. No time to look right now, but very interested when I get a chance.
Comment #7
neurojavi CreditAttribution: neurojavi commentedI'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.-
Comment #8
RobW CreditAttribution: RobW commentedSubscribing.
Comment #9
effulgentsia CreditAttribution: effulgentsia commentedHere'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?
Comment #10
effulgentsia CreditAttribution: effulgentsia commentedIf 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.
Comment #11
JacobSingh CreditAttribution: JacobSingh commentedw/o the export UI, how would someone know they had overriden a default or revert?
Comment #12
effulgentsia CreditAttribution: effulgentsia commentedIMO, 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.
Comment #13
effulgentsia CreditAttribution: effulgentsia commentedSince 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.
Comment #14
Dave ReidI believe this would now be File entity's responsibility.
Comment #15
Devin Carlson CreditAttribution: Devin Carlson commentedMarking this as a duplicate of #2118773: Add an API to enable the revert the file types configurations to the ones stored in HOOK_file_default_types..
Comment #16
Devin Carlson CreditAttribution: Devin Carlson commentedOn second though, they're related but different requests.