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.

Comments

slashrsm created an issue. See original summary.

chr.fritsch’s picture

Assigned: Unassigned » chr.fritsch
pguillard’s picture

Title: Create document field widget on top of media entity » Create file field widget on top of media entity
pguillard’s picture

Issue summary: View changes
pguillard’s picture

Issue summary: View changes
seanB’s picture

Assigned: chr.fritsch » seanB
seanB’s picture

Just finished a working version. Will clean the code up and post a patch asap!

slashrsm’s picture

Status: Active » Needs work
FileSize
44.49 KB
seanB’s picture

FileSize
45.71 KB

Here 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.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

phenaproxima’s picture

Status: Needs work » Postponed
jonathanshaw’s picture

Should this be postponed on #2113931: File Field design update which has made great strides in overhauling the file field UI and is very close to being ready?

phenaproxima’s picture

For 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 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 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 when it lands. So there's no real need to turn that one into a blocker. :)

chr.fritsch’s picture

FileSize
47.03 KB

Rerolled 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.

phenaproxima’s picture

Issue tags: +Needs tests

I just quickly scanned the patch and realized that it is completely without tests. Uh-oh.

seanB’s picture

Before we can continue we need feedback on 2 specific issue from a UX perspective:

  • How do you choose a media type when there are multiple type to fit your file?
  • What do we do with possibly required fields on the media type?

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?

yoroy’s picture

Issue tags: +Usability, +Needs design

@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?

yoroy’s picture

Status: Postponed » Active

I think this can be unpostponed now?

seanB’s picture

Status: Active » Needs review
FileSize
71.15 KB
46.47 KB
1.37 KB

Ok 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

seanB’s picture

The widget was discussed in the UX meeting. We got some useful feedback on the generic file widget. To sum it up:

  • We currently present the user with a modal to choose a media type. After choosing a media type you get the media item form.
  • The UX team thinks a modal is a good choice. Since we are going to present the library in a modal as well, that's a good reason for people to get used to this.
  • We do need to work on removing as much options as possible from the modal, and try to merge the first step and second step. So maybe load the media item form through ajax below the form element to choose a media type.
  • The plan is to create some screens for the flow, discuss those and first get consensus on the design before proceeding with actual coding.
phenaproxima’s picture

Issue tags: +Needs reroll

#2831936: Add "File" MediaSource plugin landed, so let's reroll what we have.

shashikant_chauhan’s picture

Issue tags: -Needs reroll
FileSize
56.73 KB

Rerolled the patch.

seanB’s picture

Status: Needs review » Needs work

Thanks 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.

Wim Leers’s picture

Looking 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?

phenaproxima’s picture

However, I was just told that #2831941: [PP-1] Re-create Image field widget on top of media entity should happen first?

Correct. Images are the top-priority use case for media in core, so they need to come first.

seanB’s picture

This 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.

Wim Leers’s picture

