Once the Media module currently doesn't ship enabled by default on the standard profile, we can't assume the state of the image styles when the module is installed.
I have reproduced two bugs related to this so far:
Steps to reproduce:
1) Go to simplytest.me and install Drupal Core 8.5.x and Devel
2) Go to /admin/config/media/image-styles and delete the "thumbnail" image style
3) Enable the Media module by going to /devel/php and executing: \Drupal::service('module_installer')->install(['media']);
4) Try to create a media image item by going to /media/add/image
You will see that the form has no image field:
And that's because the field is disabled on the form view settings:
In #2895382: Hide label, thumbnail, etc. for default display of media file and image references we improved the default entity view display of media images by providing some defaults.
However, if, before installing the Media module, the site has deleted the "medium" image style, the core.entity_view_display.media.image.default.yml config won't be installed due to the missing dependency.
As a result, the image media default view display will show something like:
Instead of the expected default, which was something like:
Agree on the best way to solve this and implement it. After discussing with seanB on IRC, some options appeared:
1) Delete the dependency on the "medium" image style, and show the original image by default (only applies to Bug 2)
2) Re-create the missing image styles in hook_install(). I'm not sure, however, if the config import would already have been attempted by then, in which case this creation here would be useless. (applies to both bugs)
3) Ship our own media-specific image styles and use them. (applies to both bugs)
4) Something else?
| Comment | File | Size | Author |
|---|---|---|---|
| #9 | error_in_simplytest.png | 48.35 KB | robpowell |
| #2 | media_form_display_img_disabled.png | 46.2 KB | marcoscano |
| #2 | media_add_missing_field.png | 26.84 KB | marcoscano |
| image_view_display_before.png | 228.39 KB | marcoscano | |
| image_view_display_after.png | 114.26 KB | marcoscano |
Comments
Comment #2
marcoscanoTurns out there is a bigger problem, if you delete the
thumbnailimage style.Updated the IS accordingly.
Comment #3
seanbThanks for testing this marcoscano. I see a couple of possible solution right now:
I think option 2 might be the best way, but if anyone has other ideas they are more than welcome.
Comment #4
marcoscanoI'm also in favor of just recreating the missing image styles. To make it more evident, we could show a message on the UI letting the user know this was done. However, I'm not sure if these messages during module install could cause issues with people installing from the command line, or as part of a deployment / build script.
Comment #5
chr.fritschIf we want to go with option 2 we could implement hook_module_preinstall in the standard profile.
Comment #6
phenaproximaI would prefer that we don't recreate the image styles. I can imagine this having unexpected repercussions during config sync, in addition to being mysterious to users: if you installed something other than Standard, something with doesn't have the correct image styles, you expect them not to be there. Media recreating them will look like a bug, and potentially frustrate site builders.
IMHO, we should simply fall back to the original image, without any style applied, if the desired style does not exist.
Comment #7
seanbFor the missing medium image style that could be an acceptable alternative. For the missing thumbnail style that is a bigger issue. If you install media and the media browser (the last patch isn't finished, but still), you would see a modal showing all the original images.
I agree re-installing the image styles could lead to confusion, but at least everything will look nice by default. We can inform the users it happened and they can replace and delete the image styles if they want. Not doing anything and having original images messing up the layout would be a bigger problem to me.
Comment #8
marcoscanoIn Bug 1, I believe what's happening is that the image widget requires the
thumbnailimage style by default. If it's not present, the component is directly hidden in the form display. While showing the original image inside the widget would still be better than missing the field entirely, it would probably look quite bad, and I'm not sure the benefit of "not creating anything magically" outweights how this would look like. I'm really doubting between the two options, I see pros and cons in both.Maybe some screenshots with different scenarios would help take the best decision?
Comment #9
robpowellThis was as far as I could get before hitting an error:
Comment #10
xjmWe also found #2934991: Media configuration included with Standard needs to include inherited dependencies which might impact bug 2?
Comment #11
marcoscanoI was thinking a bit more about this, pondering the weirdness of "something that you deleted appearing back again" (if we re-created the styles), versus something "looking really off" (if we fallback to the original images), and maybe a good solution to both issues could be just to use our own styles? We could, for example:
1) ship a
media_bigormedia_hdimage style, big enough to fill most screens, but without the risk of showing the original 10Mb image an angry user uploaded. (This would also be useful in other places, as pointed out in #2934850-9: Media Images should be rendered at a reasonable size by default, a concern I wholeheartedly agree with).2) use that style in
core.entity_view_display.media.image.default.yml, solving Bug 23) have a
hook_post_update_NAME()or some similar mechanism to detect whether the config objects depending on thethumbnailimage style were not installed due to the image style not present. If that's the case, then we create there our ownmedia_thumbnailimage style, and create the form display with the widget using this style. This way, Bug 1 is solved, and we don't really need to either re-createthumbnailagain, or rely on the original image for the widget. I guess we could also use themedia_bigstyle here if we wanted, but IMHO I still think having a huge image for an image widget will be very confusing.Thoughts?
Comment #13
Jaesin commentedIn my opinion, it's not that big of a deal that installing the media module would recreate some missing image styles.
Comment #25
luke.stewart commentedThis is the daily target for The Bug Smash Initiative "Last Updated Core Issue Queue page"
I've just verified this on 11.3.2 following the steps above - modified (removing both thumbnail and media image style - and installing via the UI)
I've added https://www.drupal.org/project/drupal/issues/2940205 as a related issue.
It seems live from reading the above the next steps required here are to either mark this as works as designed, or decide on a solution.
Given the lack of engagement over the last 8 years I think I'd lean towards closing works as designed.
However while it might be true that
"The repercussions of recreating an image style that was already removed from the system is easily remedied if it is even noticed."
I think the problem would be understanding that this is what has caused the issue - as there is no indication via the UI that this is what has caused the issue.
If we do want to accept this behaviour perhaps it would be okay to flag the missing config on install via a warning? Then leave it up to the site builder as to how they want to handle it?
Comment #26
marcoscanoThank you for bringing this back to life :)
I am not a huge fan of messages displayed during module installs, because not all modules are installed through the UI, and in certain scenarios it can add noise or even break some CI scripts. That said, many modules do that, and informing users that there's something off is certainly better than the current status quo. So I'd be +1 for detecting the missing styles, and logging that as an error condition both in Drupal's watchdog and on a message to the user.