Create file field widget on top of media entity

Will start on top of #2831936: Add "File" MediaSource plugin

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

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

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.

As per the roadmap (#2825215: Media initiative: Roadmap), this is postponed on #2831936: Add "File" MediaSource plugin.

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

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

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.

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

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?

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

I think this can be unpostponed now?

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

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.

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

Rerolled the patch.

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.

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?

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.

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.

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?

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?

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.

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

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

This is blocking #2831941: [PP-1] Re-create Image field widget on top of media entity.

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

Re-tagging.

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?

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

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

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

Tagging for usability review of #31...

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.

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?

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

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?

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

Does this still need usability review?

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.

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.

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.

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

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

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)

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.

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

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.

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.

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!

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.

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:

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?

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.

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

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

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!

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

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.

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.

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!

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

Looks like this is really going to be resolved in the followup, removing tag. Please make sure the followup is appropriately prioritised.

Okay, 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:

• Tests that actually test the entire workflow, including the handling of things that might go wrong.
• Better error handling during the file move operations performed in AddFileForm::prepareEntity().
• Nice, thorough reviews and sanity checks of the approach this patch takes. It really takes full advantage of the Entity Form API.
• When all uploads have their media metadata filled in, the widget needs to update with rendered versions of the created media items.
• The modal form should have the media label filled in by default, to make it as easy as possible to quickly upload a pile of files and create media items from them.
• Assorted cleanup, refactoring, and comments/documentation.
• Probably other things...but I'm too excited to enumerate what those might be.

Oops, sorry! Please ignore the patch in #71 -- it contains a lot of code that is no longer needed under the new approach.

Sorry again. Forgot to remove one other thing...

And, let's hide #71 too.

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

1. Limiting options in media type select list to only allowed media types based on file extension
2. List of existing media entities is not displayed in widget. (fe. article save, then edit the article) -> I'm not sure is that in scope of this ticket, or not.
3. Messages are not displayed properly when media is created. Only when you go to another page you can see success messages.
4. Closing of dialog should remove uploaded files from the widget. In another case, it's possible to select them what actually doesn't do anything.
5. When one file is uploaded and media is created for that file. When new file is uploaded it will pop-up dialog with two files and new media will be created for first uploaded file too.

Just one nice to have: after upload of files and when media is created, it would be nice to automatically preselect created media entities

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

Move along, there is nothing to see here.... just fixing screwups I introduced :)

Locally MediaFileTest returns to the error from #73

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

Should we add a kernel test for the FileGuesser class?

Small push forward.

• Added check to see if a file media type is added to the field
• Added description to upload fields based on file source field settings
• Fixed #multiple on file field so it's not possible to select multiple files if cardinality is 1
• Added source field validators
• Fixed fileguesser issue exception when validation fails for all files
• Limited the list of file types to actual file sources in the popup (per #78.1)
• Fixed issue with default values in massageFormValues()
• Added message above the modal form
• Removed revision information from modal form
• Added some todo's

Some things I've noticed:

• When creating manual types, fields like URL alias and authoring information are added to the file type form. We might need to add better defaults for the modal form?
• When we land the patch to auto create a name for media items, the modal doesn't make any sense for media types without custom fields and only 1 available media type. For a lot of sites this will probably be the case. I think we want a consistent flow, but I'm not sure what to do about this.

List of TODO's mentioned before:

• List of existing media entities is not displayed in widget. (fe. article save, then edit the article) -> I'm not sure is that in scope of this ticket, or not.
• Messages are not displayed properly when media is created. Only when you go to another page you can see success messages.
• Closing of dialog should remove uploaded files from the widget. In another case, it's possible to select them what actually doesn't do anything.
• When one file is uploaded and media is created for that file. When new file is uploaded it will pop-up dialog with two files and new media will be created for first uploaded file too.
• Just one nice to have: after upload of files and when media is created, it would be nice to automatically preselect created media entities
• While fixing the cardinality for the upload field you now get a nice error. This prevents the modal from opening though, so this needs to be fixed.

Just small opinion during review of patch. After I will try to solve TODO's if I can help here ...

1. +++ b/core/modules/media/src/Form/AddFileForm.php
   @@ -161,10 +176,35 @@
   +      $allowed_types = MediaType::loadMultiple($field_definitions[$field_name]->getSetting('handler_settings')['target_bundles']);
   
   +++ b/core/modules/media/src/Plugin/Field/FieldWidget/MediaFileWidget.php
   @@ -86,9 +101,24 @@
   +    $allowed_types = MediaType::loadMultiple($field_definition->getSetting('handler_settings')['target_bundles']);
   
   @@ -114,14 +144,37 @@
   +    $allowed_types = MediaType::loadMultiple($field_definition->getSetting('handler_settings')['target_bundles']);
   

   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)

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

   Here can we use directly ->isMultiple() to evaluate that directly insteadOf use getCardinality() ?

