Problem/Motivation

\Drupal\media_library\Form\MediaLibraryUploadForm had a number of protected methods that can determine what media types accept a given file, and can merge upload validators for multiple media types, which is generically useful for other parts of the media ecosystem. We should move these somewhere generic as a public API.

Proposed resolution

Move the methods that link to this issue from the upload form into some generic API.

Remaining tasks

Determine where to move them, then write a patch.

User interface changes

Undecided.

API changes

Undecided.

Data model changes

None.

Comments

samuel.mortenson created an issue. See original summary.

samuel.mortenson’s picture

samuel.mortenson’s picture

Status: Active » Postponed
phenaproxima’s picture

Issue tags: +Media Initiative
phenaproxima’s picture

I propose we add a new API to Media for this: the idea of a "dynamic type resolver" which, given arbitrary input, finds the media types which can use that input. Lightning has something similar, and I can see this concept being useful for creating new media items by entering an embed code (i.e., for an oEmbed media type).

The way I imagine it, type resolvers would be services, implementing an interface like this:

<?php

interface MediaTypeResolverInterface {

  public function getAllTypesMatchingValue($value, array $media_type_ids = NULL);

  public function alterFormElement(array &$element, FormStateInterface $form_state);

}

getAllTypesMatchingValue() just loads media types, and alterFormElement() performs any needed alterations on a form element which will accept the input value used to match against the existing media types. Since these resolvers would be services, they could employ caching and other such good stuff to keep things nice, clean, and performant.

Thoughts?

marcoscano’s picture

I like the idea! Some implementation details of it pick my curiosity though... What would happen when multiple types match a given value? I assume some sort of weighting would be in place. Would we need/want to expose that mechanism on the UI, so end users could decide what types should go first in case of multiple matching? Or expose a hook where developers could override the order?

But in general, I think it is a useful feature indeed.

phenaproxima’s picture

What would happen when multiple types match a given value?

That would be up to the calling code, although I do like the idea of a hook (or actually, an event subscriber, so that weighting could be a thing) to influence the result.

phenaproxima’s picture

Title: [PP-1] Move filter methods from MediaLibraryUploadForm somewhere else » Move filter methods from MediaLibraryUploadForm somewhere else

Blocker is in!

phenaproxima’s picture

Status: Postponed » Active
phenaproxima’s picture

Status: Active » Needs review
StatusFileSize
new17.41 KB

A first sketch at implementing this, without interfaces or dedicated tests or anything. Let's see how much breaks.

samuel.mortenson’s picture

+1 to this idea - splitting this up into a service enables other upload UIs and gives us an idea of how OEmbed support could be added. We definitely need an interface here and tests.

Should the Media Library's file moving logic be moved out as well? Maybe these services could implement a method like the upload form's createMediaEntity which takes a source field value and a media type, then does the logic necessary to set the value on the type and return the media object.

phenaproxima’s picture

Should the Media Library's file moving logic be moved out as well? Maybe these services could implement a method like the upload form's createMediaEntity which takes a source field value and a media type, then does the logic necessary to set the value on the type and return the media object.

That, to me, feels like giving the type resolver too many responsibilities. I think it should really just answer the question of "what media types can I use here?", and leave it at that. I'm open to moving the file move logic into another service or API if a use case arises, though...

Status: Needs review » Needs work

The last submitted patch, 10: 2987924-10.patch, failed testing. View results

phenaproxima’s picture

Status: Needs work » Needs review
StatusFileSize
new19.31 KB
new5.67 KB

Still gonna fail, but wrote the interface anyway...

Status: Needs review » Needs work

The last submitted patch, 14: 2987924-14.patch, failed testing. View results

phenaproxima’s picture

Status: Needs work » Needs review
StatusFileSize
new21.76 KB
new6.68 KB

Adding more doc comments and trying to fix the test failures. Let's see how far we get.

Status: Needs review » Needs work

The last submitted patch, 16: 2987924-16.patch, failed testing. View results

phenaproxima’s picture

Status: Needs work » Needs review
StatusFileSize
new21.76 KB
new1.21 KB

This oughta fix the tests.

phenaproxima’s picture

Title: Move filter methods from MediaLibraryUploadForm somewhere else » Define an API for finding media types based on an arbitrary value

Re-titling to better reflect the direction this issue is going.

phenaproxima’s picture

What would happen when multiple types match a given value?

Briefly discussed with @samuel.mortenson and we agree that the API should have no opinion on this. If multiple media types match a value, that's fine; the type resolver will return them all and let the calling code figure out what to do. I think leaving that to the consumer is a smart idea; it's just too much of an x-factor for the API to try to make a decision about it.

seanb’s picture

