Problem/Motivation

The media library adds it's upload forms by media_library_media_source_info_alter to the media sources.
This makes it impossible for contrib to overwrite these definitions. I ran into this while working on #3051658: Make dropzonejs widget available to the new Media Library in core, where I want to use a Dropzone upload form for images and files.

Proposed resolution

Specify the upload forms directly in the media sources, instead of a media_library_media_source_info_alter.

CommentFileSizeAuthor
#16 3171743-16.patch5.2 KBpaulocs
#16 interdiff-13-16.txt626 bytespaulocs
#13 3171743-13.patch5.19 KBchr.fritsch
#8 3171743-5.patch1.6 KBchr.fritsch
#4 3171743-4.patch4.63 KBchr.fritsch
#2 3171743.patch4.63 KBchr.fritsch
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chr.fritsch created an issue. See original summary.

chr.fritsch’s picture

chr.fritsch
he/him
Primary language German
Location 🇩🇪🇪🇺🌍
CreditAttribution: chr.fritsch at Thunder commented
Status: Active » Needs review
FileSize
3171743.patch4.63 KB

Here is a patch.

Status: Needs review » Needs work

The last submitted patch, 2: 3171743.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

chr.fritsch’s picture

chr.fritsch
he/him
Primary language German
Location 🇩🇪🇪🇺🌍
CreditAttribution: chr.fritsch at Thunder commented
Status: Needs work » Needs review
FileSize
3171743-4.patch4.63 KB

Removing the prepending slash

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented 
+++ b/core/modules/media/src/Plugin/media/Source/File.php
@@ -17,7 +17,10 @@
  *   allowed_field_types = {"file"},
- *   default_thumbnail_filename = "generic.png"
+ *   default_thumbnail_filename = "generic.png",
+ *   forms = {
+ *     "media_library_add" = "Drupal\media_library\Form\FileUploadForm",
+ *   }
  * )
  */
 class File extends MediaSourceBase {

I'm a bit confused. we can't have a reference to media_library in media module?

It might no be pretty, but you can be later than the media_library hook with a hook_module_implements_alter() or a module weight?

chr.fritsch’s picture

chr.fritsch
he/him
Primary language German
Location 🇩🇪🇪🇺🌍
CreditAttribution: chr.fritsch at Thunder commented

I'm a bit confused. we can't have a reference to media_library in media module?

It's not nice, but it doesn't harm IMHO.

It might no be pretty, but you can be later than the media_library hook with a hook_module_implements_alter() or a module weight?

I already thought about that, but that's really bad DX and I don't like that.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented

Well, I don't think this will be accepted :)

Alternative: check in media library if the keys are already set and don't overwrite them then.

chr.fritsch’s picture

chr.fritsch
he/him
Primary language German
Location 🇩🇪🇪🇺🌍
CreditAttribution: chr.fritsch at Thunder commented
FileSize
3171743-5.patch1.6 KB

Yes, that's possible.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Issue tags: +Needs tests

I am +1 for this refactoring, and I prefer the approach in #8. There is a reason why we did this with an alter hook in core -- it's so that Media wouldn't need to have any knowledge of Media Library's internals hard-coded into its plugin definitions.

Can we get a simple test of this?

chr.fritsch’s picture

chr.fritsch
he/him
Primary language German
Location 🇩🇪🇪🇺🌍
CreditAttribution: chr.fritsch at Thunder commented

In testMediaTypeAddForm we already test that the forms are set. I think that's ok.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Status: Needs review » Reviewed & tested by the community

Fair :) Looks legit to me, then!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott as a volunteer and commented
Status: Reviewed & tested by the community » Needs work

I think we should have a kernel test that we can override them otherwise it'd be easy for this to regress.

chr.fritsch’s picture

chr.fritsch
he/him
Primary language German
Location 🇩🇪🇪🇺🌍
CreditAttribution: chr.fritsch at Thunder commented
Status: Needs work » Needs review
FileSize
3171743-13.patch5.19 KB

Added a test to ensure a different add form is used.

Status: Needs review » Needs work

The last submitted patch, 13: 3171743-13.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

chr.fritsch’s picture

chr.fritsch
he/him
Primary language German
Location 🇩🇪🇪🇺🌍
CreditAttribution: chr.fritsch at Thunder commented
Status: Needs work » Needs review

Random fail

paulocs’s picture

paulocs
Primary language Portuguese, Brazil
Location Belo Horizonte
CreditAttribution: paulocs at CI&T commented
FileSize
interdiff-13-16.txt626 bytes
3171743-16.patch5.2 KB

That's nice. Once I was trying to change the form using the media_library_media_source_info_alter() but I couldn't do it and I didn't even thought about patch #8.

I attached a new patch to fix code standard. But it looks good.

Cheers, Paulo.

volkerk’s picture

volkerk CreditAttribution: volkerk at Thunder commented
Status: Needs review » Reviewed & tested by the community

The test does work. Looks good to me.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott as a volunteer and commented
Title: Not possible to overwrite the upload forms for media library » [backport] Not possible to overwrite the upload forms for media library
Version: 9.1.x-dev » 9.0.x-dev
Issue tags: -Needs tests

Committed afba4ff and pushed to 9.1.x. Thanks!

This seems like a good thing for contrib. Also the risks seem minimal so triggering tests for 8.9.x and 9.0.x

  • alexpott committed afba4ff on 9.1.x 
    Issue #3171743 by chr.fritsch, paulocs, Berdir, phenaproxima: Not...
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott as a volunteer and commented
Version: 9.0.x-dev » 8.9.x-dev
Status: Reviewed & tested by the community » Fixed

Cherry-picked to 8.9.x and 9.0.x as a low-risk bug fix that helps contrib and custom.

  • alexpott committed 3dfa560 on 9.0.x 
    Issue #3171743 by chr.fritsch, paulocs, Berdir, phenaproxima: Not...

  • alexpott committed 3930613 on 8.9.x 
    Issue #3171743 by chr.fritsch, paulocs, Berdir, phenaproxima: Not...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.