For 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.

  1. +++ b/core/modules/media/js/media_file_widget.js
    @@ -0,0 +1,19 @@
    +  Drupal.AjaxCommands.prototype.open_url = function (ajax, response, status) {
    
    +++ b/core/modules/media/src/Ajax/OpenUrlCommand.php
    @@ -0,0 +1,40 @@
    +class OpenUrlCommand implements CommandInterface {
    

    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.

  2. +++ b/core/modules/media/js/media_file_widget.js
    @@ -0,0 +1,19 @@
    +      dialog: { width: '70%', height: 500 },
    

    Ideally we wouldn't be hardcoding this.

  3. +++ b/core/modules/media/media.field.inc
    @@ -0,0 +1,108 @@
    +function template_preprocess_media_file_widget_multiple(array &$variables) {
    

    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.

  4. +++ b/core/modules/media/media.field.inc
    @@ -0,0 +1,108 @@
    +  $variables['table'] = [
    +    '#type' => 'table',
    +    '#header' => $headers,
    +    '#rows' => $rows,
    +    '#attributes' => [
    +      'id' => $table_id,
    +    ],
    +    '#tabledrag' => [
    +      [
    +        'action' => 'order',
    +        'relationship' => 'sibling',
    +        'group' => $weight_class,
    +      ],
    +    ],
    +    '#access' => !empty($rows),
    +  ];
    +
    

    This stuff definitely should not happen in a preprocess function.

  5. +++ b/core/modules/media/src/Ajax/OpenUrlCommand.php
    @@ -0,0 +1,40 @@
    +    $this->url = $url instanceof Url ? $url->toString() : $url;
    

    This is currently bubbling metadata (because it's not doing ->toString(TRUE)). Is that intentional?

  6. +++ b/core/modules/media/src/Controller/MediaController.php
    @@ -0,0 +1,169 @@
    +    if ($this->request->get('modal') == 'media_file') {
    

    ===

  7. +++ b/core/modules/media/src/Controller/MediaController.php
    @@ -0,0 +1,169 @@
    +        $file = $this->entityTypeManager
    +          ->getStorage('file')
    ...
    +      $file = $this->entityTypeManager
    +        ->getStorage('file')
    +        ->load($fids[0]);
    

    This is repeating quite a bit. Inject it once, then there's no more need to repeat this.

  8. +++ b/core/modules/media/src/Controller/MediaController.php
    @@ -0,0 +1,169 @@
    +    // Change title for the file widget modal.
    +    if ($this->request->get('modal') == 'media_file') {
    

    Why do we want a custom title only when it's a modal?

  9. +++ b/core/modules/media/src/Controller/MediaController.php
    @@ -0,0 +1,169 @@
    +  protected function redirect($route_name, array $route_parameters = [], array $options = [], $status = 302) {
    +    $options = array_merge_recursive($options, [
    +      'query' => $this->request->query->all(),
    +    ]);
    +    $redirect = parent::redirect($route_name, $route_parameters, $options, $status);
    +
    +    // Change redirect response for the file widget modal.
    +    if ($this->request->get('modal') == 'media_file') {
    +      return (new AjaxResponse)
    +        ->addCommand(new OpenUrlCommand($redirect->getTargetUrl()));
    +    }
    +    else {
    +      return $redirect;
    +    }
    

    This is super confusing.

  10. +++ b/core/modules/media/src/Controller/MediaController.php
    @@ -0,0 +1,169 @@
    +      $handler = $type->getSource();
    +      if ($handler instanceof MediaSourceInterface) {
    

    When would this ever not be true?

  11. +++ b/core/modules/media/src/Controller/MediaController.php
    @@ -0,0 +1,169 @@
    +        $field = $handler->getSourceFieldDefinition($type);
    +        if (in_array($field->getType(), ['file', 'image'])) {
    +          return in_array($extension, explode(' ', $field->getSetting('file_extensions')));
    +        }
    

    Ok so this is interpreting settings for the file and image field types.

    But isn't this issue only about file fields?

  12. +++ b/core/modules/media/src/MediaForm.php
    @@ -60,6 +65,15 @@ public function form(array $form, FormStateInterface $form_state) {
    +    // Add extra submit handler for ajax requests.
    +    if ($request->query->get('modal') == 'media_file') {
    +      $element['submit']['#submit'][] = '::fileWidgetAfterSave';
    +      $element['submit']['#ajax'] = [
    +        'callback' => '::fileWidgetAjaxSubmit',
    +      ];
    +    }
    
    @@ -105,6 +119,12 @@ protected function actions(array $form, FormStateInterface $form_state) {
    +    // Ajax doesn't work for dropbuttons in modal.
    +    if ($request->query->get('modal') == 'media_file') {
    +      unset($element['publish']['#dropbutton']);
    +      unset($element['unpublish']['#dropbutton']);
    +    }
    

    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?

  13. +++ b/core/modules/media/src/MediaForm.php
    @@ -154,4 +174,102 @@ public function save(array $form, FormStateInterface $form_state) {
    +    $form_state->disableRedirect();
    

    If we're doing this, then why are we doing the modal-specific redirect things earlier?

Berdir’s picture

Didn'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?

Berdir’s picture

Ah, 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.

seanB’s picture

Rerolled 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 :)

  • Step 1: Widget before upload, ready to select a file/files.
  • Step 2: File(s) selected and uploaded, multiple media types available (optional, of only 1 type is available this step is skipped)
  • Step 3: Media add form
  • Step 4: Media entity is saved and added to the parent form

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
Step 1 single file

Multiple files
Step 1 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
Step 2 type selection current

Proposal
Step 2 type selection 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
Step 3 add form current

Proposal
Step 3 add form 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
Step 4 single file

Multiple files
Step 4 multiple files

Wim Leers’s picture

Wim Leers’s picture

#29:

  • RE 3/4: Thanks for pointing that out. Also: :(.
  • RE 12: AJAX stuff is indeed very complicated. And it's fine for contrib or custom code. I'm just worried about the complexity we're bringing into core. Because we must maintain it, support it, and ensure it's secure. And most scarily of all: we must make sure that other modules can extend/alter this, and we should avoid breaking those while we maintain it. Just being careful! I like the sound of a separate form operation and then having all this logic in a separate entity form class!

#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 making DependencySerializationTrait check if ($value instanceof EntityStorageInterface)?

#31:

  • I think you mean file/image MediaSource plugins, not formatters :)
  • Got #2836381: Seven's entity-add-list template omits link attributes going again.
  • Step 2: I think it should use radios, not a dropdown. So much screen real estate in a dialog. Let's use it.
  • Step 3: oh, this is AJAX-updating compared to step 2, and so it does use more than the screen real estate than step 2 would make you believe. Perhaps we want a column-based UI, where step 1 is the first column (as many radios as necessary below each other), and step 2 is the second column. The second column can then still list 1-N fields to fill out.
  • Step 4: in my limited pre-existing Media knowledge, I am wondering Why wouldn't clicking "Remove" remove the media item?
phenaproxima’s picture

Issue tags: -D8Media +Media Initiative

Re-tagging.

marcoscano’s picture

Crazy 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?

seanB’s picture

@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).

marcoscano’s picture

@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.

Wim Leers’s picture

seanB’s picture

Issue tags: +Needs usability review

Tagging for usability review of #31...

seanB’s picture

Worked 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

Step 4: in my limited pre-existing Media knowledge, I am wondering Why wouldn't clicking "Remove" remove the media item?

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:

  1. A user adds a media item, does not remove it, and saves the parent entity: This case the media item should be permanent an can be used elsewhere.
  2. A user adds a media item and presses remove before saving the parent entity: We could remove the media item, since we are sure it is not used yet.
  3. A user adds a media item, does not remove it, but doesn't save the parent entity (browser closed or whatever): I guess this is a reason to remove the media entity, but I'm not sure how we could figure out this happened? Knowing all the issues with automatically deleting files, I don't think we want to go down this road again.

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.

phenaproxima’s picture

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.

+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?

yoroy’s picture

Then lets add a link to the words "media library".

phenaproxima’s picture

Status: Needs review » Needs work

I 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.

  1. +++ b/core/modules/media/media.libraries.yml
    @@ -11,3 +11,10 @@ type_form:
    +media_file_widget:
    

    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.

  2. +++ b/core/modules/media/src/Ajax/MediaOpenUrlCommand.php
    @@ -0,0 +1,40 @@
    +/**
    + * AJAX command to open a URL in a modal dialog.
    + */
    +class MediaOpenUrlCommand implements CommandInterface {
    

    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.

  3. +++ b/core/modules/media/src/Controller/MediaController.php
    @@ -0,0 +1,181 @@
    +    $fids = $request->get('fids');
    +    if ($fids) {
    +      $this->file = $this->entityTypeManager
    +        ->getStorage('file')
    +        ->load($fids[0]);
    +    }
    

    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?

  4. +++ b/core/modules/media/src/Controller/MediaController.php
    @@ -0,0 +1,181 @@
    +      if ($this->file) {
    +        // Only allow types whose source field supports the file.
    +        $types = array_intersect_key($types, $this->getSupportedTypesForFile($this->file, array_keys($types)));
    +      }
    

    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.

  5. +++ b/core/modules/media/src/Controller/MediaController.php
    @@ -0,0 +1,181 @@
    +        $types = $this->propagate($types);
    

    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.

  6. +++ b/core/modules/media/src/Controller/MediaController.php
    @@ -0,0 +1,181 @@
    +    if ($this->request->get('modal') === 'media_file' && $this->file) {
    

    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.

  7. +++ b/core/modules/media/src/MediaForm.php
    @@ -154,4 +174,102 @@ public function save(array $form, FormStateInterface $form_state) {
    +      if ($handler instanceof MediaSourceInterface) {
    

    Under what circumstances will $handler NOT be an instance of MediaSourceInterface?

  8. +++ b/core/modules/media/src/Plugin/Field/FieldWidget/MediaFileWidget.php
    @@ -0,0 +1,765 @@
    +      $media_handlers = [];
    +      $definitions = \Drupal::service('plugin.manager.field.widget')->getDefinitions();
    +      foreach ($definitions as $definition) {
    +        if ($definition['class'] == static::class) {
    +          $media_handlers = $definition['media_handlers'];
    +        }
    +      }
    

    I don't understand what this is doing, or why. Can it be documented?

  9. +++ b/core/modules/media/src/Plugin/Field/FieldWidget/MediaFileWidget.php
    @@ -0,0 +1,765 @@
    +        if (in_array($type->getSource()->getPluginId(), $media_handlers)) {
    

    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.

  10. +++ b/core/modules/media/src/Plugin/Field/FieldWidget/MediaFileWidget.php
    @@ -0,0 +1,765 @@
    +   * Override form method.
    

    Shouldn't this entire doc block just be {@inheritdoc}, then?

  11. +++ b/core/modules/media/src/Plugin/Field/FieldWidget/MediaFileWidget.php
    @@ -0,0 +1,765 @@
    +    switch ($cardinality) {
    +      case FieldStorageDefinitionInterface::CARDINALITY_UNLIMITED:
    +        $max = count($items);
    +        $is_multiple = TRUE;
    +        break;
    +
    +      default:
    +        $max = $cardinality - 1;
    +        $is_multiple = ($cardinality > 1);
    +        break;
    +    }
    

    Nit: I think this would make more sense as an if/else, not a switch/case.

  12. +++ b/core/modules/media/src/Plugin/Field/FieldWidget/MediaFileWidget.php
    @@ -0,0 +1,765 @@
    +    if ($empty_single_allowed || $empty_multiple_allowed) {
    +
    +      // Fetch the upload validators from the source field.
    

    Nit: There's an extra blank line here.

  13. +++ b/core/modules/media/src/Plugin/Field/FieldWidget/MediaFileWidget.php
    @@ -0,0 +1,765 @@
    +      $allowed_types = MediaType::loadMultiple($this->fieldDefinition->getSetting('handler_settings')['target_bundles']);
    

    Can the media types be loaded via $this->entityTypeManager->getStorage('media_type')->loadMultiple()?

  14. +++ b/core/modules/media/src/Plugin/Field/FieldWidget/MediaFileWidget.php
    @@ -0,0 +1,765 @@
    +        '#multiple' => $cardinality != 1 ? TRUE : FALSE,
    

    We don't need the ? TRUE : FALSE part of this expression :)

  15. +++ b/core/modules/media/src/Plugin/Field/FieldWidget/MediaFileWidget.php
    @@ -0,0 +1,765 @@
    +      if ($cardinality != 1 && $cardinality != -1) {
    

    This could just be if ($cardinality > 1) {...}

  16. +++ b/core/modules/media/src/Plugin/Field/FieldWidget/MediaFileWidget.php
    @@ -0,0 +1,765 @@
    +      $source_field = $media->getSource()->getSourceFieldDefinition($media->bundle->entity);
    

    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.

  17. +++ b/core/modules/media/src/Plugin/Field/FieldWidget/MediaFileWidget.php
    @@ -0,0 +1,765 @@
    +      // We use a hidden field weith the media id and show a managed file
    

    s/weith/with :) Also, 'id' should be capitalized.

  18. +++ b/core/modules/media/src/Plugin/Field/FieldWidget/MediaFileWidget.php
    @@ -0,0 +1,765 @@
    +        'file_field' => [
    +          '#type' => 'managed_file',
    +          '#value' => [
    +            'fids' => [$file->id()],
    +          ],
    +          '#process' => [],
    +          'remove_button' => [
    

    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.

  19. +++ b/core/modules/media/src/Plugin/Field/FieldWidget/MediaFileWidget.php
    @@ -0,0 +1,765 @@
    +      $message = t('Field %field can only hold @max values but there were @count uploaded. The following files have been omitted as a result: %list.', $args);
    

    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.

  20. +++ b/core/modules/media/src/Plugin/Field/FieldWidget/MediaFileWidget.php
    @@ -0,0 +1,765 @@
    +      $media = \Drupal::entityTypeManager()->getStorage('media')->loadMultiple($ids);
    

    Can we inject the entity type manager?

phenaproxima’s picture

Cleaned up a lot of my minor complaints. I decided not to heavily refactor things without tests -- this is way too complex for that! :)

Bojhan’s picture

Does this still need usability review?

Gábor Hojtsy’s picture

UI 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:

  • We think you may have labeled your media fields with single and multiple file. The single file vs. one file only text did look odd though this way as it was repeating the same thing. This should not be an issue if the field is labeled "Media".
  • Multiple file lacks size limit and allowed types info, which could be confusing. It has one file upload field at all times, so should not be a big distraction to add, but it would give value to users.

Step 2:

  • Select label should say select media type, not file type, unless somehow some magic figured out a subset of media is named files, which we don't think is possible :)

Step 3:

  • Maybe remove the Remove button for the image case -- that would help with removing the option to screw up the file type/media type after the fact (eg. uploading a PPT after you picked the image media type for a GIF). Not required for the file question though?
  • For Name, do we keep the value entered if you pick another media type? (There are no other base fields that are required. We assume keeping this info for "the same" configured fields or fields of fields (eg. image alt text) are not possible to keep?)
  • We assume the submit buttons would be simplified with #2863431: Change "Save and keep un-/published" buttons for media module, wondering if it makes sense to expose the possibility to post unpublished media here? What happens with unpublished media referenced by published content and vice versa. This sounds like a huge can of worms here...
  • When adding multiple files, we should have some labeling "Item 3 of 4" in the heading and/or rename the submit button to "Next" in the interim one and "Save" on the last one or somesuch.

Step 4:

  • @yoroy says we should show the message :)
  • Where it says "File ... has been created" is "File" coming from the media type name? If not, should it be "Media ...".
  • Deleting the media should not be possible from here as we are introducing media for reuse and we think we want to train people to that expactation. So the default behavior of "Remove" as in "Detach"/"Unlink"/"Remove reference" sounds fine. While it is not possible to re-attach a media once it has been detached (using core UIs only), it feels like the right choice.
yoroy’s picture

Thanks 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.

Berdir’s picture

Some 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.

yoroy’s picture

@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:

Media file text-1.txt has been created.
The media files have been created.
Wim Leers’s picture

#40:
RE #28

  1. I agree it's maybe generically useful enough… but until there's >1 use for this, it's better to keep it inside Media. You renamed it from OpenUrl to MediaOpenUrl. But that's not really what it does — it opens a URL in a modal. So I think OpenUrlInDialogCommand with an @see \Drupal\Core\Ajax\OpenDialogCommand probably is better & clearer.
  2. Talk to the Settings Tray people! They've had similar challenges/needs AFAIK :)
  3. Ok, but we'll need to address it at some point. Let's mark this as @deprecated and @internal for now, to send a strong signal that this is bound to change?
  4. /
  5. 👍
  6. 👍
  7. 👍
  8. Hm… that sounds like this actually should be a form. If it's a form, then you can rely on form state rather than the Request object to switch between "add" and "edit" titles. For the EditorImageDialog, this value is actually even set from the client side. dialogTitle: widget.data.src ? editor.config.drupalImage_dialogTitleEdit : editor.config.drupalImage_dialogTitleAdd in core/modules/ckeditor/js/plugins/drupalimage/plugin.js. I don't think that approach is a great fit for this use case though.
  9. Ok.
  10. :)
  11. Ok!
  12. Alright. Should we block this on #2863431: Change "Save and keep un-/published" buttons for media module then? (i.e. block commit on that, but keep going in the mean time)
  13. Ok.

RE #33

  • How does this work in the Media contrib module?
  • The 3 cases you describe: this is going to be mighty confusing I think — I suspect we'll want to run this by the UX team.
  • 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.

    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.

yoroy’s picture

Yeah, 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)

seanB’s picture

Just 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:

  • The current patch has a lot of issues, we agreed the best thing is to start fresh and just copy what we need for the next version.
  • We want to create a custom form to handle the modal. This removes the need for the custom MediaController, the seven patch and it's nasty random fails, messing with the MediaForm etc. Might even remove the need for the custom Ajax command.
  • We want to use a custom form mode for the media form in the modal, we could disable a bunch of fields that should not be needed by default, and still allow sitebuilders to change that.
  • When adding multiple files the modal form should be a multi-step form to handle the next/next/finish situation.
  • Since this is a pretty complex thing, we start with the tests first to make sure we don't break anything.
xjm’s picture

As per my update in #2825215-87: Media initiative: Essentials - first round of improvements in core: 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.

phenaproxima’s picture

FileSize
14.26 KB

Work-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.

Gábor Hojtsy’s picture

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.

Yeah 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.

xjm’s picture

Talked 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!

xjm’s picture

Issue summary: View changes
FileSize
29.62 KB

So @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:

Deleting the media should not be possible from here as we are introducing media for reuse and we think we want to train people to that expactation. So the default behavior of "Remove" as in "Detach"/"Unlink"/"Remove reference" sounds fine. While it is not possible to re-attach a media once it has been detached (using core UIs only), it feels like the right choice.

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.

Gábor Hojtsy’s picture

Issue summary: View changes
FileSize
47.21 KB

I 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:

xjm’s picture

Well, 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. :P

On 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?

Berdir’s picture

Aside, on naming Media. 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?

See #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.

xjm’s picture

I 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.)

xjm’s picture

Issue summary: View changes
FileSize
38.6 KB

Combined with "Name it what I already named it", the autocomplete behavior really isn't that bad:

Wim Leers’s picture

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.

This 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.

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.

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.)

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.

+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!

Gábor Hojtsy’s picture

Surely 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).

xjm’s picture

So 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.

mtodor’s picture

FileSize
26.25 KB

Here 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.

mtodor’s picture

FileSize
35.46 KB

Uploaded new WiP patch:

  1. a bit of refactoring
  2. added handling of errors in inner form (*not complete)
  3. commented a bit modal form

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!

Wim Leers’s picture

Thanks for all your work! I think seanB and/or phenaproxima are taking over now?