I can definitely see how having a dedicated service is an improvement over having all this logic specific for the upload form. Some thoughts:

  • The resolver currently helps when you already have a value. One of the problems is actually that there are different types of source fields for different media sources. And if you want to support all media types in a form, you need a way to figure out which types of source fields you need and which resolver goes with that. Do you have a vision on how this could help us with that?
  • Currently the resolver alters an existing source field, wouldn't it be better if it provided the whole field for input instead?
  • What do we do when multiple resolvers match for a specific type/input? Should we maybe use plugins and a plugins manager to determine the best resolver for a case?
  • We allow media sources to provide constraints for source fields. Should we use these constrains as well to determine the best type for the input?

While thinking about it, it could be nice if you could load all resolvers based on the enabled types for you media field. The resolvers would provide the source fields making sure the source field collects the right type of input.
We could build a single form with all source fields for all types, an upload field that allows 1 or more uploads, for oEmbed we could allow a textarea to paste some youtube URLs separated by newline, etc. Or we could show a tab per resolver, we could at least use some input on the UX of this.
Since providing a field to collect the data is closely linked to actually resolving the right types when we have the input, I'm not sure if they are "just resolvers". It does a little more than that. It seems this is more like a form builder or something?

On the other hand...

Since guessing the best type based on the input is hard, complex and maybe not even doable when you have multiple image types for example, we could also solve this another way. The user must always select which type of media it wants to add first. Not sure what the place of the resolvers would be in that case? I'm trying to think of a case where the user would not know the type of media that is going to be added, but I'm pretty sure a user already knows if it wants to add a photo, logo, brochure or video before the media library is even opened. We might be making this more complicated than it should be.

phenaproxima’s picture

The resolver currently helps when you already have a value. One of the problems is actually that there are different types of source fields for different media sources.

(Emphasis mine.) This is a red herring. There is nothing at all about type resolvers, as an API, which should restrict them to a single source field type. You could have a MediaTypeResolverInterface implementation which knows how to handle both file fields and link fields, or even those two plus entity references. I think that'd be a highly unusual use case, but that's why we define an interface for these things. Let contrib deal with those crazy edge cases -- our job is to define a solid, clear, reliable API to answer the question "given a value, which media types will accept it?" for 90% of cases. We don't need to cater to exotic implementations; we need to cater to what less technical users expect.

Currently the resolver alters an existing source field, wouldn't it be better if it provided the whole field for input instead?

I think that would be transferring a lot more responsibility into the type resolver than it should have. They really should only make the modifications that they need to make in order to filter the media types correctly, and no more than that. Keeping things lightweight is paramount. Adding responsibilities adds complexity, which is what we should be aiming to avoid.

What do we do when multiple resolvers match for a specific type/input?

This sounds like a misunderstanding of the purpose of the type resolvers. You don't need code to determine which resolver to use -- you, as the developer (coder) choose which one you want to use for your specific case, and that's that. If you as the developer want to combine the functionality of multiple resolvers, you could write a "meta-resolver" to do that, which itself implements MediaTypeResolverInterface. But again -- edge case that we don't need to worry about for core.

We allow media sources to provide constraints for source fields. Should we use these constrains as well to determine the best type for the input?

As previously explained, I think that the consuming code should be solely responsible for knowing what resolver(s) it wants to use, and what kind of form elements it wants to use to collect and input value. I'm all for using constraints if they would be useful for that, but I don't see how they would be.

seanb’s picture

Thank for your extensive reply! After discussing this with phenaproxima on slack he helped me understand everything a little bit better. This also helped me get to my 2 main concerns with the resolvers:

  • The resolvers assume the user provides a value, but when you have a bunch of different source fields (eg. file, URL, entity reference), we are skipping the step where the user needs to make a choice to provide the right type of input. If a user needs to make a choice for an input type anyway, why don't we let them choose the media type so there is nothing to resolve?
  • The resolver could lead to multiple types in which case the user needs to make 2 choices: first the right type of field to provide input, and second the right media type based on the input. If the resolver does not lead to 1 specific type, how do we really make it better for the user?

I think we should focus on the flow for the users first, and focus on the implementation second.

phenaproxima’s picture

Currently, this is listed as a stable blocker for Media Library on #2834729: [META] Roadmap to stabilize Media Library. However, I agree with @seanB that it's much more important for us to ship stable user-facing features that work well than it is for us to provide an API.

So, I'm downgrading this into a "should-have" issue in the meta.

seanb’s picture

After updating the designs of the media library, we changed our approach to how new media is supposed to be added. I think this issue is no longer relevant. @phenaproxima do you think we can close this as outdated?

wim leers’s picture

Issue tags: +BarcelonaMediaSprint

Sounds sensible to me at least. Tagging BarcelonaMediaSprint since that shift in approach happened there.

phenaproxima’s picture

Status: Needs review » Closed (outdated)

Agreed. This issue is based on old assumptions about how the media library would work, and it is now a dead end since those assumptions have changed with the consent of all concerned. Fare thee well!