Show media add form directly on media type tab in media library

Partial review...

1. +++ b/core/modules/media/src/Annotation/MediaSource.php
   @@ -57,6 +57,16 @@ class MediaSource extends Plugin {
   +  /**
   +   * The classes used to define media source specific forms.
   +   *
   +   * An example of this would be the 'media_library_add' form used by the media
   +   * library module.
   +   *
   +   * @var string[]
   +   */
   +  public $forms = [];
   

   We should clear it with the framework managers, but I think this is a clear signal to implement PluginWithFormsInterface on MediaSourceInterface and MediaSourceBase. Since there is a base class, I suspect we'd be covered under the BC policy.

2. +++ b/core/modules/media_library/css/media_library.theme.css
   @@ -264,31 +277,30 @@
   +.media-library-add__media,
   +.media-library-add__file {
   

   In case anyone asks: the reason we're changing this here is because, in the not too distant future, the oEmbed media source will also include an "add" form which is exposed to Media Library. So it no longer makes sense to refer to it as an "upload" form, since uploading files is only one possible strategy that will be used to add media to the library.

   No need to change anything here, just explicitly calling it out.

3. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -0,0 +1,340 @@
   +  /**
   +   * Indicates whether the 'medium' image style exists.
   +   *
   +   * @var bool
   +   */
   +  protected $mediumStyleExists = FALSE;
   

   Let's change all references to the medium image style to use the new media_library image style that we now ship with. Since that is our fallback style, we can assume it always exists, and totally remove this member from this class and its subclasses.

4. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -0,0 +1,340 @@
   +    $form['status_messages'] = [
   +      '#type' => 'status_messages',
   +    ];
   

   Can we have a comment explaining why we're adding this to the form?

5. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -0,0 +1,340 @@
   +      $form['#attributes']['class'][] = 'media-library-add-form-has-no-input';
   

   Should we maybe use BEM conventions here? I think that media-library-add-form__without-input and media-library-add-form__with-input might be a bit clearer.

   Or, better yet, maybe we should use multiple classes. How about ['media-library-add-form', 'with-input'], to increase styling flexibility?

6. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -0,0 +1,340 @@
   +      $form['actions'] = [
   +        '#type' => 'actions',
   +      ];
   +      $form['actions']['submit'] = [
   +        '#type' => 'submit',
   +        '#value' => $this->t('Save'),
   +        '#ajax' => [
   +          'callback' => '::updateWidget',
   +          'wrapper' => 'media-library-add-form-wrapper',
   +        ],
   +      ];
   

   I wonder if this should be moved into its own protected method. ($this->buildActions())

7. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -0,0 +1,340 @@
   +  /**
   +   * Builds the element for submitting source field value(s).
   +   *
   +   * @param array $form
   +   *   The complete form.
   +   * @param \Drupal\Core\Form\FormStateInterface $form_state
   +   *   The current form state.
   +   *
   +   * @return array
   +   *   The complete form, with the element added.
   +   */
   +  protected function buildInputElement(array $form, FormStateInterface $form_state) {
   +    return $form;
   +  }
   

   Perhaps this method should be abstract. I'm not sure why any subclass would *not* want to implement it.

8. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -0,0 +1,340 @@
   +        // Parents is set here as it is used in the form display.
   

   Can we rephrase? "#parents is set here because the entity form display needs it to build the entity form fields."

9. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -0,0 +1,340 @@
   +    $plugin_definition = $source->getPluginDefinition();
   +    if ($this->mediumStyleExists && $thumbnail_uri = $source->getMetadata($media, $plugin_definition['thumbnail_uri_metadata_attribute'])) {
   +      $element['preview']['thumbnail'] = [
   +        '#theme' => 'image_style',
   +        '#style_name' => 'medium',
   +        '#uri' => $thumbnail_uri,
   +      ];
   

   We can safely change this to use the media_library fallback style. It's not being stored in configuration at all, so changing it here won't affect any upgrade path we need to write.

10. +++ b/core/modules/media_library/src/Form/AddFormBase.php
    @@ -0,0 +1,340 @@
    +    // When the name is not added to the form as a editable field, output
    

    "...as an editable field..."

11. +++ b/core/modules/media_library/src/Form/AddFormBase.php
    @@ -0,0 +1,340 @@
    +    if (isset($element['fields']['revision_log_message'])) {
    +      $element['fields']['revision_log_message']['#access'] = FALSE;
    +    }
    

    Let's make sure that the tests assert that this field is not present in the media library form display.

12. +++ b/core/modules/media_library/src/Form/AddFormBase.php
    @@ -0,0 +1,340 @@
    +      $form_display = EntityFormDisplay::collectRenderDisplay($media, 'media_library');
    +      $form_display->extractFormValues($media, $form['media'][$i]['fields'], $form_state);
    +      $form_display->validateFormValues($media, $form['media'][$i]['fields'], $form_state);
    

    Can we abstract this into a protected method, validateMediaEntity(), so that subclasses have more flexibility to override the per-entity validation logic? This is a similar pattern to what's going in submitForm().

13. +++ b/core/modules/media_library/src/Form/AddFormBase.php
    @@ -0,0 +1,340 @@
    +    $response = new AjaxResponse();
    +    if (!empty($form_state->getErrors())) {
    +      $response->addCommand(new ReplaceCommand('#media-library-add-form-wrapper', $form));
    +    }
    +    else {
    +      $response->addCommand(new OpenModalDialogCommand($this->t('Media library'), $form, [
    +        'dialogClass' => 'media-library-widget-modal',
    +        'height' => '75%',
    +        'width' => '75%',
    +        'title' => $this->t('Media library'),
    +        'modal' => TRUE,
    +      ]));
    +    }
    

    This could use a comment or two.

14. +++ b/core/modules/media_library/src/Form/AddFormBase.php
    @@ -0,0 +1,340 @@
    +  protected function createMediaEntity($value) {
    

    Let's rename $value to $source_field_value, for clarity.

15. +++ b/core/modules/media_library/src/Form/FileUploadForm.php
    @@ -0,0 +1,207 @@
    +    $field_definition = $this->mediaType->getSource()
    +      ->getSourceFieldDefinition($this->mediaType);
    +
    +    if (!is_a($field_definition->getClass(), FileFieldItemList::class, TRUE)) {
    +      throw new \InvalidArgumentException('Can only add media types which use a file field as a source field.');
    +    }
    

    I find this awkwardly placed. I wonder if this stuff should be done in a new protected method ($this->setMediaType($form, $form_state)) so we can do this validation in there.

1. +++ b/core/modules/media_library/src/Form/FileUploadForm.php
   @@ -0,0 +1,207 @@
   +        $form['upload']['upload_help'] = [
   +          '#type' => 'html_tag',
   +          '#tag' => 'p',
   +          '#attributes' => [
   +            'class' => ['file-upload-help'],
   +          ],
   +          '#value' => \Drupal::translation()
   +            ->formatPlural($slots, 'Maximum 1 file.', 'Maximum @count files remaining.'),
   +        ];
   

   Let's be sure the tests assert that this text appears.

2. +++ b/core/modules/media_library/src/Form/FileUploadForm.php
   @@ -0,0 +1,207 @@
   +    if ($form_state->getErrors()) {
   

   We should use $form_state::hasAnyErrors() here.

3. +++ b/core/modules/media_library/src/MediaLibraryUiBuilder.php
   @@ -56,16 +68,19 @@ public static function dialogOptions() {
   +   * @param bool $form_rebuild
   +   *   Whether to rebuild the form or not.
   

   Can we expand the doc comment here to explain why and how this parameter would be set?

4. +++ b/core/modules/media_library/src/MediaLibraryUiBuilder.php
   @@ -192,6 +211,41 @@ protected function buildMediaTypeMenu(MediaLibraryState $state) {
   +   * @param \Drupal\media_library\MediaLibraryState $state
   +   *   The media library configuration.
   

   Let's change the doc comment here: "The current state of the media library, derived from the current request."

5. +++ b/core/modules/media_library/src/MediaLibraryUiBuilder.php
   @@ -192,6 +211,41 @@ protected function buildMediaTypeMenu(MediaLibraryState $state) {
   +    $selected_type = $this->entityTypeManager->getStorage('media_type')->load($selected_type_id);
   +    $form_class = $selected_type->getSource()->getPluginDefinition()['forms']['media_library_add'] ?? NULL;
   

   If we use PluginWithFormsInterface, this can all go down to one long line:

   $form_class = $this->entityTypeManager->getStorage('media_type')->load($selected_type_id)->getSource()->getFormClass('media_library_add');

6. +++ b/core/modules/media_library/src/MediaLibraryUiBuilder.php
   @@ -192,6 +211,41 @@ protected function buildMediaTypeMenu(MediaLibraryState $state) {
   +    // media library with an new version of the media add form. The form API
   

   "...with a new version..."

7. +++ b/core/modules/media_library/src/MediaLibraryUiBuilder.php
   @@ -192,6 +211,41 @@ protected function buildMediaTypeMenu(MediaLibraryState $state) {
   +      $form_state->setUserInput([]);
   

   Wouldn't setRebuild() have the same effect? If not, why not?

1. +++ b/core/modules/media_library/css/media_library.theme.css
   @@ -70,6 +70,23 @@
   +.media-library-add-form.with-input {
   +  border-bottom: 0;
   +}
   +
   +.media-library-add-form.without-input .form-item {
   +  margin: 0 0 1em;
   +}
   

   I'm not sure we need both with-input and without-input classes. .media-library-add-form, by itself, can target the form without any input.

2. +++ b/core/modules/media_library/css/media_library.theme.css
   @@ -264,31 +281,26 @@
   +.media-library-add__media:last-child {
   +  border-bottom: 0;
   

   Can this rule get a comment explaining why :last-child is added?

3. +++ b/core/modules/media_library/media_library.module
   @@ -41,6 +39,21 @@ function media_library_help($route_name, RouteMatchInterface $route_match) {
   +  $addition = [
   +    'forms' => [
   +      'media_library_add' => '\Drupal\media_library\Form\FileUploadForm',
   +    ],
   +  ];
   +  $sources['audio_file'] = array_merge($sources['audio_file'], $addition);
   +  $sources['video_file'] = array_merge($sources['video_file'], $addition);
   +  $sources['file'] = array_merge($sources['file'], $addition);
   +  $sources['image'] = array_merge($sources['image'], $addition);
   

   I know I wrote this originally, but now this array_merge seems weird to me. IMHO, we should just explicitly set the class four times, like so:

   $class = '\Drupal\media_library\Form\FileUploadForm';
   $sources['audio_file']['forms']['media_library_add'] = $class;
   $sources['video_file']['forms']['media_library_add'] = $class;
   ...
   

   I think this will be less intrusive, not to mention more readily understandable, than array_merge().

4. +++ b/core/modules/media_library/media_library.routing.yml
   @@ -1,9 +1,3 @@
   -media_library.upload:
   -  path: '/admin/content/media-widget-upload'
   -  defaults:
   -    _form: '\Drupal\media_library\Form\MediaLibraryUploadForm'
   -  requirements:
   -    _custom_access: '\Drupal\media_library\Form\MediaLibraryUploadForm::access'
   

   I hope this doesn't constitute a BC break. Even if it does, I don't think we can keep it around -- the very form it refers to is disappearing, and that code path is dead as a doornail. So if this is a BC break, then as far as I as a subsystem maintainer am concerned, it's a necessary one in an experimental module. No need to change anything here, just pointing it out.

5. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -0,0 +1,405 @@
   +   * Static cache of the type of media items being created by this form.
   

   This isn't exactly a static cache (or at least, that terminology has a different connotation), so can this simply say "The media type being created by this form"?

6. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -0,0 +1,405 @@
   +  protected $mediaType;
   

   I'm tempted to say that we should make this private, specifically to force subclasses into using $this->getMediaType() where possible, which means that any validation or processing they do in getMediaType() will always run (which, IMHO, reduces the number of possible vectors for bugs). For now, let's hold off, but this is yet another question I want to defer to a framework manager.

7. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -0,0 +1,405 @@
   +    $this->mediaType = $this->entityTypeManager->getStorage('media_type')
   +      ->load($form_state->get('media_library_state')->getSelectedTypeId());
   +    if (!$this->mediaType) {
   +      throw new \LogicException('Exactly one media type is needed in order to create media items.');
   +    }
   

   This exception message no longer makes sense. How about "The '$selected_type' media type does not exist"?

8. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -0,0 +1,405 @@
   +      $form['#attributes']['class'][] = 'without-input';
   

   As noted above, I think we can remove this class.

9. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -0,0 +1,405 @@
   +      $form['actions'] = $this->buildActions($form, $form_state);
   +      $form['actions']['#type'] = 'actions';
   

   I think that buildActions() should be responsible for building the _entire_ actions element, including the #type => 'actions' piece of it.

10. +++ b/core/modules/media_library/src/Form/AddFormBase.php
    @@ -0,0 +1,405 @@
    +    $form_state->set('media', $media);
    +    $form_state->setRebuild();
    

    Nit: These calls are chainable.

11. +++ b/core/modules/media_library/src/Form/AddFormBase.php
    @@ -0,0 +1,405 @@
    +  /**
    +   * Prepares a created media item to be permanently saved.
    +   *
    +   * @param \Drupal\media\MediaInterface $media
    +   *   The unsaved media item.
    +   */
    +  protected function preSaveMediaEntity(MediaInterface $media) {
    +    // Intentionally empty by default.
    +  }
    

    I think we should rename this method to prepareMediaEntityForSave(). Longer, yes, but clearer! preSaveMediaEntity() sounds like a pre-save hook, which is completely different :)

12. +++ b/core/modules/media_library/src/Form/AddFormBase.php
    @@ -0,0 +1,405 @@
    +    if (!empty($form_state->hasAnyErrors())) {
    

    hasAnyErrors() is static, so this should be $form_state::hasAnyErrors(). Also, we don't need to use !empty() -- hasAnyErrors() returns a boolean, so calling empty() is redundant.

13. +++ b/core/modules/media_library/src/Form/FileUploadForm.php
    @@ -0,0 +1,232 @@
    +    if ($this->mediaType) {
    +      return $this->mediaType;
    +    }
    +
    +    $this->mediaType = parent::getMediaType($form_state);
    

    This isn't needed. We can just do $media_type = parent::getMediaType($form_state).

14. +++ b/core/modules/media_library/src/Form/FileUploadForm.php
    @@ -0,0 +1,232 @@
    +      '#title' => $this->formatPlural($slots, 'Add file', 'Add files'),
    

    Nice!

15. +++ b/core/modules/media_library/src/Form/FileUploadForm.php
    @@ -0,0 +1,232 @@
    +    $file_upload_help = [
    +      '#theme' => 'file_upload_help',
    +      '#upload_validators' => $form['upload']['#upload_validators'],
    +      '#cardinality' => $slots,
    +    ];
    +    $form['upload']['#description'] = $this->renderer->renderPlain($file_upload_help);
    

    Is there some reason we need to render the upload help now, rather than just set it on $form['upload_help'] and let it be rendered later? If so, we need to document that with a comment.

16. +++ b/core/modules/media_library/src/Form/FileUploadForm.php
    @@ -0,0 +1,232 @@
    +    if ($form_state->getErrors()) {
    

    Should be $form_state::hasAnyErrors().

17. +++ b/core/modules/media_library/src/Form/FileUploadForm.php
    @@ -0,0 +1,232 @@
    +    if (!file_prepare_directory($upload_location, FILE_CREATE_DIRECTORY)) {
    +      throw new \Exception("The destination directory '$upload_location' is not writable");
    +    }
    +    $file = file_move($file, $upload_location);
    +    if (!$file) {
    +      throw new \Exception("Unable to move file to '$upload_location'");
    +    }
    

    Let's throw \RuntimeException instead of just \Exception.

There is a lot of work gone into this, its really appreciated.

There is a lot of css/js here so adding Needs frontend framework manager review tag.

1. +++ b/core/modules/media_library/css/media_library.theme.css
   @@ -5,6 +5,84 @@
   +  border-right: 1px solid #fcfcfa;
   ...
   +  border-left: 1px solid #b3b2ad;
   

   do we need RTL equivalents here?

2. +++ b/core/modules/media_library/js/media_library.ui.es6.js
   @@ -0,0 +1,312 @@
   +            $('#media-library-content').focus();
   

   should we use getElementById here instead of jQuery?

3. +++ b/core/modules/media_library/js/media_library.ui.es6.js
   @@ -0,0 +1,312 @@
   +          .find('#media-library-modal-selection')
   +          .val(currentSelection.join())
   

   note to self, do we check access on this

4. +++ b/core/modules/media_library/media_library.module
   @@ -90,11 +68,7 @@ function media_library_views_post_render(ViewExecutable $view, &$output, CachePl
   +      $query = MediaLibraryState::fromRequest()->all();
   

   we should pass along the request here

5. +++ b/core/modules/media_library/media_library.services.yml
   @@ -0,0 +1,4 @@
   +  media_library.ui_builder:
   

   the only place I see this used is in the routing and in tests.

   we deliberately decided not to make controllers services in the D8 design process because we didn't want to bloat the service container.

   I don't see a reason to do it here either.

   Am I missing something?

   We have ContainerInjectionInterface if you need to inject stuff into the controller and we have the ControllerResolver if you need to instantiate the class in a test.

6. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -0,0 +1,401 @@
   +    $source_field_name = $this->getSourceFieldName($media->bundle->entity);
   +    if (isset($element['fields'][$source_field_name])) {
   +      $element['fields'][$source_field_name]['#attributes']['class'][] = 'media-library-add__source-field';
   +    }
   

   Why do we hide them with CSS and not use form modes?

7. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -0,0 +1,401 @@
   +    return $this->entityTypeManager
   +      ->getStorage('media')
   +      ->create([
   +        'bundle' => $media_type->id(),
   +        $this->getSourceFieldName($media_type) => $source_field_value,
   

   we're calling this in the array_map which means we're doing the getStorage and the getSourceFieldame each time, but really we could do it once and pass it along

8. +++ b/core/modules/media_library/src/MediaLibraryState.php
   @@ -0,0 +1,204 @@
   +      $request->query->get('media_library_opener_id'),
   +      $request->query->get('media_library_allowed_types'),
   +      $request->query->get('media_library_selected_type'),
   +      $request->query->get('media_library_remaining')
   

   I think we need an anti-tamper token here too

9. +++ b/core/modules/media_library/src/MediaLibraryState.php
   @@ -0,0 +1,204 @@
   +    $existing_ids = \Drupal::entityTypeManager()->getStorage('media_type')
   

   should we inject this?

10. +++ b/core/modules/media_library/src/MediaLibraryUiBuilder.php
    @@ -0,0 +1,260 @@
    +    $state = MediaLibraryState::fromRequest();
    ...
    +    $state = MediaLibraryState::fromRequest();
    

    i feel we should be passing the request object here, this is a non-static method, we have container injection, we should fetch it and pass it along

    Once we do that, we make the parameter required and it no longer needs the singleton

11. +++ b/core/modules/media_library/src/MediaLibraryUiBuilder.php
    @@ -0,0 +1,260 @@
    +    $view = Views::getView('media_library');
    

    should we inject here instead of using the singleton?

12. +++ b/core/modules/media_library/src/MediaLibraryUiBuilder.php
    @@ -0,0 +1,260 @@
    +    $allowed_types = $this->entityTypeManager->getStorage('media_type')->loadMultiple($allowed_type_ids);
    

    yeah, defs should be adding an anti-tamper token to prevent people meddling with the allowed types

13. +++ b/core/modules/media_library/src/MediaLibraryUiBuilder.php
    @@ -0,0 +1,260 @@
    +      $query['media_library_selected_type'] = $allowed_type_id;
    

    the last one is always selected?

14. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -41,6 +43,13 @@ class MediaLibraryWidget extends WidgetBase implements ContainerFactoryPluginInt
    +  protected static $openerIdPrefix = 'field:';
    

    are we sure a global static is the right choice? - from the uses I see, could be a constant

15. +++ b/core/modules/media_library/src/Plugin/views/field/MediaLibrarySelectForm.php
    @@ -64,6 +64,17 @@ public function viewsForm(array &$form, FormStateInterface $form_state) {
    +    $selection_field_id = $this->options['id'] . '_selection';
    +    $form[$selection_field_id] = [
    +      '#type' => 'hidden',
    +      '#attributes' => [
    +        // This is used to identify the hidden field in the form via JavaScript.
    +        'id' => 'media-library-modal-selection',
    +      ],
    +    ];
    

    what controls are in place to prevent someone from editing the dom on this and adding in references to media entities they cannot access?

16. +++ b/core/modules/media_library/src/Plugin/views/field/MediaLibrarySelectForm.php
    @@ -95,17 +109,22 @@ public function viewsForm(array &$form, FormStateInterface $form_state) {
    +    $opener_id = MediaLibraryState::fromRequest()->getOpenerId();
    

    we should pass on the request here, obtained from DI on the plugin

Sure, @xjm asked me to review this one - sorry for the crossed wires

OK, so in addition to general framework manager sign-off, these are the two questions we specifically need framework managers to answer for us:

1. This patch introduces the idea of attaching a "media_library_add" form class to source plugin definitions. Should we, can we, implement PluginWithFormsInterface? See #7.1.

2. In AddFormBase, can we/should we make the $mediaType member private? This would force all subclasses to use the protected getMediaType() method, which is where validation or other logic surrounding the media type would go. See #11.6.

Now that #3020716: Add vertical tabs style menu to media library is in, we probably need a reroll here.

+++ b/core/modules/media_library/src/Form/AddFormBase.php
@@ -0,0 +1,404 @@
+      $form['#attributes']['class'][] = 'without-input';
...
+      $form['#attributes']['class'][] = 'with-input';

This should be media-library-add-form--without-input and media-library-add-form--with-input.

This was a very partial review since I don't seem to be able to get the uploading working. I get the following error when trying to upload either file or image:

An AJAX HTTP error occurred.
HTTP Result Code: 200
Debugging information follows.
Path: /media-library?media_library_opener_id=field%3Afield_media&media_library_allowed_types%5Bfile%5D=file&media_library_allowed_types%5Bimage%5D=image&media_library_selected_type=image&media_library_remaining=8&media_library_content=1&_wrapper_format=drupal_ajax&ajax_form=1
StatusText: OK
ResponseText: Error: Call to a member function transformDimensions() on null in template_preprocess_image_style() (line 301 of /Users/lauri.eskola/Projects/drupal/core/modules/image/image.module).

I got the patch working after @phenaproxima suggested me to run database updates.

1. Media entities selected prior to uploading new media file are lost. I would have expected those to remain attached.
2. I found the second step of the upload confusing; I wasn't sure if the file was already saved or not. This was relevant because I accidentally first uploaded a wrong file. I'm wondering also if the text on the submit button should be changed from "Save" to something like "Save and attach" to be more clear of the action.
3. There's no way to cancel upload without losing selected files in the media listing. This is again relevant because I uploaded a wrong file, and the only way to restart the process was to close the dialog.

4. +++ b/core/modules/media_library/css/media_library.theme.css
   @@ -313,7 +323,7 @@
   +.media-library-add__media-preview img {
   

   Let's add a class directly to the img element and target that.

5. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -0,0 +1,404 @@
   +    $form['#prefix'] = '<div id="media-library-add-form-wrapper">';
   +    $form['#suffix'] = '</div>';
   

   Is there another way this markup could be generated? Generating markup like this in #prefix and #suffix isn't necessarily the cleanest approach. Also, the id given for the element is not ensured to be unique.

6. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -0,0 +1,404 @@
   +          'media-library-add__media',
   

   Not sure why this includes a double underscore. Feel free to come up with a better class name but this should be something like media-libary-add-media-form

7. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -0,0 +1,404 @@
   +          'media-library-add__media',
   ...
   +            'media-library-add__media-preview',
   

   This should be media-libary-add-media-form__preview

8. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -0,0 +1,404 @@
   +            'media-library-add__media-fields',
   

   This should be media-libary-add-media-form__fields

9. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -0,0 +1,404 @@
   +      $element['fields'][$source_field_name]['#attributes']['class'][] = 'media-library-add__source-field';
   

   This should be media-libary-add-media-form__source-field

The bug reported on #3031662 still exist in this last patch. There is no warning for the user when uploading a file bigger than what's allowed.

From a UX perspective, could we load all the media options interfaces (for image, video, etc.) and show only the active one using css? Currently, switching between media types displays a spinner and takes between 0.5 - 1 seconds to load.

I don't see mentions of #21.1-3 on those issues. We should make sure those problems get documented somewhere specifically so they get addressed one way or another.

Thanks, @lauriii! Documented in #3023797-11: Let users choose what to do after selecting and/or adding items in the media library.

I had a long discussion with @xjm about framework manager review, and what it means. The conclusion I reached is that this issue does not need framework manager review, so I'm removing the tag.

Why?

1. 99% of this patch is refactoring classes in Media Library which are explicitly marked @internal. This impacts no other subsystems, or even other modules (including Media). A single class is split into two (one abstract and one concrete), and both are still marked internal. All this is being done specifically in order to pave the way for #2996029: Add oEmbed support to the media library.
2. Only one change is made to the stable Media module: a public property is added to its MediaSource plugin annotation. But this presents no problem from a backwards compatibility standpoint.
3. A route is removed, but there was no way to keep it. Not only is it completely dead, it's not even usable with the new design and architecture of the media library, which is still experimental.

All this does not, I think, add up to a "significant" change to Drupal core. From an architecture, API, and BC standpoint, I think we're safe.

@seanB and I demonstrated this patch to the UX team in the usability meeting. The findings were very productive!

• Everyone agreed with @lauriii that the current flow, where you upload an image, enter some metadata, and click "Save", only to be kicked back to the node form with your previous selections forgotten, is unacceptable. It's too much of a regression, even to address in a follow-up issue (which would, necessarily, be a critical). So, in this patch, we are going to change the action of the Save button so that it sends you back to the media library, with your previous selections, in addition to your new upload, still selected.
• But we can't stop there, because that workflow is going to be too cumbersome for some. To mitigate that, we're going to repurpose #3023797: Let users choose what to do after selecting and/or adding items in the media library as a high-priority follow-up which will a) add a second button the metadata screen, which will give the user two options (actual wording may vary): "Save and return to library", and "Save and insert selected items". This is still not amazing, but it allows the user to use the media library in whichever way is most efficient for them. As part of that work, we will add an area to the metadata screen which will display small thumbnails of all the items you have already selected from the library. This is important from both an accessibility and usability perspective. @ckrina has promised to create a design for this area.

See #3023797-12: Let users choose what to do after selecting and/or adding items in the media library for the new direction that issue is taking based on the UX feedback.

1. +++ b/core/modules/media/src/Annotation/MediaSource.php
   @@ -57,6 +57,16 @@ class MediaSource extends Plugin {
   +   * The classes used to define media source specific forms.
   

   Nit: This should be "source-specific". Also, we should mention that these are to be keyed by operation, and the values are the fully qualified class names of the forms to use.

2. +++ b/core/modules/media_library/js/media_library.ui.es6.js
   @@ -13,6 +13,13 @@
   +    /**
   +     * When a user is returned to the media library after adding new media items
   +     * the item IDs are passed via the Drupal settings. The IDs of processed
   +     * media items are stored to make sure every created media item is only
   +     * added to the current selection once.
   +     */
   +    processedCreatedItems: [],
   

   I'm not sure I understand the rationale for this. Why wouldn't we just add them to currentSelection? How will that cause duplication?

3. +++ b/core/modules/media_library/js/media_library.ui.es6.js
   @@ -240,6 +248,18 @@
   +      // selection, and add the IDs to a list of processed items so the are
   

   "so they are..."

4. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -0,0 +1,420 @@
   +/**
   + * Provides a base class for creating media items from within the media library.
   + */
   +abstract class AddFormBase extends FormBase {
   

   I think we should mark this class @internal too, with a warning that it should not be extended or used by any code outside of Media Library.

5. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -0,0 +1,420 @@
   +  /**
   +   * The media library UI builder.
   +   *
   +   * @var \Drupal\media_library\MediaLibraryUiBuilder
   +   */
   +  protected $libraryUiBuilder;
   

   This almost seems like a circular dependency -- the UI builder already has the form builder, which in turn builds this form, which also has the form builder...scary stuff. Not sure if there's any reason to be concerned, but it's a bit unnerving.

6. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -0,0 +1,420 @@
   +    // @todo Make this configurable in https://www.drupal.org/node/2988223
   

   Make what configurable? Can we be a bit more specific?

7. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -0,0 +1,420 @@
   +   * @return array
   +   *   An array of supported Form API action elements keyed by name.
   +   */
   +  protected function buildActions(array $form, FormStateInterface $form_state) {
   

   This isn't really returning an array of action elements; it's returning an element containing the actions.

8. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -0,0 +1,420 @@
   +   * Update the form state with media items created from source field values.
   

   Let's rephrase this: "Creates media items from source field input values."

9. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -0,0 +1,420 @@
   +   *   The values for source field of the the media item.
   

   "field" and "item" should be plural.

10. +++ b/core/modules/media_library/src/Form/AddFormBase.php
    @@ -0,0 +1,420 @@
    +  protected function updateFormState(array $source_field_values, array $form, FormStateInterface $form_state) {
    

    I think this method is strangely named. How about something like processInputValues() or createMediaItemsFromInputValues()? Not sure what would be best here, but I don't think updateFormState() is descriptive enough.

11. +++ b/core/modules/media_library/src/Form/AddFormBase.php
    @@ -0,0 +1,420 @@
    +  protected function createMediaEntity(MediaTypeInterface $media_type, EntityStorageInterface $media_storage, $source_field_name, $source_field_value) {
    

    Let's rename this as well -- createMediaFromValue() is probably descriptive enough.

12. +++ b/core/modules/media_library/src/Form/AddFormBase.php
    @@ -0,0 +1,420 @@
    +    if ($form_state::hasAnyErrors()) {
    +      $response = new AjaxResponse();
    +      $response->addCommand(new ReplaceCommand('#media-library-add-form-wrapper', $form));
    +      return $response;
    +    }
    

    So, question -- isn't this logic pretty much the same thing as return $form?

13. +++ b/core/modules/media_library/src/Form/AddFormBase.php
    @@ -0,0 +1,420 @@
    +    $state->remove('media_library_content');
    

    What is this? Can we get a comment here?

14. +++ b/core/modules/media_library/src/Form/AddFormBase.php
    @@ -0,0 +1,420 @@
    +    $response = new AjaxResponse();
    +    $response->addCommand(new ReplaceCommand('#media-library-add-form-wrapper', $library_ui));
    +    return $response;
    

    Can't we just return $library_ui here?

15. +++ b/core/modules/media_library/src/Form/FileUploadForm.php
    @@ -0,0 +1,244 @@
    + * @internal
    + */
    +class FileUploadForm extends AddFormBase {
    

    Let's copy that @internal warning from AddFormBase here too.

16. +++ b/core/modules/media_library/src/Form/FileUploadForm.php
    @@ -0,0 +1,244 @@
    +    // The file upload help needs to be rendered since the description does not
    +    // accept render arrays. The FileWidget::formElement() method adds the file
    +    // upload help in the same way, so any theming improvements made to file
    +    // fields would also be applied to this upload field.
    +    // @see \Drupal\file\Plugin\Field\FieldWidget\FileWidget::formElement()
    +    $form['upload']['#description'] = $this->renderer->renderPlain($file_upload_help);
    

    Nice comment here. Thanks!

17. +++ b/core/modules/media_library/src/Form/FileUploadForm.php
    @@ -0,0 +1,244 @@
    +    if ($form_state::hasAnyErrors()) {
    +      $element['#value'] = [];
    +    }
    

    I think this could use a comment. What will setting $element['#value'] to an empty array accomplish? Is there any relevant method in ManagedFile that we can refer to?

18. +++ b/core/modules/media_library/src/Form/FileUploadForm.php
    @@ -0,0 +1,244 @@
    +    $element['upload_button']['#ajax'] = [
    +      'callback' => '::updateFormCallback',
    +      'wrapper' => 'media-library-wrapper',
    +    ];
    

    The fact that this knows the ID of the wrapper is a bit troublesome, because it shows a lack of encapsulation. That said, given how the AJAX system works, I'm not sure what else we could do here. Also, since this code is also in Media Library, and it's marked internal, it's not a big deal for now.

19. +++ b/core/modules/media_library/src/MediaLibraryUiBuilder.php
    @@ -77,16 +89,25 @@ public static function dialogOptions() {
    +   * @param bool $form_rebuild
    +   *   After the form to add new media is submitted, we need to rebuild the
    +   *   media library with an new version of the media add form. The form API
    +   *   allows us to do that by forcing empty user input.
    

    We need to start this description with (optional) and mention that it defaults to FALSE.

20. +++ b/core/modules/media_library/src/MediaLibraryUiBuilder.php
    @@ -110,11 +131,15 @@ public function buildUi() {
    +   * @param bool $form_rebuild
    +   *   After the form to add new media is submitted, we need to rebuild the
    +   *   media library with an new version of the media add form. The form API
    +   *   allows us to do that by forcing empty user input.
    

    Same here.

21. +++ b/core/modules/media_library/src/MediaLibraryUiBuilder.php
    @@ -216,6 +242,44 @@ protected function buildMediaTypeMenu(MediaLibraryState $state) {
    +    $form_state = new FormState();
    +    if ($form_rebuild) {
    +      $form_state->setUserInput([]);
    +    }
    

    I'm not a huge fan of passing random boolean flags around to do stuff like this, but it makes even less sense to have the calling code create and pass in a FormState object. So I guess I'm okay with this for now...hopefully we can refactor it later.

22. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
    @@ -533,6 +533,46 @@ public function testWidgetUpload() {
    +    $assert_session->assertWaitOnAjaxRequest();
    +    $assert_session->pageTextContains('Media library');
    

    Can we wait for a specific element instead?

23. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
    @@ -533,6 +533,46 @@ public function testWidgetUpload() {
    +    $assert_session->fieldNotExists('Add file');
    +    $assert_session->fieldNotExists('Add files');
    

    I thought this was a duplicate at first :) It might be better to assert this by the name of the field, rather than the label.

24. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
    @@ -544,10 +584,15 @@ public function testWidgetUpload() {
    +    // Assert the default tab for media type one does not have an upload form.
    +    $assert_session->fieldNotExists('Add file');
    +    $assert_session->fieldNotExists('Add files');
    

    Same here.

25. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
    @@ -544,10 +584,15 @@ public function testWidgetUpload() {
    +    $page->find('css', '.media-library-menu .media-library-menu-type-three > a')->click();
    

    Can't we click on the link by its text? That'd be so much clearer. I believe we could even do

    $page->find('css', '.media-library-menu')->clickLink('Link text')</a>, which is quite a bit cleaner.
    </li>
    
    <li>
    <code>
    +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
    @@ -577,52 +643,79 @@ public function testWidgetUpload() {
    +    $checkbox_selector = '.media-library-view .js-click-to-select-checkbox input';
    +    $checkboxes = $this->getSession()->getPage()->findAll('css', $checkbox_selector);
    +    $checkboxes[0]->click();
    

    This could just be one (long) line: $page->find('css', '.media-library-view .js-click-to-select-checkbox input')->click(). find() will automatically target the first one.

26. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
    @@ -577,52 +643,79 @@ public function testWidgetUpload() {
    +    // Ensure the created item is added the widget.
    

    "added the widget"? I think we're missing an "in" :)

Awww yeah. Sam saves the day -- that's a much nicer solution.

1. +++ b/core/modules/media_library/js/media_library.ui.es6.js
   @@ -13,13 +13,28 @@
   +  Drupal.AjaxCommands.prototype.updateCurrentSelection = function(
   

   We should probably namespace this a bit -- updateMediaLibrarySelection might be a better name.

2. +++ b/core/modules/media_library/js/media_library.ui.es6.js
   @@ -13,13 +13,28 @@
   +    if (response.mids) {
   +      Object.values(response.mids).forEach(value => {
   +        Drupal.MediaLibrary.currentSelection.push(value);
   +      });
   +    }
   

   If we always supply an array in response.mids, even if it's empty, we don't need the if check.

3. +++ b/core/modules/media_library/src/Ajax/UpdateCurrentSelectionCommand.php
   @@ -0,0 +1,51 @@
   +class UpdateCurrentSelectionCommand implements CommandInterface {
   

   I think the word "current" in this name is redundant. Can we just call this UpdateSelectionCommand?

4. +++ b/core/modules/media_library/src/Ajax/UpdateCurrentSelectionCommand.php
   @@ -0,0 +1,51 @@
   +  /**
   +   * Constructs an UpdateCurrentSelectionCommand object.
   +   *
   +   * @param int[] $mids
   +   *   An array of media IDs to add to the current selection.
   +   */
   +  public function __construct(array $mids) {
   +    $this->mids = $mids;
   +  }
   

   Idea -- let's add a flag in here which allows the command to either _add_ the IDs to the current selection, or completely _replace_ the current selection with the new IDs. I think this will serve us well in the future.

5. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -131,7 +134,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +   * the user input and store them in the form state using ::processInputValues().
   

   Supernit: This line is 82 characters long.

6. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -586,11 +584,10 @@ public function testWidgetUpload() {
   +    $assert_session->elementExists('named', ['link', 'Type Three'])->click();
   

   I think we can use $page->clickLink() for this.

Another partial review...

1. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -0,0 +1,430 @@
   +    $this->mediaType = $this->entityTypeManager->getStorage('media_type')
   +      ->load($form_state->get('media_library_state')->getSelectedTypeId());
   +    return $this->mediaType;
   

   For paranoia's sake, let's throw exceptions if $form_state doesn't have media_library_state, and if $this->mediaType is null after the call to load(). That will be preferable to the possibility of calling methods on null values.

2. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -0,0 +1,430 @@
   +
   +    $form['#attributes']['class'][] = 'media-library-add-form';
   +    $media_items = $form_state->get('media');
   +    if (empty($media_items)) {
   +      $form['#attributes']['class'][] = 'media-library-add-form--without-input';
   +      $form = $this->buildInputElement($form, $form_state);
   +    }
   +    else {
   +      $form['#attributes']['class'][] = 'media-library-add-form--with-input';
   

   Do the tests assert that these CSS classes are applied and removed as expected? If not, they probably should. :)

3. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -0,0 +1,430 @@
   +      foreach ($media_items as $delta => $media) {
   +        $form['media'][$delta] = $this->buildEntityForm($media, $form, $form_state, $delta);
   +      }
   

   Mink doesn't support multi-file uploading, so we'll probably have to be sure to manually test this before commit.

4. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -0,0 +1,430 @@
   +    // We hide certain elements in the image widget with CSS.
   

   Which elements are hidden? This should probably be a little more specific and say what is hidden, and why.

5. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -0,0 +1,430 @@
   +    if (isset($element['fields']['revision_log_message'])) {
   +      $element['fields']['revision_log_message']['#access'] = FALSE;
   +    }
   

   This could use a comment explaining that we hide the field here because it is not configurable at the form display level. If there's an issue to make it configurable, we should reference it here, with a @todo to remove this bit.

6. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -0,0 +1,430 @@
   +  protected function createMediaFromValue(MediaTypeInterface $media_type, EntityStorageInterface $media_storage, $source_field_name, $source_field_value) {
   

   I'm not entirely sure we need to pass the entity storage handler to this method; we already have the entity type manager as a protected property on this class, and it's pretty harmless to have this method call $this->entityTypeManager->getStorage(). This is a method that subclasses are likely to override, so I feel like streamlining the method signature would be preferable.

7. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -0,0 +1,430 @@
   +    // Pass the created IDs via the Drupal settings so we can automatically
   +    // select them in the library.
   +    $library_ui['#attached']['drupalSettings']['media_library']['created'] = $mids;
   

   Is this still needed?

After these changes, I have read the patch enough. From there, we'll need screenshots, plus a demo video, and manual testing (i.e., for multiple uploads and general stuff). Then RTBC.

1. +++ b/core/modules/media_library/js/media_library.ui.es6.js
   @@ -15,6 +15,26 @@
   +    Object.values(response.mids).forEach(value => {
   

   Do we really need to use Object.values()? I ask because response.mids is just a simple list of integers, which _should_ JSON-ize as an array which can be directly forEach'ed over.

2. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -0,0 +1,443 @@
   +      throw new \UnexpectedValueException('The media library state is not present in the form state.');
   

   I think this should probably be InvalidArgumentException.

3. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -0,0 +1,443 @@
   +    if (!$this->mediaType) {
   +      throw new \UnexpectedValueException('The selected media type in the media library state does not exist.');
   +    }
   

   This exception could be a little more helpful with a message like "The '$selected_type_id' media type does not exist." Also, this should probably be InvalidArgumentException.

   Also, can we add some unit test coverage for these exceptions? I ask because, if the media type is invalid in some way, the entire form is unusable. So we should probably ensure this validation works.

4. +++ b/core/modules/media_library/src/MediaLibraryUiBuilder.php
   @@ -77,16 +89,25 @@ public static function dialogOptions() {
   +   *   (optional) After the form to add new media is submitted, we need to rebuild the
   

   Nit: Longer than 80 characters.

+++ b/core/modules/media_library/src/MediaLibraryUiBuilder.php
@@ -77,16 +89,25 @@ public static function dialogOptions() {
+  public function buildUi(MediaLibraryState $state = NULL, $form_rebuild = FALSE) {
+    if (!$state) {
+      $state = MediaLibraryState::fromRequest($this->request);
+    }

I had an idea for a way to not continually pass $form_rebuild down through three method calls -- let's just set a flag on $state!

if (!$state) {
  $state = MediaLibraryState::fromRequest($this->request);
}
$state->set('_media_library_form_rebuild', $form_rebuild);

Then, later on, in buildMediaTypeAddForm():

$form_state = new FormState();
if ($state->get('_media_library_form_rebuild')) {
  $form_state->setUserInput([]);
  $state->remove('_media_library_form_rebuild');
}

We could also add a new getter/setter on MediaLibraryState specifically for this (which might be cleaner and doesn't require us to pollute the parameter bag); something like $state->isRebuildingMediaAddForm() and $state->setRebuildMediaAddForm().

+++ b/core/modules/media_library/tests/src/Kernel/MediaLibraryAddFormTest.php
@@ -0,0 +1,111 @@
+    // Assert the media library UI does not contains the add form when the user

Nit: "contains" should be "contain".

Otherwise, this is ready to go with a bit of manual testing, screenshots, and demo videos. Nice!

Adding screenshots to the IS.

Created a short demo video. I was surprised by the error message showing up floated to the left of the vertical tabs, though, so kicking back for that. :)

Meant to send it back to "Needs work"...

This mostly looks great. You two have been doing amazing work on all these media issues!

Partial drive-by review, mostly nit-tastic quibbling. I'm out of time right now so I didn't go over every line, but a few things stuck out. Please ignore as appropriate. ;)

1. +++ b/core/modules/media/src/Annotation/MediaSource.php
   @@ -57,6 +57,16 @@ class MediaSource extends Plugin {
   +  /**
   +   * The classes used to define media source-specific forms.
   +   *
   +   * An example of this would be the 'media_library_add' form used by the media
   +   * library module.
   +   *
   +   * @var string[]
   +   */
   +  public $forms = [];
   +
   

   I don't fully grok this comment, nor the name of the member. ;) It's an array of strings. It's called "forms". The doc says "The classes used..." the example looks like a form_id, not a class name. Can we get a live example in this comment? Can the help text match? Can the member be named something more self-documenting? $formClasses or $formIds or whatever?

2. +++ b/core/modules/media_library/media_library.module
   @@ -41,6 +40,16 @@ function media_library_help($route_name, RouteMatchInterface $route_match) {
   +  $sources['audio_file']['forms']['media_library_add'] = FileUploadForm::class;
   +  $sources['file']['forms']['media_library_add'] = FileUploadForm::class;
   +  $sources['image']['forms']['media_library_add'] = FileUploadForm::class;
   +  $sources['video_file']['forms']['media_library_add'] = FileUploadForm::class;
   

   Ahah! ;) That helps. So $forms is supposed to be keyed by form_id with a classname as the value, huh? Let's say so above.

3. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -0,0 +1,444 @@
   + * @internal
   

   FYI: I just saw @xjm complain in the JSON:API code review that she wanted an explanation why something is @internal, and what might go wrong if you try to use it. I can see her point. On the surface, it's not clear why I'm not supposed to extend this myself...

4. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -0,0 +1,444 @@
   +   * @return array
   +   *   The element containing the required fields sub-form.
   +   */
   +  protected function buildEntityForm(MediaInterface $media, array $form, FormStateInterface $form_state, $delta) {
   

   the return value says it's an "element", but the function name says "buildEntityForm". Should this be called buildEntityFormElement() or something?

5. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -0,0 +1,444 @@
   +    // display, so it hidden by changing the access.
   

   "...so hide it by..."

6. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -0,0 +1,444 @@
   +    // @todo Make the revision_log_message field configurable in
   +    //   https://www.drupal.org/project/drupal/issues/2696555
   +    if (isset($element['fields']['revision_log_message'])) {
   +      $element['fields']['revision_log_message']['#access'] = FALSE;
   +    }
   

   Yes, please! ;)

7. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -0,0 +1,444 @@
   +   *   The value for source field of the the media item.
   

   Nit: "The value for the source field of the media item."

8. +++ b/core/modules/media_library/src/MediaLibraryUiBuilder.php
   @@ -216,6 +234,42 @@ protected function buildMediaTypeMenu(MediaLibraryState $state) {
   +    // media library with an new instance of the media add form. The form API
   

   s/an new/a new/

9. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -169,6 +169,18 @@ public function testWidgetAccess() {
   +    // Allow users with 'view media' permission to access the media library view
   +    // and controller.
   +    $this->grantPermissions($role, [
   +      'create type_three media',
   +    ]);
   

   Comment doesn't seem to match the permissions we're granting here.

Thanks again!
-Derek

FYI: I just saw @xjm complain in the JSON:API code review that she wanted an explanation why something is @internal, and what might go wrong if you try to use it. I can see her point. On the surface, it's not clear why I'm not supposed to extend this myself...

+1 for this. I suggest we say something "Media Library is an experimental module and its internal code may be subject to change in minor releases. External code should not instantiate or extend this class." And let's copy that warning to everything we've marked @internal. (Which should be just about everything, until we've at least fixed our must-have stable blockers.)

A few comment nitpicks.

1. +++ b/core/modules/media/src/Annotation/MediaSource.php
   @@ -57,6 +57,18 @@ class MediaSource extends Plugin {
   +   * An array of form classes keyed by ID. The ID does not have to be an actual
   +   * form ID and is used to get a specific form class from the media source. An
   +   * example of this would be the 'media_library_add' form used by the media
   +   * library module.
   

   I think we should rephrase this a bit. I propose something like this:

   "An array of form class names, keyed by ID. The ID represents the operation the form is used for."

   Maybe we should avoid mentioning Media Library here for now, since it's still experimental.

2. +++ b/core/modules/media_library/js/media_library.ui.es6.js
   @@ -15,6 +15,26 @@
   +    Object.values(response.mids).forEach(value => {
   

   For consistently, we should rename 'mids' to 'mediaIds'. (And make a similar change in the corresponding AJAX command class.)

3. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -0,0 +1,447 @@
   + * @internal
   + *   Media Library is an experimental module and its internal code may be
   + *   subject to change in minor releases. External code should not instantiate
   + *   or extend this class.
   

   Nice.

4. +++ b/core/modules/media_library/src/MediaLibraryUiBuilder.php
   @@ -77,11 +90,16 @@ public static function dialogOptions() {
   +   * @param \Drupal\media_library\MediaLibraryState|null $state
   +   *   The current state of the media library, derived from the current request.
   

   We should not have |null thing in the type; instead, the description should be prefixed with (optional).

5. +++ b/core/modules/media_library/src/MediaLibraryUiBuilder.php
   @@ -178,13 +197,15 @@ protected function buildMediaTypeMenu(MediaLibraryState $state) {
   +    // Get the state parameters but remove the wrapper format, AJAX form and
   +    // form rebuild parameters. These are internal parameters that should never
   +    // be part of the vertical tab links.
   

   Referring to a vertical tab link here is confusing. How about "These are internal parameters that can influence the response in undesirable ways" instead?

6. +++ b/core/modules/media_library/src/MediaLibraryUiBuilder.php
   @@ -178,13 +197,15 @@ protected function buildMediaTypeMenu(MediaLibraryState $state) {
   +    // Also add the 'media_library_content' parameter so the link can replace
   +    // only the updated content for the tab.
   

   s/the link can replace/the response will contain

I think this has undergone enough review, testing, and poking and prodding. We have other things we gotta do, and we gotta get to 'em; this unblocks two or three major must-have issues in our roadmap. Let's do it! RTBC on green.

There's a nice screencast of this feature with #3023801: Allow newly uploaded files to be deleted from the media library without saving them added on top.

However:

A.) The fact that, once a new media file is being added, all preexisting media seems to disappear, is confusing. Keeping the preexisting media at the bottom of the upload form seems no option, so we need to reassure the user, where he/she is:

1. The dialog title should switch to "Add files to media library."
2. The "m of n items selected." counter needs to disappear from the bottom. Finally, we're in a completely different context.
3. The "X" at the top IMO should lead back to the media library. Make sure it does.
4. Do we need a "cancel" button as well or is the "X" at the top enough?
5. Possibly the submit button should say something like "Save and return to media library," but not sure about that.

B.) Additional nitpick:
There is an afterglow after hovering a delete "X" button that is larger than the "X" even after the "delete" hover text disappeared, see 00:18 or 00:50 [edit: this belongs to the followup #3023801: Allow newly uploaded files to be deleted from the media library without saving them]

Sorry I have to reset this to Needs work... :/

I can see some of your points, and certainly this will get improved in followups. Apart from that, Media (as a relatively new Core module) might have a different standard for RTBC features than the (more settled) rest of Core does.

Fact is that as much as I'm welcoming the new shiny feature (and that's why I'm reviewing it), usability-wise it's half-baked IMHO, and I think the points I raised shouldn't be brushed away that easily. But in the end, if they are relevant, at some point someone else will probably come up with the same criticisms, otherwise not.

Setting back to "Needs review." RTBC is more than I could justify here, but I'm sure it's gonna be fine in the end.

this issue blocks a lot of the other work

Fair point, that's certainly the case, and indeed I didn't hit any regressions.

Probably you're right and - no matter what's missing - it's better we're getting this committed ASAP, so we're no longer piling up combined patches everywhere, given the impressive speed of development here! :)

Setting back to RTBC, as it was ahead of my UX review in #60.

I haven't reviewed the backend parts in detail but +1 RTBC for the frontend parts.

Reviewed as well. Discussed the form extension point with @seanB. I initially misunderstood that the media info altering would be on the type level (this requiring code changes when you manually create new media types on the UI), but it is indeed on the source level. So modules providing new sources can/should provide the file upload info as well.

That was the only concern I had, and also based on the rest of the reviews, this looks great. Thanks all! Let's work on the followups.