Closed (outdated)
Project:
Drupal core
Version:
8.7.x-dev
Component:
media system
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
24 Jul 2018 at 18:13 UTC
Updated:
12 Feb 2019 at 00:52 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
samuel.mortensonComment #3
samuel.mortensonComment #4
phenaproximaComment #5
phenaproximaI 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:
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?
Comment #6
marcoscanoI 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.
Comment #7
phenaproximaThat 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.
Comment #8
phenaproximaBlocker is in!
Comment #9
phenaproximaComment #10
phenaproximaA first sketch at implementing this, without interfaces or dedicated tests or anything. Let's see how much breaks.
Comment #11
samuel.mortenson+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
createMediaEntitywhich 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.Comment #12
phenaproximaThat, 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...
Comment #14
phenaproximaStill gonna fail, but wrote the interface anyway...
Comment #16
phenaproximaAdding more doc comments and trying to fix the test failures. Let's see how far we get.
Comment #18
phenaproximaThis oughta fix the tests.
Comment #19
phenaproximaRe-titling to better reflect the direction this issue is going.
Comment #20
phenaproximaBriefly 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.
Comment #21
seanbI can definitely see how having a dedicated service is an improvement over having all this logic specific for the upload form. Some thoughts:
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.
Comment #22
phenaproxima(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.
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.
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.
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.
Comment #23
seanbThank 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:
I think we should focus on the flow for the users first, and focus on the implementation second.
Comment #24
phenaproximaCurrently, 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.
Comment #25
seanbAfter 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?
Comment #26
wim leersSounds sensible to me at least. Tagging since that shift in approach happened there.
Comment #27
phenaproximaAgreed. 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!