Problem/Motivation
When #2831274: Bring Media entity module to core as Media module lands we'll have new entity type for multi-media assets and after #2831936: Add "File" MediaSource plugin we'll have support for files. At that point we'll be able to create file media entities through stand-alone entity forms. This can be useful, but it is not the primary way content creators create media.
It is far better to allow them to do this from the content creation context. This is currently the case with file fields.
Proposed resolution
Create (or extend existing one) file field widget to be able to work with entity reference fields that reference media entities. Replicate look & feel of the existing image field. Also keep support for alt/title and save them as field values on the media entity.
Current behavior of the widget (as per 11. Aug. 2017)
- On a node add form
- Select and upload two images in one step
- Dialog opens for metadata population
- Metadata is added for the first image
- Metadata is added for the second image
- Dialog closes
- Media entities are created and the node can be saved
Comment | File | Size | Author |
---|---|---|---|
#144 | interdiff-2831940-alternate-143-144.txt | 4.48 KB | samuel.mortenson |
#144 | 2831940-alternate-144.patch | 52.12 KB | samuel.mortenson |
#136 | interdiff-129-136.txt | 25.36 KB | bdimaggio |
#136 | interdiff-134-136.txt | 23.02 KB | bdimaggio |
#136 | 2831940-136.patch | 35.51 KB | bdimaggio |
Comments
Comment #2
chr.fritschWill start on top of #2831936: Add "File" MediaSource plugin
Comment #3
pguillard CreditAttribution: pguillard commentedComment #4
pguillard CreditAttribution: pguillard commentedComment #5
pguillard CreditAttribution: pguillard commentedComment #6
seanBComment #7
seanBJust finished a working version. Will clean the code up and post a patch asap!
Comment #8
slashrsm CreditAttribution: slashrsm at MD Systems GmbH commentedInitial patch based on http://cgit.drupalcode.org/media_entity/log/?h=8.x-1.x-core-widgets-rebase. Should be credited to @seanB and @phenaproxima.
Depends on #2831274: Bring Media entity module to core as Media module.
Comment #9
seanBThis also depends on #2831936: Add "File" MediaSource plugin. Besides that #2836381: Seven's entity-add-list template omits link attributes is a blocker for this patch.
Comment #10
seanBHere is a new patch based on the last version of media. Also fixed some bugs and added a lot of documentation etc. It would be nice to get some feedback on the general approach.
Basically everything should work but it's still missing tests.
Comment #12
phenaproximaAs per the roadmap (#2825215: Media initiative: Roadmap), this is postponed on #2831936: Add "File" MediaSource plugin.
Comment #13
jonathanshawShould this be postponed on #2113931: File Field design update: Upload field. which has made great strides in overhauling the file field UI and is very close to being ready?
Comment #14
phenaproximaFor now, let's not postpone on that. And here's why...
Since this issue is one of the main blockers to getting Media stable in core (which must be done by 8.4.0-alpha1 to avoid trouble), I'd rather postpone on #2113931: File Field design update: Upload field. only if it's RTBC by the time we're ready to get this one in. I say that because UX/design issues tend to take forever to resolve, and Media has a deadline to meet. So we cannot afford to wait for any and all bikeshedding and tinkering in #2113931: File Field design update: Upload field. to die down.
Either way, I believe there is a follow-up issue to take advantage of the changes in #2113931: File Field design update: Upload field. when it lands. So there's no real need to turn that one into a blocker. :)
Comment #15
chr.fritschRerolled the patch on top of #2831936: Add "File" MediaSource plugin but currently it's not working. I think it would be best if @seanB could take a look.
Comment #16
phenaproximaI just quickly scanned the patch and realized that it is completely without tests. Uh-oh.
Comment #17
seanBBefore we can continue we need feedback on 2 specific issue from a UX perspective:
Our current approach was to open a modal to select a media type (same as you would see on media/add). When there is only 1 type available, you skip this step and go directly to the media add form. In the modal you can add any required fields for the media item. After saving the media item the modal closes and the file is added to the parent form.
This solves the issues but is not consistent with the way core works at the moment (since files don't have a type and are not fieldable). The big question is: What do we do?
Comment #18
yoroy CreditAttribution: yoroy at Roy Scholten commented@seanB Can you describe a scenario around both questions?
I think the first one means: I drop an image (jpg or png or whatever) on a file field and then have to make a choice whether to store this as a "product image" media type or a "press photo" media type.
For the second, required scenario I would think there would have to be some kind of modal to present the required fields. And yes, maybe the first scenario could be handled through a modal as well.
Any change of a couple of screenshots to show how this is handled now?
Comment #19
yoroy CreditAttribution: yoroy at Roy Scholten commentedI think this can be unpostponed now?
Comment #20
seanBOk got it to work again. Let's review this (still work to be done, but it's good to get some feedback).
Patches included for the test:
#2831936: Add "File" MediaSource plugin
#2836381: Seven's entity-add-list template omits link attributes
Comment #21
seanBThe widget was discussed in the UX meeting. We got some useful feedback on the generic file widget. To sum it up:
Comment #22
phenaproxima#2831936: Add "File" MediaSource plugin landed, so let's reroll what we have.
Comment #23
shashikant_chauhan CreditAttribution: shashikant_chauhan as a volunteer and at Iksula commentedRerolled the patch.
Comment #24
seanBThanks for the reroll. I see some config files in the standard profile which should not be there. Did you use 2831940-20-do-not-test.patch?
There should probably be a reroll of only that patch, and another one for testing that includes #2836381: Seven's entity-add-list template omits link attributes.
Comment #25
Wim LeersLooking forward to being able to review this!
However, I was just told that #2831941: [PP-1] Re-create Image field widget on top of media entity should happen first?
Comment #26
phenaproximaCorrect. Images are the top-priority use case for media in core, so they need to come first.
Comment #27
seanBThis is not relying on image in any way, so I guess we can do this in parallel. Looking forward to a review, we had to jump through some hoops to get this working, so I'm really curious to see if the general approach is good enough. Specially the way the modal communicates with the form and how we pass data along.
Comment #28
Wim LeersFor now, this is just a very rough initial review. This is going to take quite a while to get right. This is pretty complex Render/Theme/Dialog/AJAX system stuff. We'll want reviews from the Theme and Dialog system maintainers.
This is specifying a new AJAX command.
open_url
. That's far too generic a name for something that's specific to the media file widget.Ideally we wouldn't be hardcoding this.
Does all of this _really_ need to be in a preprocess function? :( :O
We tried so hard to get rid of preprocess functions in D8, and we cut down significantly on them.
This stuff definitely should not happen in a preprocess function.
This is currently bubbling metadata (because it's not doing
->toString(TRUE)
). Is that intentional?===
This is repeating quite a bit. Inject it once, then there's no more need to repeat this.
Why do we want a custom title only when it's a modal?
This is super confusing.
When would this ever not be true?
Ok so this is interpreting settings for the
file
andimage
field types.But isn't this issue only about
file
fields?Conditionally defining additional form elements. Ideally we wouldn't need this.
Don't we have pre-existing examples of these needs? How did core do that elsewhere?
If we're doing this, then why are we doing the modal-specific redirect things earlier?
Comment #29
BerdirDidn't look at the patch yet, just some notes on the previous review:
3/4: This is how it works for all widgets right now I think. See template_preprocess_field_multiple_value_form(), this is likely more or less the same but I didn't check in depth. This is a left-over from pre 8.x when it was not possible to put form elements inside tables directly, and preprocess was the only way to do it. But yes, it should be possible now to use render element children now for the table cells so we can move all that logic into the widget.
12: This will not be necessary anymore when #2863431: Change "Save and keep un-/published" buttons for media module is fixed. Core doesn't have a lot of ajax modals yet, at least not for entity forms, I think this is all taken from media_entity in contrib but it is mostly necessary as that kind of ajax stuff is quite complicated. Maybe there is a cleaner way though, could we maybe use a separate form operation to have all that logic in a separate entity form class?
Comment #30
BerdirAh, and on 7:
If this is in the same method then we can do a $storage or $file_storage local variable but I'm against doing something like $this->fileStorage. It can be tricky to store properties of instances with injected dependencies that are not services because our dependency serialization doesn't work on it. See #2882347: Serialization of config fails for fun things that can happen.
Comment #31
seanBRerolled the patch since the file/image formatters are in. The patch for testing still contains #2836381: Seven's entity-add-list template omits link attributes. Did not address #28 yet, will look at that asap.
Here are some screens for the widget summarized in #21. I used the feedback from the UX meeting to create some proposals. Would be nice to get some feedback on these. Changing to needs review for that purpose :)
Steps 2 and 3 may be repeated when multiple files were uploaded at the same time. We continue to step 4 after all media entities for all files are saved.
Step 1
This is already as it should be I believe. We should leave this as is.
Single file
Multiple files
Step 2
In the current situation we show the same page as media/add. The proposal is to show a form element for this step, whith a little more feedback to help the user select the right type for the uploaded file.
Current
Proposal
Step 3
In the current situation we show the same form as media/add/[type] in a separate. The proposal is to load the form below the type choice element via ajax. This will make it easier to correct a wrong type choice, but also give the user the feeling that selecting a type and filling out the required fields is a single step. Another thing to change would be to remove all the fields users normally don't care about at this moment:
- The file field, since they just uploaded a file changing it here would be weird. They can always close the modal if it's wrong. This might be an issue for image fields though, since the alt/title fields are required.
- The URL alias. When adding a file/media item to a parent entity, you don't are about the detail page of the media item (for the standard use cases at least).
- Revision related fields, it is a new file/media item. Revisions are not even a thing until you start to change the media item.
- Authoring information. For standard use cases the person uploading the file is the author.
We could even remove the name field, since we could auto generate that. But I guess that's a separate issue.
Current
Proposal
Step 4
The field currently shows a message after reload. I guess we should remove this message. Big question is, what happens when you use the remove button. Does it remove the media item (I think it should)?
Single file
Multiple files
Comment #32
Wim LeersThis is blocking #2831941: [PP-1] Re-create Image field widget on top of media entity.
Comment #33
Wim Leers#29:
#30:
RE: 7: :O But … aren't we doing exactly that all over the place already? Are you saying it's problematic in all those cases, or only in this particular case? In any case, this is the first time I hear this being a problem. It's in lots of places AFAICT. For example
\Drupal\node\Access\NodeRevisionAccessCheck::__construct()
, dating back to March 2014. Can't we solve this in a generic way by makingDependencySerializationTrait
checkif ($value instanceof EntityStorageInterface)
?#31:
Comment #34
phenaproximaRe-tagging.
Comment #35
marcoscanoCrazy question:
- Are we planning to do any sort of garbage collection of media entities created through this widget but then orphaned later?
With direct files this happened with the deletion of temporary files during cron, but the same will not happen here once the files will be being used by media entities, hence permanent. Should we care at all about this?
Comment #36
seanB@marcoscano: I believe the garbage collection of files causes a range of issues. Not sure what the latest status is, but there was some talk about disabling the automatic removal of files (since you can't know for sure that it is actually unused).
I would propose to keep created media items. Since media is now a full blown content entity media items could have their own detail page and don't have to be referenced at all. Also we are going to have a library you never know for what purpose the media item was created (you could have a media editor maintaining a media library or something like that).
Comment #37
marcoscano@seanB Yeah, I'm in fact +1 for not tracking media entities at all (which could lead to a lot of complex scenarios when revisions come into play). I just wanted to point out that the most basic use case (simple [media] image field on a node, for instance) would have a different behavior from what core has right now.
Comment #38
Wim LeersRE: garbage collection: There's a whole set of critical issues wrt file deletion: #2821423: Dealing with unexpected file deletion due to incorrect file usage, #2826127: Usage of Files Referenced in Entity Reference Fields Not Tracked, #2708411: [PP-1] editor.module's editor_file_reference filter not tracking file usage correctly for translations, and so on. Garbage collection is definitely out of scope here :)
Comment #39
seanBTagging for usability review of #31...
Comment #40
seanBWorked on #28, before starting with #31. Things are broken now, but it's a start.
#28.1 Renamed, although I think this is generic/useful enough to be move to
Drupal\Core\Ajax
? Should we do that?#28.2 Leaving this for now. Any suggestions?
#28.3 Leaving this for now.
#28.5 Not intentional, changed it.
#28.6 Fixed
#28.7 Fixed
#28.8 Because that's when we know a file has already been uploaded. Changed comment to reflect this.
#28.9 Yeah, when there is only 1 media type found you want to redirect to the media item form but pass along the params. Since we want to change the way the media type is selected, this will probably go away.
#28.10 Never :), removed.
#28.11 True, removed for now we will see if we need to add it again in #2831941: [PP-1] Re-create Image field widget on top of media entity
#28.12 As mentioned in #29, this will no longer be needed when #2863431: Change "Save and keep un-/published" buttons for media module is done. Dropbuttons and ajax are not friends.
#28.13 The redirects in the controller are for the media type selection page, not for the media form. But this will change anyway based on the UX feedback we got so far.
#33
This is an interesting issue. I don't know a shorter way to explain this, so bear with me:
Imho we want the behaviour to be consistent. When you add a new entity with a media item, it is probably ok to remove it. When editing an entity though, the referenced media item might also be used somewhere else, so we can't delete it. We could add a check for this?
But then again, let's say the the media item is created in the modal and the user decides to not save the parent entity. Normally a file would get deleted since it is orphaned (even though it causes issues, this is still the case). Once you create a media item though, it is never automatically deleted.
Not sure how to tackle this. The fact that a user is creating a separate media entity is not really obvious yet.
The cases I see now:
I guess the easiest/safest way is make it obvious to the user that when a media item is created through the modal, the only way to delete it is through the media admin overview. In all cases.
Comment #41
phenaproxima+1 for this idea. Once you have created a media item, even if it's implicit, we should set a message that reads something like "The image 'foobar.jpg' has been added to your media library." Would that be enough?
Comment #42
yoroy CreditAttribution: yoroy at Roy Scholten commentedThen lets add a link to the words "media library".
Comment #43
phenaproximaI did a relatively shallow review of this. To be honest, I think it needs a substantial amount of clean-up work, or at least inline documentation, because it is extremely complex (granted, that's not its fault; it's wrapping around some pretty terrible, and equally complex, file handling APIs in core that are bogged down with ancient cruft and tech debt) and hard to understand. If we don't refactor it now, my fear is that it will become one of the more unmaintainable sections of core, and we don't need more of those.
We don't need the media_ prefix here anymore (or in the name of the JS file), as per https://www.drupal.org/node/2889470.
I defer to @Wim Leers on this, but I think we can and should move this into \Drupal\Core\Ajax. Opening a URL in a dialog seems like a useful command to have, and I can see use cases for it beyond Media.
I don't think this should be in the constructor, nor should it be a stateful property of the class. Can we just add a protected getter method for this?
I wonder if it's too late for us to adopt Lightning's technique of asking every available media type if they can handle a given piece of input, then present those media types as the options here. It would be a fairly simple API change, and would allow this controller to be a little less file-centric. Granted, our current mandate is to get support for files and images, so this is OK for now...but I think we should open a follow-up issue to change that.
I think this method should be renamed. I know I named it originally, but I was in haste :) 'propagate' is confusing as hell...not sure, off the top of my head, what a better name would be.
The check for the 'media_file' modal should be a static method of this class, so it can be reused elsewhere. MediaController::isMyModal($request) or something.
Under what circumstances will $handler NOT be an instance of MediaSourceInterface?
I don't understand what this is doing, or why. Can it be documented?
Micro-optimization: getSource() involves instantiating the plugin...just to get the plugin ID. That makes me sad :( I think it would be a little more performant to call $type->get('source') to get the plugin ID directly.
Shouldn't this entire doc block just be {@inheritdoc}, then?
Nit: I think this would make more sense as an if/else, not a switch/case.
Nit: There's an extra blank line here.
Can the media types be loaded via $this->entityTypeManager->getStorage('media_type')->loadMultiple()?
We don't need the ? TRUE : FALSE part of this expression :)
This could just be
if ($cardinality > 1) {...}
Is it too late for us to add a new method to MediaInterface which is capable of returning this (assuming we don't have one already)? If we can do it, I don't mind deferring this to a follow-up.
s/weith/with :) Also, 'id' should be capitalized.
Aren't the remove_button and file_$delta sub-elements automatically added when ManagedFile is processed? Do we need to merge all this extra stuff in there? If we do, we need to document why that's the case.
I don't think this validation callback should be modifying the input. It should simply set an error and bail out, then let the user decide which files to ice.
Can we inject the entity type manager?
Comment #44
phenaproximaCleaned up a lot of my minor complaints. I decided not to heavily refactor things without tests -- this is way too complex for that! :)
Comment #45
Bojhan CreditAttribution: Bojhan as a volunteer and commentedDoes this still need usability review?
Comment #46
Gábor HojtsyUI reviewed #31 tonight with @yoroy! The steps look good. Improvements are needed in smaller areas but we don't see that it would need to be fundamentally different.
Step 1:
Step 2:
Step 3:
Step 4:
Comment #47
yoroy CreditAttribution: yoroy at Roy Scholten commentedThanks for talking me through the proposal in real life @seanB :)
For step 4 Gabor and I discussed wether to link the file name or link to the media listing in the message. In the interest of preventing people from navigating away and losing work we opted for not linking to anything in the message.
Comment #48
BerdirSome more or less related feedback in regards to the media library message
* Agree with not adding a link, that's a similar problem as e.g. links on node previews, clicking on them results in you going away and your input is lost.
* The message should be tied to an access check of actually having access to that library/overview. Which brings me to the next point..
* The change record () says "The access media overview permission is removed." which already confused me a bit when I read that change record. The interesting part is that views.view.media.yml still uses exactly that permission? My guess is that this likely only works because admin users have any permission, including those that don't exist. Not sure how tests are passing, though?
* The media library is only optional views configuration. Integrating optional, views-based overviews in code is *really* hard. What if the module is not enabled, what if the view is disabled, what if the user has a different view or multiple views? node and user have collection routes and fallback entity list builders, which results in a fixed route name that might or not not be a view. We might want to do the same here if we want to have a message pointing to that.
* Besides, the view is AFAIK still quite far from the planned library functionality and not sure how useful it is to link to that until it is.
So, in short... lets make that message a follow-up, yes? :)
* Edit: The views/link stuff is obviously not much of a problem if we don't have a link, wasn't really thinking there.. but we still can't realy know for sure if a site actively uses/has a media library or not.
Comment #49
yoroy CreditAttribution: yoroy at Roy Scholten commented@Berdir It's not clear to me if you propose to make "show a message" or "refine the message" a follow up :) I don't think we're proposing to add words or links to a media library. Only to add the indicator that a media item has been created:
Comment #50
Wim Leers#40:
RE #28
OpenUrl
toMediaOpenUrl
. But that's not really what it does — it opens a URL in a modal. So I thinkOpenUrlInDialogCommand
with an@see \Drupal\Core\Ajax\OpenDialogCommand
probably is better & clearer.@deprecated
and@internal
for now, to send a strong signal that this is bound to change?Request
object to switch between "add" and "edit" titles. For theEditorImageDialog
, this value is actually even set from the client side.dialogTitle: widget.data.src ? editor.config.drupalImage_dialogTitleEdit : editor.config.drupalImage_dialogTitleAdd
incore/modules/ckeditor/js/plugins/drupalimage/plugin.js
. I don't think that approach is a great fit for this use case though.RE #33
That sounds reasonable!
#43: I'm sort of relieved to hear you raise similar concerns about this code. And I agree with you wholeheartedly that it's not anybody's fault. And I VEHEMENTLY agree that if we don't refactor it now, it'll become another unmaintainable part of core.
I'm wondering how we can accelerate this as much as possible. I think the best thing I could think of is: get a Form API expert involved. The current maintainers of Form API are @tim.plunkett and @effulgentsia. I suspect they'll be best able to steer all the form-related things to the most architecturally sound direction, and hence the most maintainable. I've pinged them.
Comment #51
yoroy CreditAttribution: yoroy at Roy Scholten commentedYeah, we're not going to let people actualle *delete* media items, only unlink them. Delete media items from the media listing (now)/media browser (later)
Comment #52
seanBJust had a talk with phenaproxima about this issue. I think the scope is really clear. Thanks for the UX review and input!
We decided on the following approach:
Comment #53
xjmAs per my update in #2825215-87: Media initiative: Roadmap: If it becomes absolutely necessary, we can ship 8.4.0 without the widgets, since the first goal is stability for contrib and contrib does not need the widgets. (We'd still need a widget solution for 8.5.0 and before we unhide the module again.)
Something @Gábor Hojtsy brought up verbally (but that I don't see mentioned here) is the question of whether we should add the dedicated file and image widgets at all (versus just waiting until we have widgets provided by the Media Library) since the module will be hidden through the UI for now.
I asked @seanB about this earlier and he said that he wasn't sure the file or image widgets would even necessarily be based on the library.
I also am reluctant to postpone an MVP file upload widget on Media Library, which will probably start its journey as an experimental module, and which we hope will be in 8.5.x (but who knows). The widgets are the last thing I think we absolutely need for a skilled site builder to begin using media for their sites' files and images, to avoid future data migrations and be forward-compatible. So, I still see a lot of value in adding the dedicated widgets before 8.4.0, but I did want to capture @Gábor Hojtsy's feedback.
Comment #54
phenaproximaWork-in-progress patch to begin implementing the new UX. I've been writing this tests-first, and we should continue with that (IMHO) because this is so damn complicated.
Comment #55
Gábor HojtsyYeah to clarify that I said that in 8.4.x you need to know that media is there (it is hidden) and really decide to use it. You would also need to add some ckeditor integration so content editors can use your media there and do not upload images to the "legacy" Drupal core system but instead as media. Once you are so in the know and would need to work to fix more of these issues, this issue gets you some of the way there but does not make the experience complete. So whether we draw the line before or after this issue it is a by-feel pick.
I think once/if core uses media as the standard profile solution in 8.5.x (given that media and its replacement tools for image/file fields will be unhidden and useful by then) we absolutely need this in core. I don't expect media library to be stable in 8.5.x straight away. However, in this current release (8.4.x), we don't have a non-stable place to put anything, so wherever we draw the line we are saying we can make all the thing that are in stable. The argument that the contrib media module was well tested/used is a good one to make it stable in core. Whatever we add on top is brand new. We need to be confident about the quality of these to put in stable and commit to supporting it for however long it is supported. I am not saying we cannot make something good in this short timeframe left, but given that the line looks to be picked by-feel to me, it does give us an option to keep the actually proven stable media API in core, and start with the widgets and formatters in 8.5.x. (It is true that that would give us less user feedback on these and you would still need to find the contrib module if it were in contrib, but the later is true for the ckeditor integration as well).
TLDR: we can draw the line before or after this issue IMHO, if this is good stable stuff, let's get it in, otherwise don't throw out media because of it.
Comment #56
xjmTalked about the above a bit more today with @Gábor Hojtsy, @Wim Leers, and @effulgentsia. We agreed that we do need these widgets independently of the Media Library. @Gábor Hojtsy and @Wim Leers pointed out that there may be some shared requirements with the CKEditor integration, but @effulgentsia and I agreed that we can go ahead with work on the file and image widgets from these issues, and that it is valuable to have it in 8.4.x core as a part of the stable Media API if we can complete it successfully by then.
Great to see #54 already!
Comment #57
xjmSo @Gábor Hojtsy and I figured out why he and I had different views on the longterm importance of this issue, which is that since this widget is intended to provide a direct replacement for file fields for Standard profile, the previously demoed design doesn't include any mechanism for referencing an existing media entity of that source type (not even a bare-bones one).
On the other hand, #46 seems to halfway contradict that:
Somehow I'd just assumed it would work like this:
And that something like my little "choose existing" link there would toggle it to a bland old barely-usable entity reference autocomplete widget for now, but later (in the future, when available) link a fancy media library browser widget.
I guess, if we give people the option to "detach" a reference, then they might want to get it back, too.
If something like that is explicitly out of scope, though, then we should probably document that here.
Comment #58
Gábor HojtsyI think the requirements were defined backwards from what we want core standard profile to do. Because the goal was/is to replace the core file/image handling entirely and set people on a path where they can choose to reuse media without a costly migration path later.
On that way the awkwardness of needing to name your media was identified (it is like needing to add subjects to comments, it makes sense to a data modeling person but not necessarily to a user). See #2882473: Hide the media name basefield from the entity form by default. The bare-bones entity reference autocomplete only works to some degree if there are sensible names to your media and we identified it as a UX problem that you are required to name your media. Even then you would really need to carefully name your media to be able to tell apart "Flower 1" from "Flower 3" without visually seeing anything of them.
So I believe the goal/scope of these issues was to reproduce the features available in standard profile for image and file fields so we can start using media for them. So the goal was/is to instead of cramming all the possible features, create a widget with the limited features image/file fields do now, so we can actually honestly put them to use in the standard profile. I don't personally think the UX team would sign off on a bare-bones entity reference widget to reuse images/files/video, etc. as the default shipped experience of Drupal core. And it would be really sad to postpone using media at all in standard profile until the media library is ready (8.5 months out minimum but maybe 14.5 months). Therefore the interim step between the image and file fields of now and the media library is to reproduce the same upload-once experience just using a more modern and future proof backend with media. In the meantime we can figure out a useful reuse tool (using the media library).
I believe that was the operating assumption of the media team and the UX team in the past 8 months. In graphic form:
Comment #59
xjmWell, wouldn't the name of the media created with the upload just default to the filename that was previously uploaded? For images, if you can't see an image thumbnail, yeah that's mostly not useful. But this issue is just about the basic file widget, so it'd be more like autocompleting
Drupal Major Issue Triage Flowchart_0 (1).pdf
. :P What's wrong with a link that gives you an autocomplete field where you can start typing that? It's a progressive improvement over only being able to upload things on a different screen. :POn the other hand, I guess we can make that into a separate issue. Start with an upload widget only, with no possibility of referencing existing media, because being able to upload on the same screen is a showstopper. Then have a followup to enhance the file widget to allow it to "choose existing" and just toggle the existing autocomplete field like I'd envisioned. If we want to explicitly scope it that way, then I'd suggest having a separate followup issue for something like "Allow Media file widgets to toggle the autocomplete field to select existing files". We can easily do that in a BC way, get this into 8.4.x first and then enhance 8.5.x (and Standard profile) with the toggle to the autocomplete.
Aside, on naming media files. It's kind of like block titles or field names; most people don't care about them. Should we do a similar thing where by default the name of the uploaded media is the name of your file, so you don't have to fill out the field, but you can customize it if you want by clicking a little link or a checkbox?
Comment #60
BerdirSee #2882473: Hide the media name basefield from the entity form by default, we do something like that in a client project, nothing fancy like checkbox or so, just a non-required optional text field.
Comment #61
xjmI was thinking it'd be a design behavior of the media file upload widget specifically to provide the filename as a default title. Not quite the same as #2882473: Hide the media name basefield from the entity form by default because that would (I think) be about the "Add media" page that we're avoiding with the upload widget, and as a normal site user I wouldn't expect funky tokens to be added to the beginning of my filename. (Since we're now going to have a custom form instead of a modal to the Add media page, we can make that choice for the file widget.)
Comment #62
xjmCombined with "Name it what I already named it", the autocomplete behavior really isn't that bad:
Comment #63
Wim LeersThis is exactly how I also imagined it would work. But … AFAIK (I wasn't involved in Media roadmap discussions) the intent is not to provide reusability, it's to enable sites to use Media in 8.4.0 so that sites that choose to do this don't have to go through a complex update path in 8.5.0 or later. Which is what Gábor is describing in #58.
Indeed. Selecting based on name rather than thumbnail is hard. OTOH, it'd work fine for smaller sites probably. (I know it would for my blog for example.)
+1. (Although it looks like that's not actually what's been happening in this issue. Probably because the Media field allows multiple Media Types, which means it's impossible to provide the exact same experience as core does today when multiple Media Types are allowed on a single Media Entity Reference field.)
Looking forward to the next iteration after #54!
Comment #64
Gábor HojtsySurely we can bring the autocomplete widget idea to the UX meeting and discuss it there with a wider set of people if inclusion of that with the standard profile in 8.5 is desired. I would underline that that will be the immediate experience of users jumping on after hearing "Drupal now supports media reuse!", so we need to talk about whether it is worth it to go to that interim reuse step with that in mind. Anyway, I think that would be a followup to be designed and discussed (eg. how do you get something from the autocomplete to the table without leaving the form, we need some button/AJAX interaction there; how do you identify the media result if the titles are the same for multiple items, etc).
Comment #65
xjmSo I guess we should file "WIDGETS: THE FOLLOWUP" and explore the proposed implementations there. And then keep this issue scoped to parity for the first version.
Comment #66
mtodor CreditAttribution: mtodor at Thunder commentedHere is work in progress patch. Currently it covers steps 1,2,3 from #31.
From proposed concept I've made one change -> there is no need to load only combobox for selection. I made to load fist media type on first load of file. So one click (step) less and one AJAX request less -> and it looks better in my opinion. And it's quite handy when you have multi step, cos you can drop 5 files of same type then you just change to wanted media type and keep clicking save. ;)
I have wrote a lot TODOs in patch, what should be done, etc.
In general solution is, that main form with combobox generates and adds media editing form, provided by selected media type and display it. In order to handle that properly without mixing forms, there is additional form state just for inner form.
Comment #67
mtodor CreditAttribution: mtodor at Thunder commentedUploaded new WiP patch:
Anyone can take over from here. I think next step should be to finish circle of adding media -> so defined step 4. in #31. Dialog should return list of processed media IDs and they should be displayed in widget. There are several TODOs in patch, that can help to continue with integration.
It's long way to go!
Comment #68
Wim LeersThanks for all your work! I think seanB and/or phenaproxima are taking over now?
Comment #70
Bojhan CreditAttribution: Bojhan as a volunteer and commentedLooks like this is really going to be resolved in the followup, removing tag. Please make sure the followup is appropriately prioritised.
Comment #71
phenaproximaOkay, you crazy kids. This patch implements almost all of the desired flow!! Unfortunately I largely didn't end up building on @chr.fritsch and @mtodor's excellent (and, frankly, heroic) work, which I feel bad about, but...if it works, it works. I hope there are no hard feelings.
It still needs:
Comment #72
phenaproximaOops, sorry! Please ignore the patch in #71 -- it contains a lot of code that is no longer needed under the new approach.
Comment #73
phenaproximaSorry again. Forgot to remove one other thing...
Comment #74
phenaproximaAnd, let's hide #71 too.
Comment #78
mtodor CreditAttribution: mtodor at Thunder commented@phenaproxima
I have tested patch a bit and looked at code too. It looks good, way cleaner and better than the solution I wrote.
I have found few issues, that should be solved before we start with proper code review/refactoring/tests.
Just one nice to have: after upload of files and when media is created, it would be nice to automatically preselect created media entities
Comment #79
slashrsm CreditAttribution: slashrsm at MD Systems GmbH commentedTested it a bit and it looks very nice in general. I noticed two problems with cardinality:
- Widget ignores unlimited cardinality. When that is set only one file is allowed to be uploaded. This does seem to work for multi-value with limited number of items.
- Widget crashes with limited multi-value field if user attempts to upload more files than allowed by the limit (files are most likely uploaded but then nothing happens; no dialog, no files listed in the widget, ...). Question is how to handle this situation. I don't think that it is possible to limit number of allowed files in HTML input upload element. I'd say that we allow files to be uploaded and definitely display a message to the user about the problem. The question is whether we automatically remove extra files (last K where N + K is number of uploaded files and N the cardinality limit) to meet the limit or let user to do so.
It would be great to add tests for those cases as well.
IS: Added animated gif that demos the current behavior of the widget.
Comment #80
martin107 CreditAttribution: martin107 as a volunteer commented1) Just noticed/corrected a few coding standard violations while looking the code over.
2) I have sorted out a @TODO in the MediaFileWidget I am now injecting an element_info array
containing data about the 'managed_file' 'element type' plugin as described by the plugin.manager.element_info service
3) Regarding a second todo - it was incorrect so I have just deleted it.
The 'media.type_guesser.file' service cannot be injected into the MediaFileWidget class and used in one of its static methods as static methods cannot make use of $this->xyz()
using \Drupal::service is ugly anywhere .. so if anyone can provide an alternative I am all ears.
Comment #82
martin107 CreditAttribution: martin107 as a volunteer commentedMove along, there is nothing to see here.... just fixing screwups I introduced :)
Comment #83
martin107 CreditAttribution: martin107 as a volunteer commentedComment #89
martin107 CreditAttribution: martin107 as a volunteer commentedLocally MediaFileTest returns to the error from #73
Comment #91
martin107 CreditAttribution: martin107 as a volunteer commentedJust nudging this forward a little by clearing a TODO
media.type_guesser.file is now injected into the add form
I have created a new interface FileGuesserInterface -- so that we don't have to inject a concrete class.
Comment #93
johnwebdev CreditAttribution: johnwebdev commentedShould we add a kernel test for the FileGuesser class?
Comment #94
chr.fritschComment #95
seanBSmall push forward.
Some things I've noticed:
List of TODO's mentioned before:
Comment #96
woprrr CreditAttribution: woprrr as a volunteer and at NeoLynk commentedJust small opinion during review of patch. After I will try to solve TODO's if I can help here ...
That's verry redundant can we add a static method to group the responsibility of recovery of Media authorized types for given FieldDefinition.
Like MediaType::allowesTypes(FieldDefinitionInterface $field_definition)
Here can we use directly ->isMultiple() to evaluate that directly insteadOf use getCardinality() ?
Comment #97
chr.fritschSince #2883813: Move File/Image media type into Standard once Media is stable landed,
core.entity_form_display.media.file.add_file.yml
andcore.entity_form_display.media.image.add_file.yml
should be moved into the standard profile.Comment #98
bdimaggioI have been testing this patch and was unable to install Media until (with @phenaproxima's help) I moved these config files from core/modules/media/config/install/ into optional/:
Comment #99
phenaproximaThat's consistent with Media's default configuration having been moved into Standard's optional config in 8.5.x. So yes, +1 to a reroll. Thanks, @bdimaggo!
Comment #100
bdimaggioI'm working with @phenaproxima and others on a slightly different approach to this problem. Basically, the uploaded file creates a Media entity whose required fields (minus the file-upload one, naturally) then appear inline in the parent entity's form. This way we can validate those fields on submission of the parent-entity form.
Still working some bugs out of the code, but here's a video of the behavior. Looking forward to discussing with all!
Comment #101
marcoscanoThanks for picking up this @bdimaggio!
I think the new direction makes a lot of sense, and it could simplify the implementation, respect to the modal approach.
The only doubt I have with this approach is: What will happen when a field is configured to accept multiple media types? It may not be the most common scenario, but I think we probably want to make sure our approach can be extended to cover that case as well.
Great work!
Comment #102
phenaproximaI think, for now, we are going to enforce that this widget can only be used with fields that reference a single media type. We can add support for multiple types in a follow-up.
Comment #103
seanBThis would definitely make it easier to push this forward, but we should keep in mind that this needs to be added later and take this in to account when making architectural decisions. Besides that +1 for splitting this up.
Comment #104
bdimaggioHere's the code that takes the approach I showed in that video last week. It's... rough. I look forward to you more experienced media folks' review and improvement.
To start playing with this, do this on core 8.5:
Most pressing needs & notes:
Comment #105
bdimaggioha ha just kidding, here's the code.
Comment #106
Ivan Berezhnov CreditAttribution: Ivan Berezhnov as a volunteer and at Drupal Ukraine Community for Levi9 commentedComment #107
marcoscanoThanks @bdimaggio!
I'll give it a shot today/tomorrow.
Comment #108
marcoscano@bdimaggio awesome work!
I think this is a great direction for this issue. Even if at some point we may have to accept some limitations (such as restricting the widget to only one media type, etc), I believe the "inline" approach is a very good compromise for the 80% user case.
I spent some time wrapping my head around the current approach, and before working on anything else, fixed the test so it's green. (this way at least I know when I break stuff... :)
Just for reference, this is a WIP patch, where I have:
- Added a bunch of @TODOs to things I plan to be working on next
- Fixed some very minor details directly
- Fixed the functional test for the basic scenario: Go to the node form, upload a file, check that new required fields appear, fill in those, save the node, check that the media is created and all expected info is there.
I'll be working next on fixing the TODOs and working on the points from #104.
Comment #109
marcoscanoExtended the test a little bit.
Comment #110
marcoscanoSome more progress.
- Made the media type generic (instead of always "file")
- Enforced that the widget is used only with File-sourced types, and in fields that have only one Media type as target_bundle.
- Removed the required fields' errors that appeared on the empty form as soon as it was embedded (even if this needed a pretty ugly hack. Alternative ways of solving this would be very much appreciated...)
Comment #111
marcoscanoThere's still some work to do, but maybe this starts to be reviewable :)
Things still to be done:
1) Properly handle the language inheritance, as indicated in #104
2) Decide if we want the name field (label) inside the media form. I would argue that for file and images, the filename should be OK for 95% of the people, so let's just let
Media::getName()
handle the automatic name generation.3) Try to avoid the hacks that are in the patch :)
4) Add more tests
Comment #112
phenaproxima+1 for this.
Comment #113
phenaproximaReviewed about half the patch. Here's what I came up with. I also want to say that I tip my hat and genuflect respectfully before the excellent work both @bdimaggio and @marcoscano have put into this patch.
I think we should mark this class as @internal. Nothing else should ever be using it or extending it, and it's not part of Media's API. To enforce that, I also think we should make it final.
We should add a comment explaining how the element would have come to include a 'target_id' element in its value array. Also, we are using $element['#value'] several times in this method, so IMHO we should copy that into a variable for readability.
Can we add a comment above this passage? Also, since the mids are strictly a server-side thing, I don't know if we need to actually create a hidden field; we should just be able to set the element type to 'value', which will pass it through the form state without ever rendering it on the client side.
&$entity_form should be type hinted.
I think this can be reduced to one line:
\Drupal::entityTypeManager()->getHandler('media', 'add_inline')->entityFormValidate($entity_form, $form_state)
This class is not part of Media's API, so I think it should be @internal and final.
Can we use $entity->getFieldDefinitions(), as we do later on in this method?
An alternative idea: we could simply rely on the form display to tell us which fields to show, and which fields not to show. When a new required field is added, we could automatically add it to the form display. This would provide the maximum amount of flexibility to site builders, and allow us to simplify this and the surrounding code substantially.
This is an ugly hack, but I understand why we had to do it. Rather than mess around with server-side validation logic, an alternative approach might be to remove the #required property, and then add the 'form-required' CSS class and 'required' attribute in an #after_build or #pre_render callback.
Where did we do this? Can we add an @see comment?
Where is the #translating property defined?
Can we rename $definition to $field_definition?
Nit: There's an extra blank line here that shouldn't be :)
Why is $form_mid['mids'] an array? Can there be a comment explaining the structure of this thing?
We can reduce this to
if ($entity instanceof ContentEntityInterface)
.Nit: Need a space after 'foreach'.
$form_state is an object, so it's already passed by reference. No need for the ampersand.
Seems like we could eliminate this method entirely and just call EntityFormDisplay::collectRenderDisplay() as needed.
There's no real reason for this method to be private.
MediaInlineForm is not part of Media's public API and should be marked @internal. For that reason, I think we can delete this entire interface.
Comment #114
jonathanshawExcuse my ignorance, but isn't extending an existing element a very common way of creating a new element with very slightly different behavior?
Comment #115
phenaproximaYes, it is, but this is a very specialized case with a lot of Media-specific hacks and workarounds in it (in order to flex around Form API's inherent limitations). Because of that, I don't think we should be encouraging re-use or extension of this code.
In this patch, we have essentially built a "lite" version of Inline Entity Form, built specifically to meet Media's short-term goals. The real long-term solution is to actually bring Inline Entity Form, which is far more robust and generic, into core. Then we can refactor this on top of that. But that is a longer-term project, and we cannot block Media on that. Therefore, in the meantime, the stuff in this patch definitely should not be considered part of Media's API, and IMHO that should be documented and enforced.
Comment #116
jonathanshawThanks for explanation!
Comment #117
marcoscanoThanks @phenaproxima for reviewing!
Couldn't work on all points, but fixed some of the minors indicated in #114.
1) Marked as
@internal
. About making it final, can we keep the discussion open a bit more? :) I'm not 100% convinced we should prevent developers (who supposedly know what they are doing... :P) from extending it if they really want to do so despite the@internal
?2) Pending
3) Pending
4) OK
5) OK
6) OK (same as 1)
7) OK
8) That's a great idea, but how could we do that in a reliable way? For example, with
EntityFormDisplayInterface::getComponents()
we get some elements even if they are not explicitly set on the form display configuration. For example: langcode, revision_log_message, etc. If we need to account for these "special" elements as well, wouldn't, in the end, be quicker just to check if the field is required? Open to hear other alternative approaches though!9) Pending
10) OK
11) Pending
12) OK
13) OK
14) Pending
15) OK
16) OK
17) OK
18) Pending
19) OK
20) OK (same as 1)
Comment #118
marcoscanoSome more progress.
This fixes 9 and 18 from #114, as well as #112 and the language inheritance indicated in #104.
The remaining points from #114 are mostly documentation requests (2, 3, 11 and 14), for which I'd ask @bdimaggio to chime in once he's probably more authoritative on those parts of the code than me... ;P
I removed the hack to bypass required fields validation because it's apparently possible to solve the issue it by using
#limit_validation_errors
. However, when I do so, the part of the form that interests us doesn't get properly rebuilt to the AJAX callback (so I assume previously it worked as a side effect of the form having a validation error...). I left those two lines commented out, in the hope that someone with a better idea of the form API internals can help out with this.Comment #119
marcoscanoSpent some more time on this.
I added a hack again (hopefully less ugly than the previous one) to prevent the form from rebuilding, so things work with the
#limit_validation_errors
approach. Maybe the best way to move forward at this point is to collect suggestions from people more experienced with this so these hacks are not necessary. (I have the feeling that there must be a cleaner way of doing this, but I couldn't find it myself...).At least now the
MediaInlineFileWidgetTest
is green again, and we can continue testing, gathering feedback, and possibly improving other parts of the code.Comment #120
bdimaggioHey @marcoscano, I could use this addition to MediaInlineForm, built on top of the patch in 119, in order to avoid some PHP warnings that occur when a site builder sets Image media entities' "alt" and/or "title" fields to be required. (See #2831941-18: [PP-1] Re-create Image field widget on top of media entity.) Would you mind incorporating this change?
Comment #121
bdimaggioJust want to note here the thing I mentioned over Slack this afternoon: looks like #118 broke the ability to upload more than one file. (After that initial upload, the "Add more files" button disappears because the form doesn't finish being redrawn.) I have worked on this some, but not gotten too far with it; fortunately we're due some review from @tim.plunkett early next week and I have high hopes that he'll have good suggestions for dealing with validation problems.
Comment #122
bdimaggioOK, I have:
* Added in my change from #120.
* Restored the validation approach using MediaInlineForm::disableElementChildrenValidation() that you first came up with, @marcoscano.
* Built out the tests to be sure that we're checking for the reappearance of the upload button after a file is uploaded.
I'm going to keep looking into prettier ways to keep media-entity form validation without screwing up the rebuild of the parent form, but at least we've got a working approach in place for the moment.
Comment #123
bdimaggio@phenaproxima and @marcoscano, I've finally gotten to the remaining issues from @phenaproxima's code review. (I also removed a couple of unnecessary use statements.)
Yup, both good points. Addressed in this patch.
OK, commented. Re: that hidden "mids" field, I'm afraid that we're just following suit on the FileWidget code. (You'll notice a hidden "fids" field in the HTML as well when using this to upload files.) They both need to be there, I'm pretty sure, for the case where the user is editing an existing node with existing media items associated with it, and adding some files. When the form gets submitted and redrawn, the only way to keep track of those existing media items is to have them in a hidden field. I'm guessing this owes to the file field's inherent inability to have a default value.
ha ha nowhere! This was a mistake (a holdover from some copied IEF code) and I removed it.
That's a great point, and it frustrated me too when I was getting the hang of this thing. The fact that this is a multiple-upload file fields enables a situation where the user uploads 2+ files at a time. In that situation, MediaFileWidget::value() (at least) needs to be able to accommodate several fids and then cycle through them to return an array of corresponding mids, for the media items that were created from those uploaded files. I've talked to @xjm about this point and am going to start a separate issue about documenting the painful and confusing code flow.
Comment #124
benjifisherFrom #121:
I tested this, and it still seems to be broken. Looking at dblog, I see this error:
I am not entirely sure this is related. Still investigating.
Comment #125
benjifisherIt was not just multiple-file uploads that were broken. I got the same error message as in my previous comment when uploading a single file.
Without really understanding where the problem was, I made a quick hack to get it to work. See the interdiff. There may well be a better way to fix it, but at least this makes it possible to continue testing.
I can now upload one or two images at once. I do not see how to set the associated text fields as in the video in the IS, but maybe that is just because I have not configured any.
This probably still needs more work, but I am setting the issue to NR so that the testbot can give its opinion.
Comment #126
samuel.mortensonManually testing #125, here are some notes:
but the Media entity was created without errors. Going back to edit that piece of Media shows that the "Alt text" field is missing.
Comment #127
phenaproximaThis is by design. Trying to dynamically change the bundle introduces a whole other layer of complexity to this problem. So, for the time being, we decided to scale it back and only support one media type at a time. This was discussed in a UX call a couple of months ago.
That's a known issue, and certainly something we should test.
We're trying take advantage of $form_state storage caching, or tempstore, to avoid this problem. Still researching that, though. I too would like to prevent the possibility of creating invalid, orphaned media items.
That's by design. Existing media items are simply presented with their thumbnail, and a link to their edit form. Only new uploads display the required fields.
Known issue :)
The widget is wrapping around an entity reference, so this is by design. Once you have created a media item and saved it, "removing" it simply means dereferencing. IMHO this is a messaging issue -- once you save media items created by inline uploads, you should see a message like "Foobar media item was saved in your media library". That might help clarify what's going on here. Ultimately, this is probably something we'll take to the UX team for improvement. But for now, it is behaving as intended.
No. The widget is intended to make media creation as quick as possible, so it enforces that only required fields are displayed. In theory, we could change this down the road, by using a new form display explicitly shipped with Media for this purpose.
Good question. We should definitely have a test of this. I believe that the created media item should simply take on the language of its "parent entity" (i.e., the one upon whose form we created the media item).
It intentionally has no configuration options. We should find a way to make it non-configurable, or at least show a message that explains it.
Nice catch! We will fix this.
Comment #128
samuel.mortensonFirst round of reviews of #125
This element references the field widget multiple times - elements should be useable in any form. If this is usable without the field widget, we should test it. If it's not, we should mark it as internal and make it clear that it's not usable my other modules.
This feels awkward to me but I verified that the UX is much worse without this logic.
This is unused.
I'm not sure I understand this block of code - if this was just a copy paste from the parent method, do we really need it?
So the entire
element_info
service is just used for this one line of code, to add a custom#process
- is this the only way to accomplish this? I've never seen that service used in this way before.I don't think
$value
needs to be a reference here.Is it weird that a value callback is creating entities? I would expect that sort of code to go into a submit or AJAX handler.
Because of the code in
isApplicable
, "point[ing] to multiple media types" should be impossible. I think we can remove the entiregetFileMediaTypeFromSet
method.So the code right below this also checks if
fids
is an array, do we need to loop over the$submitted_values
twice?I don't think this is necessary -
$new_values
is only appended to using$new_values[] = ...
, so the indexes will always be sequential. Just saying$submitted_values = $new_values;
should be fine.As a general note, there is a significant amount of code that "massages" form values to get the current
$mid
s - and it's split across many methods in the Field Widget and Form Element so it's really hard to track down. I'm not sure if there's a quick fix for this, but since we own the entire element it's strange to see the form values transform so much.Comment #129
bdimaggioHey all -
Just wanted to get a patch in here that addresses a bunch of these concerns; others, @phenaproxima and I are working on. To respond to your latest points, @samuel.mortenson:
After some review with Adam, we determined that there wasn't really any need for MediaManagedFile. I pulled the important functionality from its processManageFile() method into MediaFileWidget::process(), and brought validateEntityForm() into MediaFileWidget wholesale.
Yeah, neither @phenaproxima, @marcoscano, nor I liked this. Like we said in that Zoom discussion the other day, we'd love suggestions on any alternative way to validate this nested entity form. Subform states? Other...?
oops. Removed.
Yeah, not needed--a remnant from the initial push to just get something working. I've removed it.
Yeah, this was borrowed from FileWidget::formElement(). I've gotten rid of the need for it.
Yep, agreed. Passing by value now.
Yeah, that's something we're working on for the next patch.
FIxed!
Haven't fixed this yet, but I will in the next patch.
Working on this too...
Yep, agreed. Will simplify this today.
Beyond the above, and the points from #'s 125 and 126, here's what I'm focusing on in the coming day or two:
Comment #130
bdimaggioDiscussed all this with @phenaproxima just now and he suggested something really great (IMHO) that could address both your final point about massaging form values to get
$mid
s, @samuel.mortenson, as well as the goal of not saving media entities until the host form is saved. This patch has been a bit difficult because it's trying to bridge the gap between media entity-reference fields, and the ManagedFile field element (which expects to be working with file entities). Rather than try to track both fids and mids everywhere, Adam suggested just loading saved media entities and saving them to$form_state->setTemporaryValue()
when the form loads; all newly-uploaded files can get assigned to unsaved media entities, which can also go into form state storage. Only when the host form is submitted will the newly-uploaded files' media entities get actually saved to the db.Would be interested to hear any problems folks might see with that approach, but if I don't hear any I'm going to prioritize working on it.
Comment #131
samuel.mortensonI ran into the same error as was reported in #124 - if you're not running PHP 7.1+ you probably can't see it, but this fixed the problem for me.
Comment #132
samuel.mortensonI was consistently getting this warning using the latest patch:
This patch reverts a change in logic from #129 that led to this.
Comment #133
benjifisherJust looking at the interdiff from #131:
I would turn it around so that we do not have to think about the double negative (not empty):
Comment #134
samuel.mortensonThis patch modifies the existing test coverage to show that a fatal error is thrown when a select field is required. While annoying, we need to add coverage for bugs like this.
#126.10 should be tested, but Selenium doesn't support multiple concurrent uploads from what I can tell. So for now, we need to manually test this bug going forward.
Comment #136
bdimaggioHey all - Here's a WIP patch that substantially simplifies the approach, and more importantly no longer saves newly-created media entities to the database until the host form is validated and submitted. @benjifisher and @samuel.mortenson, I've tested with PHP 7.0 and 7.1, and they behave the same--that issue you turned up should be gone now. Sam, I also pulled in your testing patch from earlier today.
One key issue I'm having trouble with is temporarily storing those unsaved media entities. I would have expected to be able to use $form_state->set('var_name', $val) and $form_state->get('var_name'), but that has only been working within a given page load. When I store temporary entities on a given file upload, then hit the file-upload button again and try to get them on that ajax request, $form_state->get() isn't finding my stuff. Just to continue development, I'm using tempstore, which I know isn't good. @phenaproxima and I have spent some time on this and will continue, but it'd be great to have other folks' testing/opinions.
BTW, including an interdiff with #129 as well as with #134 in order to highlight the changed approach.
Now I'm getting back to work ensuring that the widget works with images and the other built-in media types!
Comment #137
samuel.mortenson@bdimaggio From my experience, form state is rebuilt submission-to-submission, based on the user input. I've also tried to use its storage but that's only been useful to persist data from form build to submit, not between submits. I've had good luck using visually hidden elements that store entity IDs.
I know this is temporary, but it's worth pointing out that if temp store is going to be used here, we'll need a unique key per instance of the field widget, to support multiple media widgets per form and multiple open forms (in different tabs).
If there's any way to use this form without tempstore, I would push for that.Edit: Temp store is likely necessary to store temporary Media entities, so I'm deleting my suggestion to not use it. If the key is unique and the logic looks OK, then I'm fine using it.
Comment #138
larowlanVery quick review
embedding specific knowledge of a widget here indicates that the API here needs work.
We should be asking the widgets, not embedding knowledge of them
this feels wrong - are we creating the entities as unpublished even though they're technically invalid (until someone fills in the field values)? Unpublished or not it appears we're letting our entities get in an invalid state, which is bad. Can we not use form state or similar?
what is our plan here?
Did we consider using SubFormState here, it could simplify things considerably.
Looking at the screencast, it appears as though media entity metadata is done via a dialog when files are uploaded.
What happens if the person closes the dialog (the close button is there)?
Do we end up with corrupt entities?
Comment #139
Wim LeersFirst of all: kudos to anybody who's working on this, this is really hard stuff! Thank you, and respect! 👊 (I was last involved in July, see #68, and I remember how daunting/complex this issue was from back then.)
I did not (have time to) read every comment, I only read the most recent patch. So, I'm sorry if I'm bringing up things that have been discussed before.
Which brings me to an overall observation: this is so complex that I fear almost nobody in the future will be able to understand and therefore maintain it. To a large extent that's probably due to the Form API. But clearly documenting the architecture that this patch is implementing would help enormously, and would help explain how this seems to often choose to reuse existing infrastructure, but then alter things to suit our needs here.
I'm confused to see both a new form mode and logic to limit the number of fields (for which widgets are exposed).
Either one should be sufficient?
Shouldn't this assert that
#form_mode
equalsadd_inline
?Both of these are required fields that would still show up despite the above simplification logic, because they are required.
I think this should be changed (for clarity) to something like:
+1 for @larowlan's concerns. However, this is
@internal
, so I'm okay with this for now.But then we do need a
@todo
with a follow-up issue to remove this hardcoded knowledge.This is hardcoding a call to a specific widget.
This is very frightening.
This is fairly frightening.
Woah…
This is frightening too.
I think
class MediaFileWidget
would benefit enormously from documentation in its class docblock explaining how this is reusingManagedFile
and/orFileWidget
.Comment #140
samuel.mortensonAnother thing I thought of this morning: how will the current, inline-form approach be compatible with #2834729: [META] Roadmap to stabilize Media Library?
If you look at the mockups from that issue, there are designs to upload files in the Modal (uxpin link), but I'm not sure how the latest patches could be re-used in the context of the Media Library.
Comment #141
phenaproximaThey're not. The Media Library will, I believe, supersede this patch. That's why it's @internal, and that's why it's OK if it's not perfect. It's a short-term solution until we get the media library finished :)
Comment #143
samuel.mortensonI spent the end of last week trying to debug the taxonomy and select field issues in #136, but had a really hard time tracking anything down. While this inline widget experience is closer to how the Image/File widgets work today, the current bugs, only having support for one target bundle, not being able to re-use Media, and being incompatible with the Media Library, leads me to think that we should go back to the modal approach from #95.
With that in mind, I started to work on a widget based on #95 that has a number of benefits:
/admin/content/media/upload
after applying the patch for the standalone page.The widget and bulk upload functionality in this patch still needs tests written, but I wanted to post this now and see what the Media maintainers thought. If the current inline approach is still preferred as the first step for core, I can merge this code with #2834729: [META] Roadmap to stabilize Media Library and start work on a "future media widget" there.
Comment #144
samuel.mortensonSome tweaks for testers looking at #143
Comment #145
samuel.mortensonComment #148
samuel.mortensonHere's a quick tour of #144's features: https://www.youtube.com/watch?v=l7F4FaV-HzE
Comment #149
benjifisherJust browsing through the patch (not a thorough review) I noticed this. I have not looked it up, but I am pretty sure that Drupal is supposed to support browsers that do not support
display: flex
. What happens if you lose this rule?No time for more ... supper is served.
Comment #150
samuel.mortenson@benjifisher "Everything that happens now is happening now" :-). Browser compatibility for flexbox is pretty good, but all that would happen without those rules is that the thumbnail, Media label, and remove button would be vertically aligned at the bottom of the table row, not the center. Nothing functionality-breaking.
Comment #151
phenaproximaThanks for the video, @samuel.mortenson!
Although a modal is worse UX than an inline form, I think what you've cooked up solves an awful lot of problems, and not just in the short term. That looks, to me, like a basis for a proper "do all the things" media widget. My thoughts on the matter, in no particular order:
Comment #152
samuel.mortensonWith #151 in mind - please reserve specific code review of #144 for future issues (both should be made this week). Any usability notes are super-duper welcome, as the flow in the video is pretty close to the finished product (the only thing missing is the Media Library).
Comment #153
phenaproximaI think it should be discussed with the release managers and Media Initiative members, but I'm leaning towards wontfixing this issue once we agree on the way forward.
Comment #154
webchickNot a release manager, but agreed that dramatically shifting direction of an issue 140+ comments in is generally a path to pain and tears. ;) So new issue post-sign-off sounds good.
Comment #155
seanBFirst of all @samuel.mortenson: YOU ARE A WIZARD! It looks really nice. Thank you!
Some small remarks since you asked for feedback (based on the video):
Again, wow! This is really exciting stuff.
Regarding #151, I agree with everything said there.
Comment #156
jonathanshawAgreed, but for a different reason. The upload section has a lot of distracting text (cardinality, allowed types, file size limit, etc.). Putting
"Use existing" under all that buries it a bit, and doesn't make it so visually clear that there's a choice between 2 things here. Whereas because "Use existing" doesn't have all that trailing baggage, so the choice between it and "Upload new" would be clearer if existing came before upload.
Separate point: "Maximum x files" is applicable to both upload and existing, but currently is a footnote under upload. Not a big deal.
Comment #157
phenaproxima@jonathanshaw: That is great feedback, thank you!
Comment #158
samuel.mortenson#155.1 and #156 bring up a good point about visually losing the use existing area. If we follow the mockups, these actions show up side-by-side, so we should do that: https://preview.uxpin.com/c9905d865a57bed1fc72be93a4e937f126dac889#/page...
I'm holding back other feedback responses because we should probably just copy+paste everything to a new issue, if/when it's created. :-)
Comment #159
marcoscanoI echo what others said about welcoming the new approach based in @samuel.mortenson's patch and outlined in #151.
Awesome help Sam! Thanks!
Comment #160
samuel.mortensonCreated #2938116: Allow media to be uploaded with the Media Library field widget
Comment #161
phenaproxima#2938116: Allow media to be uploaded with the Media Library field widget kicks off the plan outlined in #151.
This has been discussed amongst the members of Media Initiative, with input from @Gabor Hojtsy (product manager) and @xjm (release manager). Everyone seems to agree that #2938116: Allow media to be uploaded with the Media Library field widget is a fast, clear path forward for 8.6.x.
Therefore, I think this issue has run its course. A big thank-you goes to everyone who participated in this one, for your very hard work and/or equally valuable input.
I believe we should wontfix this issue once commit credit has been properly transferred over to #2938116: Allow media to be uploaded with the Media Library field widget (if necessary). Leaving open for now.
Comment #162
webchickDone!