Media module treats default thumbnails as user content, allowing them to be lost when syncing files across environments

There's not even a patch. Needs work is misleading.

I bet the reason for copying these images over to the public files dir is that as #3060509-23: Media module treats default thumbnails as user content, allowing them to be lost when syncing files across environments → points out Drupal needs to create styled derivatives from them. Still, they are not actually content because you ain't even able to simply change them from anywhere in the UI.

It would be cool if Media would out of the box every time it requests these images simply check if the file copies already exist at the specified icon_base_uri and if not, copy them over on the fly. While at the same time you ideally also can specify an icon_source_uri where it should copy the files from. And that the icon_source_uri can be any custom or contrib module's or theme's subdirectory.

Actually it makes no sense to me you can specify an icon_base_uri at all. This could simply be hard-coded while a configurable icon source URI would be much more interesting.

Rephrase title into a fixable task

Let's kick this off. Here's a first simple implementation following the suggestion of @jonathanshaw in #3060509-28: Reprovision default media thumbnails when not found at icon_base_uri → to reprovision the icons on hook_rebuild().

Was tagged for tests which are still needed.

As patches are phased out it's encouraged to use MRs. Patches no longer auto run.

I leveraged the patch from #53, but did it simpler and direct to point.

Thanks @filipe82sp!

--- core/modules/media/media.module	2023-11-06 21:17:57.000000000 -0300
+++ core/modules/media/media.module	2023-11-06 21:22:08.000000000 -0300
@@ -528,3 +528,12 @@

+function media_rebuild(): void {
+  // Loads media install file and execute hook_install.
+  \Drupal::moduleHandler()->loadInclude('media', 'install');
+  media_install();
+}

Calling the entire install hook might be risky since there is a possibility that more functionality will be added to the install hook in the future. To ensure we only execute the code we want to execute, it is better to refactor it in a dedicated function, like is done in patch #53.

--- core/modules/media/media.module	2023-11-06 21:17:57.000000000 -0300
+++ core/modules/media/media.module	2023-11-06 2
@@ -528,3 +528,12 @@

+function media_rebuild(): void {
+  // Loads media install file and execute hook_install.

This documentation says _what_ is being done but it would be better to explain _why_ it is needed, like is done in patch #53.

I know it is my own patch, but would respectfully argue that patch #53 is safer and better documented, even though I agree your version is more compact.

@pfrenssen Well said and +1. Thank you for laying out the pros/cons so clearly. Should we create a merge request so that GitLab CI tests can run against this?

Hi @pfrenssen, thanks for your feedback!

I agree with you too! Since in my case there's no issue related to permissions, once we didn't change it, then worked satisfactory and as expected. It would be good to proceed with MR of your changes as @markdorison suggested.

carlos romero → made their first commit to this issue’s fork.

This makes sense to me! +1 on the approach.

artis.bajars → made their first commit to this issue’s fork.

Opened a merge request based on #53.