Add oEmbed support to the media library

The original mockups also included a way to go to the oembed form once the library was already open, which you can see here: https://preview.uxpin.com/c9905d865a57bed1fc72be93a4e937f126dac889#/page... Not sure if I like that design though, we may want to re-think how users would get to OEmbed once the modal was already open.

1. +++ b/core/modules/media_library/src/Form/MediaLibraryOEmbedForm.php
   @@ -0,0 +1,540 @@
   +   * Constructs a new MediaLibraryUploadForm.
   

   Doc needs update.

2. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -280,6 +280,23 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   +      '#access' => $this->addAccess && ($cardinality_unlimited || $remaining > 0),
   

   IIRC, addAccess is specifically about the ability to add new media that accept file/image sources, _not_ oembed.

1. +++ b/core/modules/media_library/src/Form/MediaLibraryOEmbedForm.php
   @@ -0,0 +1,540 @@
   +      $process = (array) $this->elementInfo->getInfoProperty('url', '#process', []);
   +      $form['url'] = [
   +        '#type' => 'url',
   +        '#title' => $this->t('URL'),
   +        // @todo Move validation in https://www.drupal.org/node/2988215
   +        '#process' => array_merge(['::validateUrlElement'], $process),
   +      ];
   

   🤔So the reason we used ElementInfo in the upload form was so that we could process uploads as soon as they were finished (skipping an extra mouse click for UX reasons) - but if this is just a URL field you should be able to do your validation in a normal validate method.