Since #2883813: Move File/Image media type into Standard once Media is stable landed, core.entity_form_display.media.file.add_file.yml and core.entity_form_display.media.image.add_file.yml should be moved into the standard profile.

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

• core.entity_form_display.media.file.add_file.yml
• core.entity_form_display.media.image.add_file.yml
• core.entity_form_mode.media.add_file.yml

That'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!

I'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!

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

The only doubt I have with this approach is: What will happen when a field is configured to accept multiple media types?

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

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

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

Here'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:

1. Install Media
2. Create a required text field on the File bundle
3. Add a media-reference field on one of the content bundles
4. On that bundle's default form mode, change that field's widget to "File"
5. Create a node of that bundle.

Most pressing needs & notes:

• Needs to be made media-bundle-independent. (It's presently hard-coded to use the File bundle.)
• I'm not certain whether it's necessary to include a form mode (media.add_inline), but that may be necessary to give tests a form display on which to set the fields which we're checking for.
• That said, the tests aren't passing yet.
• When creating the media entity on file upload, need a little code in MediaFileWidget::value() to figure out the parent entity's langcode and give this new media entity that same langcode.
• After you upload the file and the code creates the media entity, as ajax rebuilds the parent form (which now includes the child media entity's required fields), the child form thinks it's time to validate. This causes your required field to fail validation, because it has only just been added to this form and you haven't had a chance to put a value into it.

ha ha just kidding, here's the code.

Thanks @bdimaggio!

I'll give it a shot today/tomorrow.

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

Extended the test a little bit.

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

There'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

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.

+1 for this.

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

1. +++ b/core/modules/media/src/Element/MediaManagedFile.php
   @@ -0,0 +1,76 @@
   +/**
   + * @FormElement("media_managed_file")
   + */
   +class MediaManagedFile extends ManagedFile {
   

   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.

2. +++ b/core/modules/media/src/Element/MediaManagedFile.php
   @@ -0,0 +1,76 @@
   +    if (!empty($element['#value']['target_id'])) {
   

   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.

3. +++ b/core/modules/media/src/Element/MediaManagedFile.php
   @@ -0,0 +1,76 @@
   +    $mids = isset($element['#value']['mids']) ? $element['#value']['mids'] : [];
   +    $element['mids'] = [
   +      '#type' => 'hidden',
   +      '#value' => $mids,
   +    ];
   

   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.

4. +++ b/core/modules/media/src/Element/MediaManagedFile.php
   @@ -0,0 +1,76 @@
   +  public static function validateEntityForm(&$entity_form, FormStateInterface $form_state) {
   

   &$entity_form should be type hinted.

5. +++ b/core/modules/media/src/Element/MediaManagedFile.php
   @@ -0,0 +1,76 @@
   +    $form_mode = 'add_inline';
   +    $inline_form_handler = \Drupal::entityTypeManager()
   +      ->getHandler('media', $form_mode);
   +    $inline_form_handler->entityFormValidate($entity_form, $form_state);
   

   I think this can be reduced to one line:

   \Drupal::entityTypeManager()->getHandler('media', 'add_inline')->entityFormValidate($entity_form, $form_state)

6. +++ b/core/modules/media/src/Form/MediaInlineForm.php
   @@ -0,0 +1,262 @@
   +/**
   + * Media entity inline form handler.
   + */
   +class MediaInlineForm implements MediaInlineFormInterface {
   

   This class is not part of Media's API, so I think it should be @internal and final.

7. +++ b/core/modules/media/src/Form/MediaInlineForm.php
   @@ -0,0 +1,262 @@
   +    $field_definitions = $this->entityFieldManager->getFieldDefinitions('media', $entity->bundle());
   

   Can we use $entity->getFieldDefinitions(), as we do later on in this method?

8. +++ b/core/modules/media/src/Form/MediaInlineForm.php
   @@ -0,0 +1,262 @@
   +      if (!$field_definition->isRequired()) {
   +        $entity_form[$field_name]['#access'] = FALSE;
   +      }
   

   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.

9. +++ b/core/modules/media/src/Form/MediaInlineForm.php
   @@ -0,0 +1,262 @@
   +        // Elements with "#required" key set will always be validated, even if
   +        // 'limit_validation_errors' is set. Disable their validation here, we
   +        // will enforce validation happens inside the real submit handler.
   +        $entity_form[$field_name] = $this->disableElementChildrenValidation($entity_form[$field_name]);
   

   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.

10. +++ b/core/modules/media/src/Form/MediaInlineForm.php
    @@ -0,0 +1,262 @@
    +    // We've already set the just-uploaded file as the value of the source
    +    // field, so hide that too.
    

    Where did we do this? Can we add an @see comment?

11. +++ b/core/modules/media/src/Form/MediaInlineForm.php
    @@ -0,0 +1,262 @@
    +    if (!empty($entity_form['#translating'])) {
    

    Where is the #translating property defined?

12. +++ b/core/modules/media/src/Form/MediaInlineForm.php
    @@ -0,0 +1,262 @@
    +      foreach ($entity->getFieldDefinitions() as $field_name => $definition) {
    

    Can we rename $definition to $field_definition?

13. +++ b/core/modules/media/src/Form/MediaInlineForm.php
    @@ -0,0 +1,262 @@
    +    }
    +
    +
    +    return $entity_form;
    

    Nit: There's an extra blank line here that shouldn't be :)

14. +++ b/core/modules/media/src/Form/MediaInlineForm.php
    @@ -0,0 +1,262 @@
    +        if (!empty($form_mid['mids'][0])) {
    +          $mids[] = $form_mid['mids'][0];
    +        }
    

    Why is $form_mid['mids'] an array? Can there be a comment explaining the structure of this thing?

15. +++ b/core/modules/media/src/Form/MediaInlineForm.php
    @@ -0,0 +1,262 @@
    +      if ($entity && is_a($entity, '\Drupal\Core\Entity\ContentEntityInterface')) {
    

    We can reduce this to if ($entity instanceof ContentEntityInterface).

16. +++ b/core/modules/media/src/Form/MediaInlineForm.php
    @@ -0,0 +1,262 @@
    +        foreach($form_state->getErrors() as $name => $message) {
    

    Nit: Need a space after 'foreach'.

17. +++ b/core/modules/media/src/Form/MediaInlineForm.php
    @@ -0,0 +1,262 @@
    +        call_user_func_array($function, [$entity->getEntityTypeId(), $entity, &$entity_form, &$form_state]);
    

    $form_state is an object, so it's already passed by reference. No need for the ampersand.

18. +++ b/core/modules/media/src/Form/MediaInlineForm.php
    @@ -0,0 +1,262 @@
    +  /**
    +   * Gets the form display for the given entity.
    +   *
    +   * @param \Drupal\Core\Entity\ContentEntityInterface $entity
    +   *   The entity.
    +   * @param string $form_mode
    +   *   The form mode.
    +   *
    +   * @return \Drupal\Core\Entity\Display\EntityFormDisplayInterface
    +   *   The form display.
    +   */
    +  protected function getFormDisplay(ContentEntityInterface $entity, $form_mode) {
    +    return EntityFormDisplay::collectRenderDisplay($entity, $form_mode);
    +  }
    

    Seems like we could eliminate this method entirely and just call EntityFormDisplay::collectRenderDisplay() as needed.

19. +++ b/core/modules/media/src/Form/MediaInlineForm.php
    @@ -0,0 +1,262 @@
    +  private function hostFormSubmitted(FormStateInterface $form_state) {
    

    There's no real reason for this method to be private.

20. +++ b/core/modules/media/src/MediaInlineFormInterface.php
    @@ -0,0 +1,38 @@
    +/**
    + * Defines the interface for Media inline form handlers.
    + */
    +interface MediaInlineFormInterface extends EntityHandlerInterface {
    

    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.

+++ b/core/modules/media/src/Element/MediaManagedFile.php
@@ -0,0 +1,76 @@
+/**
+ * @FormElement("media_managed_file")
+ */
+class MediaManagedFile extends ManagedFile {

I think we should mark this class as @internal. Nothing else should ever be using it or extending it

Excuse my ignorance, but isn't extending an existing element a very common way of creating a new element with very slightly different behavior?

Excuse my ignorance, but isn't extending an existing element a very common way of creating a new element with very slightly different behavior?

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

Thanks for explanation!

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

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

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

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

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

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

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

2. +++ b/core/modules/media/src/Element/MediaManagedFile.php
   @@ -0,0 +1,76 @@
   +    if (!empty($element['#value']['target_id'])) {

   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.

   Yup, both good points. Addressed in this patch.

3. +++ b/core/modules/media/src/Element/MediaManagedFile.php
   @@ -0,0 +1,76 @@
   +    $mids = isset($element['#value']['mids']) ? $element['#value']['mids'] : [];
   +    $element['mids'] = [
   +      '#type' => 'hidden',
   +      '#value' => $mids,
   +    ];

   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.

   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.

11. +++ b/core/modules/media/src/Form/MediaInlineForm.php
    @@ -0,0 +1,262 @@
    +    if (!empty($entity_form['#translating'])) {

    Where is the #translating property defined?

    ha ha nowhere! This was a mistake (a holdover from some copied IEF code) and I removed it.

14. +++ b/core/modules/media/src/Form/MediaInlineForm.php
    @@ -0,0 +1,262 @@
    +        if (!empty($form_mid['mids'][0])) {
    +          $mids[] = $form_mid['mids'][0];
    +        }

    Why is $form_mid['mids'] an array? Can there be a comment explaining the structure of this thing?

    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.

From #121:

#118 broke the ability to upload more than one file.

I tested this, and it still seems to be broken. Looking at dblog, I see this error:

Error: [] operator not supported for strings in Drupal\media\Plugin\Field\FieldWidget\MediaFileWidget::value() (line 205 of /app/core/modules/media/src/Plugin/Field/FieldWidget/MediaFileWidget.php)

I am not entirely sure this is related. Still investigating.

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

Manually testing #125, here are some notes:

1. The current patch only works for fields that reference a single Media Type - that means that you can't use the File widget for a Media field that references Image _and_ File types, for instance.
2. I created a Media field that only references Images, and uploaded an image using the file widget. The required "Alt text" field was not shown,
   but the Media entity was created without errors. Going back to edit that piece of Media shows that the "Alt text" field is missing.
3. A Media entity is created as soon as a file is uploaded, even if it has required fields to fill in afterwards. This could lead to orphaned Media entities that are essentially invalid. This is a similar problem to point #2 here.
4. If a Media entity has required fields, you fill those in after you've uploaded the Media. Once the form is submitted and you re-visit the field widget, you can't edit those required fields anymore. I see this as a problem since you could never edit alt or title text once a Media entity was uploaded.
5. I added a required Taxonomy Term field to a Media entity with the auto-create setting turned on. I tried typing a term that doesn't exist into the field while using this widget, but it's not auto-creating that term. I then tried referencing an existing taxonomy term, and that also didn't work.
6. If you "Remove" a Media entity using the field widget, the Media entity isn't really deleted or put into a temporary state. This differs from the current file widget functionality where orphaned files are marked unused, and eventually deleted.
7. Would users ever expect to edit _non required_ fields with this widget? Right now only required fields can be edited, and again can only be edited on upload of the Media entity.
8. How does this widget support multilingual? If I'm editing a French node, would I expect the French representations of Media to show up in the form? A common multilingual task for Images is translating alt/title fields, even though those aren't working right now (see #2).
9. The field widget configuration form is empty for me. I can click the configure (gear) button, but nothing shows up (besides "Update"). See attachment.
10. I created a Media field with a max cardinality of three. I then uploaded four files at once - and saw an expected error, but the widget looked pretty strange. I then tried submitting the form anyway and got a 500 error: "Error: Call to a member function getSource() on null in Drupal\media\Plugin\Field\FieldWidget\MediaFileWidget::process() (line 276 of /var/www/html/docroot/core/modules/media/src/Plugin/Field/FieldWidget/MediaFileWidget.php)". See attachment.

The current patch only works for fields that reference a single Media Type - that means that you can't use the File widget for a Media field that references Image _and_ File types, for instance.

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

I created a Media field that only references Images, and uploaded an image using the file widget. The required "Alt text" field was not shown,
but the Media entity was created without errors. Going back to edit that piece of Media shows that the "Alt text" field is missing.

That's a known issue, and certainly something we should test.

A Media entity is created as soon as a file is uploaded, even if it has required fields to fill in afterwards. This could lead to orphaned Media entities that are essentially invalid. This is a similar problem to point #2 here.

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.

If a Media entity has required fields, you fill those in after you've uploaded the Media. Once the form is submitted and you re-visit the field widget, you can't edit those required fields anymore. I see this as a problem since you could never edit alt or title text once a Media entity was uploaded.

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.

I added a required Taxonomy Term field to a Media entity with the auto-create setting turned on. I tried typing a term that doesn't exist into the field while using this widget, but it's not auto-creating that term. I then tried referencing an existing taxonomy term, and that also didn't work.

Known issue :)

If you "Remove" a Media entity using the field widget, the Media entity isn't really deleted or put into a temporary state. This differs from the current file widget functionality where orphaned files are marked unused, and eventually deleted.

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.

Would users ever expect to edit _non required_ fields with this widget?

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.

How does this widget support multilingual? If I'm editing a French node, would I expect the French representations of Media to show up in the form? A common multilingual task for Images is translating alt/title fields, even though those aren't working right now (see #2).

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

The field widget configuration form is empty for me. I can click the configure (gear) button, but nothing shows up (besides "Update"). See attachment.

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.

I created a Media field with a max cardinality of three. I then uploaded four files at once - and saw an expected error, but the widget looked pretty strange. I then tried submitting the form anyway and got a 500 error: "Error: Call to a member function getSource() on null in Drupal\media\Plugin\Field\FieldWidget\MediaFileWidget::process() (line 276 of /var/www/html/docroot/core/modules/media/src/Plugin/Field/FieldWidget/MediaFileWidget.php)". See attachment.

Nice catch! We will fix this.

First round of reviews of #125

1. +++ b/core/modules/media/src/Element/MediaManagedFile.php
   @@ -0,0 +1,90 @@
   +class MediaManagedFile extends ManagedFile {
   

   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.

2. +++ b/core/modules/media/src/Form/MediaInlineForm.php
   @@ -0,0 +1,271 @@
   +        $entity_form[$field_name] = $this->disableElementChildrenValidation($entity_form[$field_name]);
   

   This feels awkward to me but I verified that the UX is much worse without this logic.

3. +++ b/core/modules/media/src/Plugin/Field/FieldWidget/MediaFileWidget.php
   @@ -0,0 +1,416 @@
   +use Drupal\file\Plugin\Field\FieldWidget;
   

   This is unused.

4. +++ b/core/modules/media/src/Plugin/Field/FieldWidget/MediaFileWidget.php
   @@ -0,0 +1,416 @@
   +    // The field settings include defaults for the field type. However, this
   +    // widget is a base class for other widgets (e.g., ImageWidget) that may act
   +    // on field types without these expected settings.
   +    $field_settings += [
   +      'display_default' => NULL,
   +      'display_field' => NULL,
   +      'description_field' => NULL,
   +    ];
   

   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?

5. +++ b/core/modules/media/src/Plugin/Field/FieldWidget/MediaFileWidget.php
   @@ -0,0 +1,416 @@
   +      '#process' => array_merge($element_info['#process'], [[get_class($this), 'process']]),
   

   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.

6. +++ b/core/modules/media/src/Plugin/Field/FieldWidget/MediaFileWidget.php
   @@ -0,0 +1,416 @@
   +    foreach ($values as &$value) {
   

   I don't think $value needs to be a reference here.

7. +++ b/core/modules/media/src/Plugin/Field/FieldWidget/MediaFileWidget.php
   @@ -0,0 +1,416 @@
   +  public static function value($element, $input, FormStateInterface $form_state) {
   

   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.

8. +++ b/core/modules/media/src/Plugin/Field/FieldWidget/MediaFileWidget.php
   @@ -0,0 +1,416 @@
   +      // If this field happens to point to multiple media types (bundles), just
   +      // use the first one that implements a file source.
   +      $media_type = !empty($handler_settings['target_bundles']) ? self::getFileMediaTypeFromSet($handler_settings['target_bundles']) : FALSE;
   

   Because of the code in isApplicable, "point[ing] to multiple media types" should be impossible. I think we can remove the entire getFileMediaTypeFromSet method.

9. +++ b/core/modules/media/src/Plugin/Field/FieldWidget/MediaFileWidget.php
   @@ -0,0 +1,416 @@
   +    $submitted_values = NestedArray::getValue($form_state->getValues(), array_slice($button['#parents'], 0, -2));
   +    foreach ($submitted_values as $delta => $submitted_value) {
   +      if (empty($submitted_value['fids'])) {
   +        unset($submitted_values[$delta]);
   +      }
   +    }
   

   So the code right below this also checks if fids is an array, do we need to loop over the $submitted_values twice?

10. +++ b/core/modules/media/src/Plugin/Field/FieldWidget/MediaFileWidget.php
    @@ -0,0 +1,416 @@
    +    // Re-index deltas after removing empty items.
    +    $submitted_values = array_values($new_values);
    

    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 $mids - 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.

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

1. +++ b/core/modules/media/src/Element/MediaManagedFile.php
   @@ -0,0 +1,90 @@
   +class MediaManagedFile extends ManagedFile {

   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.

   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.

2. +++ b/core/modules/media/src/Form/MediaInlineForm.php
   @@ -0,0 +1,271 @@
   +        $entity_form[$field_name] = $this->disableElementChildrenValidation($entity_form[$field_name]);

   This feels awkward to me but I verified that the UX is much worse without this logic.

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

3. +++ b/core/modules/media/src/Plugin/Field/FieldWidget/MediaFileWidget.php
   @@ -0,0 +1,416 @@
   +use Drupal\file\Plugin\Field\FieldWidget;

   This is unused.

   oops. Removed.

4. +++ b/core/modules/media/src/Plugin/Field/FieldWidget/MediaFileWidget.php
   @@ -0,0 +1,416 @@
   +    // The field settings include defaults for the field type. However, this
   +    // widget is a base class for other widgets (e.g., ImageWidget) that may act
   +    // on field types without these expected settings.
   +    $field_settings += [
   +      'display_default' => NULL,
   +      'display_field' => NULL,
   +      'description_field' => NULL,
   +    ];

   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?

   Yeah, not needed--a remnant from the initial push to just get something working. I've removed it.

5. +++ b/core/modules/media/src/Plugin/Field/FieldWidget/MediaFileWidget.php
   @@ -0,0 +1,416 @@
   +      '#process' => array_merge($element_info['#process'], [[get_class($this), 'process']]),

   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.

   Yeah, this was borrowed from FileWidget::formElement(). I've gotten rid of the need for it.

6. +++ b/core/modules/media/src/Plugin/Field/FieldWidget/MediaFileWidget.php
   @@ -0,0 +1,416 @@
   +    foreach ($values as &$value) {

   I don't think $value needs to be a reference here.

   Yep, agreed. Passing by value now.

7. +++ b/core/modules/media/src/Plugin/Field/FieldWidget/MediaFileWidget.php
   @@ -0,0 +1,416 @@
   +  public static function value($element, $input, FormStateInterface $form_state) {

   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.

   Yeah, that's something we're working on for the next patch.

8. +++ b/core/modules/media/src/Plugin/Field/FieldWidget/MediaFileWidget.php
   @@ -0,0 +1,416 @@
   +      // If this field happens to point to multiple media types (bundles), just
   +      // use the first one that implements a file source.
   +      $media_type = !empty($handler_settings['target_bundles']) ? self::getFileMediaTypeFromSet($handler_settings['target_bundles']) : FALSE;

   Because of the code in isApplicable, "point[ing] to multiple media types" should be impossible. I think we can remove the entire getFileMediaTypeFromSet method.

   FIxed!

9. +++ b/core/modules/media/src/Plugin/Field/FieldWidget/MediaFileWidget.php
   @@ -0,0 +1,416 @@
   +    $submitted_values = NestedArray::getValue($form_state->getValues(), array_slice($button['#parents'], 0, -2));
   +    foreach ($submitted_values as $delta => $submitted_value) {
   +      if (empty($submitted_value['fids'])) {
   +        unset($submitted_values[$delta]);
   +      }
   +    }

   So the code right below this also checks if fids is an array, do we need to loop over the $submitted_values twice?

   Haven't fixed this yet, but I will in the next patch.

10. +++ b/core/modules/media/src/Plugin/Field/FieldWidget/MediaFileWidget.php
    @@ -0,0 +1,416 @@
    +    // Re-index deltas after removing empty items.
    +    $submitted_values = array_values($new_values);

    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.

    Working on this too...

As a general note, there is a significant amount of code that "massages" form values to get the current $mids - 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.

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:

1. Ensuring that newly-uploaded media entities' fields are correctly validated. @samuel.mortenson, I'm hoping for some thoughts from you on this point, but let me know if you don't see either time to work on this, or any alternative way forward. If either of those are true, I'll continue to iterate on the current approach.
2. Avoiding saving media items until the host form is validated and submitted.
3. Ensuring that File and Image media items (at least) can be referenced from this widget.After that, I'll test and work on making it work on any media bundle.

Discussed 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 $mids, @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.

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

I was consistently getting this warning using the latest patch:

Warning: Cannot use a scalar value as an array in Drupal\media\Form\MediaInlineForm->entityForm() (line 103 of /var/www/html/docroot/core/modules/media/src/Form/MediaInlineForm.php) #0

This patch reverts a change in logic from #129 that led to this.

Just looking at the interdiff from #131:

+    $return['mids'] = !empty($return['mids']) ? $return['mids'] : [];

I would turn it around so that we do not have to think about the double negative (not empty):

+    $return['mids'] = empty($return['mids']) ? [] : $return['mids'];

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

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

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

+++ b/core/modules/media/src/Plugin/Field/FieldWidget/MediaFileWidget.php
@@ -0,0 +1,330 @@
+  public static function getParentMedia(FormStateInterface $form_state, $fid = 'all') {
+    // @todo Debug this to use $form_state storage rather than tempstore.
+    //    $media_entities = $form_state->get('media_entities');
+    $tempstore = \Drupal::service('user.private_tempstore')->get('media');
+    $media_entities = $tempstore->get('media_entities');
+    if ($fid && is_array($media_entities)) {
+      if ($fid == 'all') {
+        return $media_entities;
+      }
+      if (array_key_exists($fid, $media_entities)) {
+        return $media_entities[$fid];
+      }
+    }
+    return FALSE;

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.

Very quick review

1. +++ b/core/modules/media/src/Form/MediaInlineForm.php
   @@ -0,0 +1,233 @@
   +    // If this is an image media entity, and the image field on that entity has
   +    // either "Alt field required" or "Title field required", we take care of
   +    // that in MediaImageWidget::process().
   +    // @see \Drupal\media\Plugin\Field\FieldWidget\MediaImageWidget::process().
   +    foreach (['#alt_field_required', '#title_field_required'] as $extra_image_field_attribute) {
   +      if (isset($entity_form[$source_field]['widget'][0][$extra_image_field_attribute])) {
   +        $entity_form[$source_field]['widget'][0][$extra_image_field_attribute] = FALSE;
   +      }
   +    }
   

   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

2. +++ b/core/modules/media/src/Form/MediaInlineForm.php
   @@ -0,0 +1,233 @@
   +      if (empty($form_state->getErrors())) {
   +        $entity->setPublished()->save();
   +      }
   

   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?

3. +++ b/core/modules/media/src/Form/MediaInlineForm.php
   @@ -0,0 +1,233 @@
   +    // @TODO Figure out a more robust way of doing this.
   +    if (strpos($triggering_element['#name'], '_button') === FALSE) {
   +      return TRUE;
   +    }
   +    return FALSE;
   

   what is our plan here?

4. +++ b/core/modules/media/src/Plugin/Field/FieldWidget/MediaFileWidget.php
   @@ -0,0 +1,422 @@
   +        $inline_form_handler = \Drupal::entityTypeManager()
   +          ->getHandler('media', $form_mode);
   +        $media_form = $inline_form_handler->entityForm($media_form, $form_state);
   

   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?

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

1. --- /dev/null
   +++ b/core/modules/media/config/install/core.entity_form_mode.media.add_inline.yml
   
   @@ -0,0 +1,228 @@
   +    // In the service of keeping this form as user-friendly as possible in the
   +    // context of a parent entity form, only show required fields.
   +    /** @var \Drupal\Core\Field\BaseFieldDefinition $field_definition */
   +    foreach ($entity->getFieldDefinitions() as $field_definition) {
   +      $field_name = $field_definition->getName();
   +      if (isset($entity_form[$field_name])) {
   +        $entity_form[$field_name]['#access'] = $field_definition->isRequired();
   +
   +        if ($field_definition->isRequired()) {
   +          // Elements with "#required" key set will always be validated, even if
   +          // '#limit_validation_errors' is set. Disable their validation here,
   +          // we will enforce validation happens inside the real submit handler.
   +          $entity_form[$field_name] = $this->disableElementChildrenValidation($entity_form[$field_name]);
   +        }
   +      }
   +    }
   

   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?

2. +++ b/core/modules/media/src/Form/MediaInlineForm.php
   @@ -0,0 +1,228 @@
   +    $form_display = EntityFormDisplay::collectRenderDisplay($entity, $entity_form['#form_mode']);
   

   Shouldn't this assert that #form_mode equals add_inline?

3. +++ b/core/modules/media/src/Form/MediaInlineForm.php
   @@ -0,0 +1,228 @@
   +    // The media name will be automatically populated by the source plugin. We
   +    // are OK with the default name in this case.
   +    if (isset($entity_form['name'])) {
   +      $entity_form['name']['#access'] = FALSE;
   +    }
   +
   +    // By this point it is expected that the source field has already been
   +    // populated, so hide it too. An example for the file widget can be found in
   +    // @see \Drupal\media\Plugin\Field\FieldWidget\MediaFileWidget::value().
   +    $source_field = $entity->getSource()
   +      ->getSourceFieldDefinition($entity->bundle->entity)
   +      ->getName();
   +    $entity_form[$source_field]['#access'] = FALSE;
   ...
   +    // Inline entities inherit the parent language, so hide translation-related
   +    // fields as well.
   +    if (isset($entity_form['langcode'])) {
   +      $entity_form['langcode']['#access'] = FALSE;
   +    }
   

   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:

   // Hide required fields with sane default values, to simplify the UX.
   $required_fields_with_default_values = [
     'name',
     'langcode' 
     $source_field_name,
   ];
   foreach ($required_fields_with_default_values as $field_name) {
     $entity_form[$field_name]['#access'] = FALSE;
   }
   

4. +++ b/core/modules/media/src/Form/MediaInlineForm.php
   @@ -0,0 +1,228 @@
   +    // If this is an image media entity, and the image field on that entity has
   +    // either "Alt field required" or "Title field required", we take care of
   +    // that in MediaImageWidget::process().
   +    // @see \Drupal\media\Plugin\Field\FieldWidget\MediaImageWidget::process().
   +    foreach (['#alt_field_required', '#title_field_required'] as $extra_image_field_attribute) {
   +      if (isset($entity_form[$source_field]['widget'][0][$extra_image_field_attribute])) {
   +        $entity_form[$source_field]['widget'][0][$extra_image_field_attribute]['#access'] = FALSE;
   +      }
   +    }
   

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

5. +++ b/core/modules/media/src/Form/MediaInlineForm.php
   @@ -0,0 +1,228 @@
   +      $media_entity = MediaFileWidget::getParentMedia($form_state, $fid);
   

   This is hardcoding a call to a specific widget.

6. +++ b/core/modules/media/src/Form/MediaInlineForm.php
   @@ -0,0 +1,228 @@
   +  private function disableElementChildrenValidation(array $element) {
   +    foreach (Element::children($element) as $key) {
   +      if (isset($element[$key]) && $element[$key]) {
   +        $element[$key] = $this->disableElementChildrenValidation($element[$key]);
   +      }
   +    }
   +
   +    if (!empty($element['#needs_validation']) || !empty($element['#required'])) {
   +      $element['#validated'] = TRUE;
   +    }
   +
   +    return $element;
   +  }
   

   This is very frightening.

7. +++ b/core/modules/media/src/Form/MediaInlineForm.php
   @@ -0,0 +1,228 @@
   +  protected function hostFormSubmitted(FormStateInterface $form_state) {
   +    $triggering_element = $form_state->getTriggeringElement();
   +    // @TODO Figure out a more robust way of doing this.
   +    if (strpos($triggering_element['#name'], '_button') === FALSE) {
   +      return TRUE;
   +    }
   +    return FALSE;
   +  }
   

   This is fairly frightening.

8. +++ b/core/modules/media/src/Plugin/Field/FieldWidget/MediaFileWidget.php
   @@ -0,0 +1,330 @@
   +    $media_item = Media::create(['bundle' => $target_bundle]);
   +    $source_field_definition = $media_item->getSource()->getSourceFieldDefinition($media_type);
   +    $source_field_items = $media_item->get($source_field_definition->getName());
   +    $source_field_items->appendItem();
   

   Woah…

9. +++ b/core/modules/media/src/Plugin/Field/FieldWidget/MediaFileWidget.php
   @@ -0,0 +1,330 @@
   +    // If we have a target_id, it's a mid; we're loading a previously-saved
   +    // media entity reference. Translate it to a fid for ManagedFile to use and
   +    // store the media entity for later.
   

   This is frightening too.

   I think class MediaFileWidget would benefit enormously from documentation in its class docblock explaining how this is reusing ManagedFile and/or FileWidget.

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

but I'm not sure how the latest patches could be re-used in the context of the Media Library.

They'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 :)

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

1. Compatibility with the Media Library - as soon as #2834729: [META] Roadmap to stabilize Media Library lands we can change the autocomplete element in this widget to open the Media Library, and almost nothing else about the widget changes. This means that we can remove the in-development widget code from the Media Library issue
2. Support for multiple target bundles
3. Support for existing entities (using an autocomplete element for now, library in the future)
4. A Media Bulk Upload form that is agnostic to this widget - which means that we can allow users to upload files and create media anywhere, which is a common feature request. Check out /admin/content/media/upload after applying the patch for the standalone page.
5. A generic way to pass Media IDs from anything to a widget - opening up different selection methods (ex: a modal popup that lets you enter a Youtube Video ID and creates new media, then closes the modal and passes the ID back to the widget)

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.

Some tweaks for testers looking at #143