2. +++ b/core/modules/media_library/src/Form/MediaLibraryOEmbedForm.php
   @@ -0,0 +1,540 @@
   +    $form['#prefix'] = '<div id="media-library-upload-wrapper">';
   ...
   +    $form['#attributes']['class'][] = 'media-library-upload';
   

   +++ b/core/modules/media_library/src/Form/MediaLibraryOEmbedForm.php
   @@ -0,0 +1,540 @@
   +          'wrapper' => 'media-library-upload-wrapper',
   ...
   +              'media-library-upload__media',
   ...
   +                'media-library-upload__media-preview',
   ...
   +                'media-library-upload__media-fields',
   ...
   +          $element['fields'][$source_field]['#attributes']['class'][] = 'media-library-upload__source-field';
   

   +++ b/core/modules/media_library/src/Form/MediaLibraryOEmbedForm.php
   @@ -0,0 +1,540 @@
   +              'media-library-upload__file',
   ...
   +            '#markup' => '<strong class="media-library-upload__file-label">' . $this->t('Select a media type for %title:', [
   ...
   +              'wrapper' => 'media-library-upload-wrapper',
   ...
   +          'wrapper' => 'media-library-upload-wrapper',
   ...
   +   *   The upload element.
   

   These need updating.

3. +++ b/core/modules/media_library/src/Form/MediaLibraryOEmbedForm.php
   @@ -0,0 +1,540 @@
   +      $remaining = (int) $this->getRequest()->query->get('media_library_remaining');
   +      if ($remaining || $remaining === FieldStorageDefinitionInterface::CARDINALITY_UNLIMITED) {
   +        $form['upload']['#cardinality'] = $remaining;
   +      }
   

   Left over code, looks like. If you end up implementing a validate method you should check cardinality there.

Nice work! @phenaproxima++

Preliminary review, lots of chunks I don't really understand and would love to come back to a more in-depth look later.

1. +++ b/core/modules/media_library/src/Form/MediaLibraryFormBase.php
   @@ -0,0 +1,441 @@
   + * Creates a form to create media entities from uploaded files.
   

   Copy/paste leftover?

2. +++ b/core/modules/media_library/src/Form/MediaLibraryFormBase.php
   @@ -0,0 +1,441 @@
   + * @internal
   

   Admittedly contentious question: Does this really need to be internal? I could see value in opening this to custom/contrib to build upon.

3. +++ b/core/modules/media_library/src/Form/MediaLibraryFormBase.php
   @@ -0,0 +1,441 @@
   +   * Constructs a new MediaLibraryUploadForm.
   

   Copy/paste leftover?

4. +++ b/core/modules/media_library/src/Form/MediaLibraryFormBase.php
   @@ -0,0 +1,441 @@
   +    $form['#attributes']['class'][] = 'media-library-upload';
   

   Should we use a more neutral class if this is a base form?

5. +++ b/core/modules/media_library/src/Form/MediaLibraryFormBase.php
   @@ -0,0 +1,441 @@
   +        EntityFormDisplay::collectRenderDisplay($media, 'media_library')
   +          ->buildForm($media, $element['fields'], $form_state);
   

   I know this is out-of-scope for this ticket, but just wanted to know if I'm not missing something here: Are we preventing the media_library form display/form mode from being deleted, or alternatively, should this fall back to the default display in that case?

6. +++ b/core/modules/media_library/src/Form/MediaLibraryFormBase.php
   @@ -0,0 +1,441 @@
   +  /**
   +   * Prepares a media entity to be saved during form submission.
   +   *
   +   * @param \Drupal\media\MediaInterface $media
   +   *   The media entity.
   +   *
   +   * @return \Drupal\media\MediaInterface
   +   *   The media entity, ready to be saved.
   +   */
   +  protected function prepareMediaEntityForSave(MediaInterface $media) {
   

   Minor: would it be helpful to expand this docblock with examples of why this method can be useful for child classes? (at this point in reading the patch top-down, I have no idea yet what this method is for. I guess it will be clearer when I reach the child implementations, but I think the docblock should make that unnecessary, if possible)

7. +++ b/core/modules/media_library/src/Form/MediaLibraryFormBase.php
   @@ -0,0 +1,441 @@
   +   * AJAX callback to select a media type for a file.
   

   Copy/past leftover?

8. +++ b/core/modules/media_library/src/Form/MediaLibraryFormBase.php
   @@ -0,0 +1,441 @@
   +   * @param array $form
   +   *   An associative array containing the structure of the form.
   +   * @param \Drupal\Core\Form\FormStateInterface $form_state
   +   *   The current state of the form.
   

   Nitpick: wasn't there a thing where AJAX callbacks don't need param documentation anymore? or am I misremembering?

9. +++ b/core/modules/media_library/src/Form/MediaLibraryFormBase.php
   @@ -0,0 +1,441 @@
   +    $this->media[] = $this->createMediaEntity($this->unresolvedValues[$i], $this->getTypes()[$type]);
   

   (I may answer this myself as I keep reading, but) Is there any chance $this->unresolvedValues[$i] is unpopulated at this point?

10. +++ b/core/modules/media_library/src/Form/MediaLibraryFormBase.php
    @@ -0,0 +1,441 @@
    +   *   The input value.
    

    How about making this more explicit with something like:
    The input value. It should match what the source field on the given type expects as data type.

11. +++ b/core/modules/media_library/src/Form/MediaLibraryFormBase.php
    @@ -0,0 +1,441 @@
    +   * @param array $form
    +   *   An associative array containing the structure of the form.
    +   * @param \Drupal\Core\Form\FormStateInterface $form_state
    +   *   The current state of the form.
    

    Same as above re: documenting ajax callback parameters. Please disregard if I'm just being crazy.

12. +++ b/core/modules/media_library/src/Form/MediaLibraryFormBase.php
    @@ -0,0 +1,441 @@
    +   *   A command to send the selection to the current field widget.
    

    Nitpick: would "A command to send the selection to the current field widget and close the modal." be more accurate?

13. +++ b/core/modules/media_library/src/Form/MediaLibraryFormBase.php
    @@ -0,0 +1,441 @@
    +   * @param array $form
    +   *   The form render array.
    +   * @param \Drupal\Core\Form\FormStateInterface $form_state
    +   *   The form state.
    

    Same as above.

14. +++ b/core/modules/media_library/src/Form/MediaLibraryFormBase.php
    @@ -0,0 +1,441 @@
    +   * Access callback to check that the user can create file based media.
    

    Copy/paste leftover?

15. +++ b/core/modules/media_library/src/Form/MediaLibraryFormBase.php
    @@ -0,0 +1,441 @@
    +   * Returns media types which use files that the current user can create.
    

    Copy/paste leftover?

16. +++ b/core/modules/media_library/src/Form/MediaLibraryOEmbedForm.php
    @@ -0,0 +1,293 @@
    +      '#value' => $this->t('Update'),
    

    This label makes me scratch my head... Wouldn't "Next" or "Save" make more sense here?

17. +++ b/core/modules/media_library/src/Form/MediaLibraryOEmbedForm.php
    @@ -0,0 +1,293 @@
    +    return parent::buildInputForm($form, $form_state);
    

    Uneducated question: Is there a reason for calling the parent's method at the end? I think I have mostly seen the opposite, calling it at the beginning, and then adding stuff to it. Don't think it would make a difference here, but I'm curious about the reasoning. Thanks!

18. +++ b/core/modules/media_library/src/Form/MediaLibraryOEmbedForm.php
    @@ -0,0 +1,293 @@
    +  /**
    +   * Creates a new, unsaved media entity.
    +   *
    +   * @param \Drupal\media\OEmbed\Resource $resource
    +   *   An oEmbed resource.
    +   * @param \Drupal\media\MediaTypeInterface $type
    +   *   A media type.
    +   *
    +   * @return \Drupal\media\MediaInterface
    +   *   An unsaved media entity.
    +   */
    

    Shouldn't this be

    /**
     * {@inheritdoc}
     */
    

    instead ?

19. +++ b/core/modules/media_library/src/Form/MediaLibraryUploadForm.php
    @@ -104,223 +61,91 @@ public function getFormId() {
    +    return parent::buildInputForm($form, $form_state);
    

    Minor: Same question as above re: calling parent at the beginning vs at the end of the method.

20. +++ b/core/modules/media_library/src/Form/MediaLibraryUploadForm.php
    @@ -104,223 +61,91 @@ public function getFormId() {
    +    $file = $media->get($source_field)->entity;
    +    $file->setPermanent();
    +    $file->save();
    

    Is it worth wrapping these lines in checks such as
    !$media->get($source_field)->isEmpty()
    and
    $file instanceof FileInterface
    or am I being too paranoid and that would never happen?

21. +++ b/core/modules/media_library/src/Form/MediaLibraryUploadForm.php
    @@ -409,12 +234,14 @@ public function uploadButtonSubmit(array $form, FormStateInterface $form_state)
    +  protected function createMediaEntity($file, MediaTypeInterface $type) {
    

    Should the $file parameter be $value instead?

22. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -280,6 +280,23 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
    +      '#title' => $this->t('Add external media'),
    

    Bikeshedding time: Would "Add remote media" be slightly clearer?

I've also quickly tested it on simplytest.me, and it works quite well! Just two minor UI thoughts that came to my mind while testing:

and

And then about:

1. Discuss if it should be possible to only enter one embed code in the widget, or several. The widget does support multiple file uploads, but it's unclear if that same paradigm should be implemented for embed codes.

I would not bother with that. Users normally expect files to be "draggable and droppable" in bulk, because this is how most file-managing tools work. But that isn't the case with URL values, it isn't normal you go to a place to "collect a bunch of YouTube URLs and drop them somewhere". So IMHO creating oEmbed media items one by one is just fine.

I agree with #5. Adding a short video of the media library UX that seems to be widely regarded as "nice to work with", implemented by our friends in the Wordpress community. There are a lot of things I like about this, but for this specific issue, we should really look at the way they allow the user to switch between upload/URL.

Not sure if the dropbutton is the best way, but we could think of a way to switch between the add forms for each media type.

Hopefully I will get some time at a later moment to dive more into the code.

I don’t like that it ends up always adding the “Add external media” button. Is there a way to enable/disable oEmbed in media_library?

From reviewing the latest patch I don't think all the feedback in #7 or #6 has been addressed. For instance, access to the oembed button is still determined by add access to media types that use the file/image source. Fixing this would address #14

I had some time to install the patch and look at the code. The problem (I think) we are trying to solve in this issue is adding new oEmbed based media from the media library widget. Since core introduces the concept of media sources that can have all kinds of source fields, I feel we should try and think of a generic solution that could work source field independent. There are different ways to solve this problem, and I'm not yet convinced the current solution with a very specific source input is the best one. It is also not in line with the UX / Design work shown in #2834729-35: [META] Roadmap to stabilize Media Library (which seems a lot more flexible).

So what would I like to see changed/improved:

1. When looking at the widget, I see 3 buttons: 'Browse media' / 'Add media' / 'Add external media'. Most of the users know they want to add a youtube video. It is not directly clear which button they should pick, it all depends on whether the video was already uploaded or not, what does external actually mean, etc? I think a better solution would be to always show 1 button 'Add media'. Don't make me think.
2. When opening the modal, you see the media library. Users can check if the media item is already present, and if not the top left should show a dropbutton to add new media with all available media types for the widget: 'Add image' / 'Add video' / 'Add remote video' etc.
3. Selecting the 'Add [media-type]' button in the library opens the media add form for that specific media type. This is a generic solution that works for all media types/providers we can think of in the future, and I think this also requires a lot less code than we currently added for the generic upload/oEmbed widgets.
4. In the solution described above we are missing the 'add multiple' feature. We could have a screen before step 3 that shows 1 or more source fields for the chosen type.

I am aware this has quite some impact on the proposed patch and also some of the code that was already added in the media library.
I feel the UX part of the media library is not yet clear on all the details and that certain points of the created mock-ups are incompatible with the level of flexibility we introduced by having different media sources/media types. Before we continue with the implementation of complex solutions such as the generic upload/oEmbed widgets, I think it would be good to re-evaluate whether the existing designs still accommodate all our needs and make sure we have a clear and detailed picture of what the end result should look like to avoid future bikeshedding in this issue and issues to come.

I would only like to add that:

• remote URLs or files can be supported by multiple media types
• Media types are fieldable, which means that just a file/URL is not always enough to save a media item

Choosing the right type and filling in possible required fields are 2 extra steps that impact to overall flow of adding media. When looking at the video of the UX meeting (sorry I missed it by the way), I think the step of choosing the correct media type was not part of the demo. Just dropping this here to make sure this is not overlooked.

A new approach to this was discussed while creating the designs in #3019150: Update/improve mockups and designs for the media library.

Attached is a full patch built on top of #3023802-2: Show media add form directly on media type tab in media library (which is built on #3020707: Streamline buttons in the media library widget and #3020716: Add vertical tabs style menu to media library).
Also a patch with just the changes for this issue.

Obviously, this is blocked on the other issues for now.

Reroll since a lot has changed since the last patch. #3020707: Streamline buttons in the media library widget and #3020716: Add vertical tabs style menu to media library have been committed.

Based the current patch on #3023802-34: Show media add form directly on media type tab in media library.

+++ b/core/modules/media_library/src/Form/OEmbedForm.php
@@ -0,0 +1,144 @@
+    $this->updateFormState([$form_state->getValue('url')], $form, $form_state);

Note to self: This was renamed and you forgot to update that (D'oh!).

Fixed the tests and some minor changes in the tests and CSS.

Whoops, forgot to update that new test.

Screenshots and a video added to the IS.

D'oh, wrong video.

New patch attached to reroll and fix #28.

1. That class is only available for an actual media item form. This is targeting the custom source field from OEmbedForm.
2. Fixed.
3. Fixed. After added the description needed to add extra HTML/CSS to make the form align nicely.
4. Fixed.
5. Fixed.
6. Fixed.
7. That would not match the Youtube regex, so it will return 'No matching provider found.'
8. Fixed.
9. I added a protected method to remove the duplication instead.

Fixed 2 small issues. The string 'https://' does not need to be translatable. And the submit button does not need a placeholder (copy/paste mistake).

Turns out, I was wrong. The FileUploadForm was doing its own thing to add a default name. Moved that to AddFormBase to let the media entity provide a default name for every form extending it.

Added back the tests that were removed in #29 for the custom name.

Added a comment and an extra assert for the name field default value. The upload form apparently already had an assert for that.

As requested by @phenaproxima let's use $assert_session->fieldValueEquals() consistently and remove the second assert doing the same thing.

1. +++ b/core/modules/media_library/css/media_library.theme.css
   @@ -83,19 +83,50 @@
   +.media-library-add-form-upload.media-library-add-form--without-input .form-item-upload {
   

   This should be .media-library-add-form--without-input .form-item-upload.

2. +++ b/core/modules/media_library/css/media_library.theme.css
   @@ -83,19 +83,50 @@
   +.media-library-add-form-oembed-submit.button {
   

   This should be just .media-library-add-form-oembed-submit.

3. +++ b/core/modules/media_library/css/media_library.theme.css
   @@ -83,19 +83,50 @@
   +.media-library-add-form-oembed.media-library-add-form--without-input .form-item-url {
   

   This selector should be .media-library-add-form--without-input .form-item-url.

@lauri

1. I thought about that, but if contrib modules add custom forms with an upload field they might not want the same styles applied. What we add here is specific for the core upload form.
2. That is not specific enough, the .button style from seven overrides the styles set by the media library.
3. Same rationale as 1.

Just had a quick chat with lauriii. The problem for 1. and 3. mentioned in #40 is actually that the classes media-library-add-form-upload and media-library-add-form-oembed are wrong.
Since these are modifiers of media-library-add-form it should be media-library-add-form--upload and media-library-add-form--oembed

For 2. we need a @todo linking to https://www.drupal.org/project/drupal/issues/2980769 since the extra .button selector can probably be removed when the styles are moved to the seven theme.

Changed the classnames and added a todo.

Back to RTBC since feedback from #40 has been addressed. ✌️

#39 is a bit vague - were you thinking about anything in particular when you said it may need an accessibility review? Like, did you have a question in mind? I don't know what you wanted me to look at.

I don't know how that changed.

Looking at the IS' remaining tasks:

• There is a patch with tests, marking remaining task #2 done.
• Task #4 is: Pass security review (oEmbed always deserves a second look from a security standpoint).. All of the necessary infrastructure for this was added in #2831944: Implement media source plugin for remote video via oEmbed. This is just providing a UI to use it. We extensively evaluated security implications there (48 occurrences of the keyword "security" on #2831944!).
• I looked at #2831944 again, and verified that indeed oEmbedded media are rendered through an <iframe>. I don't see any particular security risks here.

Patch review

1. +++ b/core/modules/media_library/css/media_library.theme.css
   @@ -303,8 +335,8 @@
    .media-library-add-form__media:last-child {
   -  border-bottom: 0;
      padding-bottom: 0;
   +  border-bottom: 0;
   

   🔍🐛 This seems like an unnecessary change?

2. +++ b/core/modules/media_library/media_library.module
   @@ -48,6 +49,7 @@ function media_library_media_source_info_alter(array &$sources) {
   +  $sources['oembed:video']['forms']['media_library_add'] = OEmbedForm::class;
   

   🤔 Why oembed:video and not just oembed?

3. +++ b/core/modules/media_library/src/Form/FileUploadForm.php
   @@ -93,7 +93,7 @@ protected function getMediaType(FormStateInterface $form_state) {
   -    $form['#attributes']['class'][] = 'media-library-add-form-upload';
   +    $form['#attributes']['class'][] = 'media-library-add-form--upload';
   

   👍 🤔 Isn't this a BC break? No, it isn't, see #42.

4. +++ b/core/modules/media_library/src/Form/OEmbedForm.php
   @@ -0,0 +1,165 @@
   +    $form['#attributes']['class'][] = 'media-library-add-form--oembed';
   ...
   +        'class' => ['media-library-add-form-oembed-wrapper']
   

   🔍🐛 These are not consistent.

   I think the wrapper class should actually be media-library-add-form--oembed-wrapper?

5. +++ b/core/modules/media_library/src/Form/OEmbedForm.php
   @@ -0,0 +1,165 @@
   +      // @todo Move validation in https://www.drupal.org/node/2988215
   

   👍 🤔 Why is it okay to add more TODOs? Because \Drupal\media_library\Form\MediaLibraryUploadForm::buildForm() already has the same TODO, and this is a sibling form.

6. +++ b/core/modules/media_library/src/Form/OEmbedForm.php
   @@ -0,0 +1,165 @@
   +        'class' => ['media-library-add-form-oembed-submit']
   

   🔍🐛 Same here.

7. +++ b/core/modules/media_library/src/Form/OEmbedForm.php
   @@ -0,0 +1,165 @@
   +  public function validateUrl(array &$form, FormStateInterface $form_state) {
   +    $url = $form_state->getValue('url');
   +    if ($url) {
   +      try {
   +        $resource_url = $this->urlResolver->getResourceUrl($url);
   +        $this->resourceFetcher->fetchResource($resource_url);
   +      }
   +      catch (ResourceException $e) {
   +        $form_state->setErrorByName('url', $e->getMessage());
   +      }
   +    }
   +  }
   

   👍 Elegant!

8. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -28,6 +31,7 @@ class MediaLibraryTest extends WebDriverTestBase {
   +    $this->lockHttpClientToFixtures();
   

   🤔 👍 Having worked a lot with Guzzle on the REST and JSON:API tests, this caught my attention. This method already exists in HEAD. Also elegant, well done!

9. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -752,4 +756,103 @@ public function testWidgetUpload() {
   +    $video_title = "Everyday I'm Drupalin' Drupal Rap (Rick Ross - Hustlin)";
   

   🥳

10. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
    @@ -752,4 +756,103 @@ public function testWidgetUpload() {
    +    $video_url = 'https://www.youtube.com/watch?v=PWjcqE3QKBg';
    ...
    +    // Assert we can not add a video ID that doesn't exist. We need to use a
    +    // video ID that will not be filtered by the regex, because otherwise the
    +    // message 'No matching provider found.' will be returned.
    +    $page->fillField('Add Type Five via URL', 'https://www.youtube.com/watch?v=PWjcqE3QKBg1');
    

    🤔 The "nonexistent video ID" is the same one as the existing one, just with a "1" appended. Perhaps it's better to do $video_url . '1'? I don't feel strongly about it, but it wasn't immediately obvious to me why this would've been invalid. Probably not worth changing. But at least a next reviewer won't have to figure this out now :)

1. CSS comb fixed that. Do we really need a followup for this?
2. That is the plugin name.
3. :)
4. media-library-add-form--oembed is a modifier of media-library-add-form. The class media-library-add-form--oembed-wrapper is not a modifier, so thats why it shouldnt have --.
5. :)
6. See 4.
7. Yay!
8. Yay!
9. :)
10. Yeah it took a while to figure out that video IDs that match the regex get different errors. Hopefully the comment explains that. It shouldn't really matter that it's the same as the original youtube URL with a 1 added (I think).

That test fail was unrelated? Ran the test again and it's green now.
https://www.drupal.org/pift-ci-job/1207569

FILE: ...rupal/commits/core/modules/media_library/src/Form/OEmbedForm.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 3 WARNINGS AFFECTING 3 LINES
----------------------------------------------------------------------
95 | WARNING | [x] A comma should follow the last multiline array
| | item. Found: ]
110 | WARNING | [x] A comma should follow the last multiline array
| | item. Found: ]
126 | WARNING | [x] A comma should follow the last multiline array
| | item. Found: ]

Sorry about that! Fixed.

Added newline in CSS before @media rule.

Forgot to merge 8.7.x in my branch. Sorry.

Superb, thanks all! Thanks for fixing the remaining css issue we chatted about on slack.