Delegate media library selection handling to the "thing" that opened the library

I love this architecture.

1. +++ b/core/modules/media_library/src/Event/MediaLibraryEvents.php
   @@ -27,4 +27,21 @@ final class MediaLibraryEvents {
   +  /**
   +   * Name of the event fired when a user selected items in the media library.
   +   *
   +   * This event allows modules to handle the selected items in the media library
   +   * based on the media library state. Example use cases that require different
   +   * handling of selected media items:
   +   * - when used in an entity reference field widget;
   +   * - when used in CKEditor.
   +   *
   +   * @Event
   +   *
   +   * @see \Drupal\media_library\Event\MediaLibrarySelectionEvent
   +   *
   +   * @var string
   +   */
   +  const SELECTION = 'media_library.selection';
   

   I think we should differentiate that this is not actually fired when someone "selects something" in the media library -- that implies that this event is fired if the user clicks on an item in the media library to select it, which is not the case. Maybe we should rephrase some of this comment to explain that this is fired once the user has finished interacting with the media library and has finished the selection process. To wit, perhaps SELECTION_COMPLETE might be a better name for the event? Not sure...

2. +++ b/core/modules/media_library/src/Event/MediaLibrarySelectionEvent.php
   @@ -0,0 +1,95 @@
   +  /**
   +   * Sets the AJAX response.
   +   *
   +   * @param \Drupal\Core\Ajax\AjaxResponse $response
   +   *   The AJAX response.
   +   */
   +  public function setResponse(AjaxResponse $response) {
   +    $this->response = $response;
   +  }
   

   I'm not sure we should be allowing people to completely overwrite the response here. Maybe it would be wiser to only allow event subscribers to add AJAX commands? (i.e., $event->getResponse()->addCommand(), and remove setResponse() entirely)

3. +++ b/core/modules/media_library/src/EventSubscriber/MediaLibraryWidgetSubscriber.php
   @@ -85,6 +87,35 @@ public function onMediaLibraryAccess(MediaLibraryAccessEvent $event) {
   +   * @throws \Exception
   +   */
   +  public function onMediaLibrarySelection(MediaLibrarySelectionEvent $event) {
   

   This method does not throw \Exception, so we don't need the @throws. :)

4. +++ b/core/modules/media_library/src/EventSubscriber/MediaLibraryWidgetSubscriber.php
   @@ -85,6 +87,35 @@ public function onMediaLibraryAccess(MediaLibraryAccessEvent $event) {
   +    $state = $event->getState();
   +
   +    // We only want to respond to opener IDs of media library entity reference
   +    // widgets.
   +    $widget_id = MediaLibraryWidget::getOpenerWidgetId($state->getOpenerId());
   

   Nit: We don't really need $state as its own variable here. We can just call $event->getState()->getOpenerId().

5. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -64,11 +73,14 @@ abstract class AddFormBase extends FormBase {
   +    $this->eventDispatcher = $event_dispatcher;
   

   Well, this sucks, but because we are beta experimental I think we need to have the $event_dispatcher parameter default to NULL, and then throw a deprecation notice if it's not passed.

Rebased #2's do-not-test patch on top of @seanB's #3038254-31: Delegate media library access to the "thing" that opened the library. Did not address anything in #3.

To be able to provide truly constructive feedback rooted in real-world needs, I first worked on #2998005 to see if I could make this work against D8.8 HEAD (without either this patch applied or #3038254: Delegate media library access to the "thing" that opened the library).

The answer is: yes, I could — see #2998005-15: [PP-1] Support Drupal core's Media Library. The latest iteration is a bit simpler: #2998005-24: [PP-1] Support Drupal core's Media Library. The relevant code: entity_embed_form_views_form_media_library_widget_alter() + entity_embed_form_views_form_media_library_widget_ajax_update().

It overrides the #ajax submit callback; it changes it from \Drupal\media_library\Plugin\views\field\MediaLibrarySelectForm::updateWidget() to entity_embed_form_views_form_media_library_widget_ajax_update().

While I was able to make it work, it also happened to confirm the need for infrastructure like the one being proposed here. It'd mean I won't have to have a form alter hook that detects which "opener" it is anymore and then swap out the #ajax submit callback. It'd mean I have an official, supported API, rather than having to reach into implementation details. That being said, we have code all over Drupal core and contrib relying on particular form structures. So I think it would be acceptable to not have the infrastructure this patch is adding.

1. +++ b/core/modules/media_library/src/Event/MediaLibrarySelectionEvent.php
   @@ -0,0 +1,95 @@
   +   * @return \Drupal\Core\Ajax\AjaxResponse
   +   *   The AJAX response.
   ...
   +   * @param \Drupal\Core\Ajax\AjaxResponse $response
   +   *   The AJAX response.
   

   Why must this be an AJAX response? Isn't this limiting us unnecessarily for the future? Shouldn't this be up to the event subscriber to decide?

2. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -724,20 +737,21 @@ public function updateWidget(array &$form, FormStateInterface $form_state) {
   +    // Return the AJAX response to handle the selection and close the media
   +    // library dialog.
   +    return $selection_event->getResponse()
   +      ->addCommand(new CloseDialogCommand());
   
   +++ b/core/modules/media_library/src/Plugin/views/field/MediaLibrarySelectForm.php
   @@ -127,21 +127,16 @@ public function viewsForm(array &$form, FormStateInterface $form_state) {
   +    // Return the AJAX response to handle the selection and close the media
   +    // library dialog.
   +    return $selection_event->getResponse()
   +      ->addCommand(new CloseDialogCommand());
   

   Note that the CloseDialogCommand being always present means that for the WYSIWYG (text editor) integration, it'll have to override the pre-generated AjaxResponse completely, because \Drupal\editor\Ajax\EditorDialogSave MUST run before CloseDialogCommand.

   EDIT: never mind, \Drupal\Core\Ajax\AjaxResponse::addCommand() has an optional $prepend = FALSE parameter; that'll allow the event subscriber for the WYSIWYG (text editor) integration to prepend its \Drupal\editor\Ajax\EditorDialogSave command 👍

I didn't want to end at #5, I wanted to 100% vet it. So I went ahead and created an alternative
#2998005 patch that builds on top of #3038254-31: Delegate media library access to the "thing" that opened the library and #3044649-4: Delegate media library selection handling to the "thing" that opened the library.

It'd work beautifully, except that we would unfortunately still need entity_embed_form_views_form_media_library_widget_alter() to attach the editor/drupal.editor.dialog asset library. That's the sole place where we need to go beyond the infrastructure that #3038254 and #3044649 (this issue) provide. That asset library is necessary to make EditorDialogSave AJAX commands work.

See #2998005-28: [PP-1] Support Drupal core's Media Library.

+1 to both; although the CloseDialogCommand remark is kinda moot.

Actually … the fact that the dialog needs to be closed is a valid reason for it to be required to use an AjaxResponse. So it might be better to leave it as-is :)

I discussed this with @effulgentsia today.

He thinks (and I agree) that adding entity-specific stuff to MediaLibraryState, as we are doing in #3038254: Delegate media library access to the "thing" that opened the library, is a strange code smell. It's strange enough that it will likely hold up that issue for a bit.

However, the current patch in this issue doesn't seem to need any entity-specific stuff at all. Which means that this patch does not need to be blocked by that patch.

So, let's flip the order. Let's add the event subscriber-based architecture in this issue and commit it first, then reroll #3038254: Delegate media library access to the "thing" that opened the library on top of that work. I validated this idea with @effulgentsia and he is for it. So let's get it done!

Can you explain that concern some more? Is the concern that this prevents non-entity things from using the Media Library selection UI?

P.S.: why is that on this issue instead of on #3038254: Delegate media library access to the "thing" that opened the library?

Nice work!

1. +++ b/core/modules/media_library/src/Event/MediaLibrarySelectionEvent.php
   @@ -0,0 +1,95 @@
   +  /**
   +   * Sets the AJAX response.
   +   *
   +   * @param \Drupal\Core\Ajax\AjaxResponse $response
   +   *   The AJAX response.
   +   */
   +  public function setResponse(AjaxResponse $response) {
   +    $this->response = $response;
   +  }
   

   I don't think we need to allow people to override the response entirely. Calling code should be able to just do $event->getResponse()->addCommand().

2. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -64,11 +73,14 @@ abstract class AddFormBase extends FormBase {
   +  public function __construct(EntityTypeManagerInterface $entity_type_manager, MediaLibraryUiBuilder $library_ui_builder, EventDispatcherInterface $event_dispatcher) {
        $this->entityTypeManager = $entity_type_manager;
        $this->libraryUiBuilder = $library_ui_builder;
        $this->viewBuilder = $this->entityTypeManager->getViewBuilder('media');
   +    $this->eventDispatcher = $event_dispatcher;
   

   I think we are going to need to default the $event_dispatcher to NULL and default it to \Drupal::service('event_dispatcher') (and trigger a deprecation notice) if it's not passed.

3. +++ b/core/modules/media_library/src/Form/FileUploadForm.php
   @@ -66,8 +69,8 @@ class FileUploadForm extends AddFormBase {
   +  public function __construct(EntityTypeManagerInterface $entity_type_manager, MediaLibraryUiBuilder $library_ui_builder, EventDispatcherInterface $event_dispatcher, ElementInfoManagerInterface $element_info, RendererInterface $renderer, FileSystemInterface $file_system) {
   +    parent::__construct($entity_type_manager, $library_ui_builder, $event_dispatcher);
   

   Since the $event_dispatcher parameter is new, it should be the last one, and default to NULL.

4. +++ b/core/modules/media_library/src/Form/OEmbedForm.php
   @@ -44,13 +45,15 @@ class OEmbedForm extends AddFormBase {
   +  public function __construct(EntityTypeManagerInterface $entity_type_manager, MediaLibraryUiBuilder $library_ui_builder, EventDispatcherInterface $event_dispatcher, UrlResolverInterface $url_resolver, ResourceFetcherInterface $resource_fetcher) {
   +    parent::__construct($entity_type_manager, $library_ui_builder, $event_dispatcher);
   

   Same here.

5. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -67,11 +67,25 @@ class MediaLibraryWidget extends WidgetBase implements ContainerFactoryPluginInt
   +  /**
   +   * The prefix to use with a widget ID for media library opener IDs.
   +   *
   +   * @var string
   +   */
   +  protected static $openerIdPrefix = 'widget:';
   +
   +  /**
   +   * The separator to use for widget IDs separating the field ID and the suffix.
       *
       * @var string
       */
   -  protected static $openerIdPrefix = 'field:';
   +  protected static $widgetIdSeparator = ':';
   

   I'm not sure if we really need these protected static members. They're completely internal implementation details of public methods, so maybe we should just hard-code them.

6. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -447,9 +471,11 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   +    // Create a unique opener ID with a fixed prefix for entity reference field
   +    // widgets. The opener ID could be used to check access to the media library
   +    // and used by the media library select/add forms to add the selected/added
   +    // media items to the widget.
   +    $opener_id = static::$openerIdPrefix . $field_widget_id;
   

   This comment should not mention access checking, since we're not adding that in this patch. :)

7. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -834,19 +860,43 @@ protected static function setFieldState(array $element, FormStateInterface $form
   +   * @see \Drupal\media_library\MediaLibraryState
   +   */
   +  public static function getFieldNameFromOpenerId($opener_id) {
   

   Why do we need the @see?

Although I do realize this patch has lots of implicit test coverage (all Media Library tests would break if this did not work), I think we should add some explicit test coverage as well, just to prove that the events are properly dispatched and handled. (I think a basic kernel test, with an accompanying test module, oughta do the trick.)

+++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
@@ -893,10 +877,10 @@
+    // Media library widget IDs are always prefixed with 'widget:' property in

s/property//

Apart from doc comment updates I have asked @seanB for in person, I think I'm out of things to complain about.

Looks great! I'd like a second opinion from Wim before we officially greenify this, but it's RTBC from my perspective.

I'm tagging this for backport to 8.7.x. I'm not sure this is technically a feature request; it is really just refactoring internal Media Library mechanisms. And although it's not a bugfix, it would allow Entity Embed to integrate with Media Library before 8.8, which is advantageous since we are intending to move parts of that module (including its integration with Media Library) into core. (See #2998005: [PP-1] Support Drupal core's Media Library).

Looks like it needs a reroll =\

Removing the "Needs reroll" tag.

1. +++ b/core/modules/media_library/src/Event/MediaLibraryEvents.php
   @@ -0,0 +1,29 @@
   + * @see \Drupal\media_library\Event\MediaLibraryAccessEvent
   

   s/Access/Selection/ :)

2. +++ b/core/modules/media_library/src/Event/MediaLibraryEvents.php
   @@ -0,0 +1,29 @@
   +   * - when used in CKEditor.
   

   s/CKEditor/a text editor/

3. +++ b/core/modules/media_library/src/Event/MediaLibrarySelectionEvent.php
   @@ -0,0 +1,85 @@
   +   * The selected media IDs.
   ...
   +   * @var array
   ...
   +   * @param int[] $selected_ids
   ...
   +   * @return int[]
   

   Inconsistent.

4. +++ b/core/modules/media_library/src/EventSubscriber/MediaLibraryWidgetSubscriber.php
   @@ -0,0 +1,52 @@
   +   * @throws \Exception
   

   No exception is thrown; should be removed.

5. +++ b/core/modules/media_library/src/EventSubscriber/MediaLibraryWidgetSubscriber.php
   @@ -0,0 +1,52 @@
   +    // Create a comma separated list of media IDs, insert them in the hidden
   

   Nit: s/comma separated/comma-separated/

6. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -64,11 +73,18 @@ abstract class AddFormBase extends FormBase {
   +   * @param \Symfony\Component\EventDispatcher\EventDispatcherInterface $event_dispatcher
   

   Nit: typehint inaccurate.

7. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -247,7 +264,7 @@ protected function buildEntityFormElement(MediaInterface $media, array $form, Fo
   -    $parents = $form['#parents'];
   +    $parents = isset($form['#parents']) ? $form['#parents'] : [];
   

   Noting that @phenaproxima asked about this in #3038254-23: Delegate media library access to the "thing" that opened the library.6 and @seanB answered this in the next comment.

Rebased #2998005 on top of #27, and #2998005-41: [PP-1] Support Drupal core's Media Library still works perfectly fine 👍 I wanted that extra piece of validation before RTBC'ing :)

I'm going to talk to @Wim Leers, @seanB and @phenaproxima about this issue at devdays cluj because I have some concerns about the architecture. I'm not sure that an event is really the right way to go but we'll hash that out and I'll make a further comment here about the resulting discussion.

@alexpott expressed concerns about using events for solving this issue, since the expectation for events is that multiple subscribers can chime in. In this case, only a single thing is supposed to have a say in this.

Creating a new plugin type seems overkill, tagged services are also overkill. What seems appropriate in this case is to let the specific media library opener nominate a particular service to generate a response upon selection (and the same service could also handle access checking, see #3038254: Delegate media library access to the "thing" that opened the library).

@seanB, @phenaproxima and I pair-programmed this together at Drupal Dev Days.

Nitpicks!

1. +++ b/core/modules/media_library/media_library.services.yml
   @@ -6,3 +6,5 @@ services:
   +  media_library.field_widget:
   +    class: Drupal\media_library\MediaLibraryFieldWidgetOpener
   

   I think we should rename this to media_library.opener.field_widget.

2. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -64,11 +71,18 @@ abstract class AddFormBase extends FormBase {
   +   * @param \Drupal\Core\DependencyInjection\ClassResolverInterface|null $class_resolver
   +   *   (optional) The class resolver.
   

   Nit: I don't think this should be type hinted with |null, or marked optional. It's not. It's only optional for backwards compatibility reasons, and the parameter documentation should reflect the intention.

3. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -724,20 +739,21 @@ public function updateWidget(array &$form, FormStateInterface $form_state) {
   +    $response = $this->classResolver->getInstanceFromDefinition($service_id)->respondToSelection(
   +      $current_media_ids,
   +      $media_library_state
   +    );
   +    return $response->addCommand(new CloseDialogCommand());
   

   Nit: This can all be one long chained call.

4. +++ b/core/modules/media_library/src/Form/FileUploadForm.php
   @@ -65,9 +66,11 @@ class FileUploadForm extends AddFormBase {
   +   * @param \Drupal\Core\DependencyInjection\ClassResolverInterface|null $class_resolver
   +   *   (optional) The class resolver.
   

   Same complaint as above.

5. +++ b/core/modules/media_library/src/Form/OEmbedForm.php
   @@ -48,9 +49,11 @@ class OEmbedForm extends AddFormBase {
   +   * @param \Drupal\Core\DependencyInjection\ClassResolverInterface|null $class_resolver
   +   *   (optional) The class resolver.
   

   Ditto.

6. +++ b/core/modules/media_library/src/MediaLibraryFieldWidgetOpener.php
   @@ -0,0 +1,43 @@
   +    throw new \Exception('not yet implemented');
   

   Might want a better method? Also, we should add a @see comment referring to the access delegation issue.

7. +++ b/core/modules/media_library/src/MediaLibraryFieldWidgetOpener.php
   @@ -0,0 +1,43 @@
   +    if (!$widget_id) {
   +      return new \UnexpectedValueException();
   +    }
   

   This should probably be \InvalidArgumentException, and needs a message.

8. +++ b/core/modules/media_library/src/Plugin/views/field/MediaLibrarySelectForm.php
   @@ -127,21 +124,16 @@ public function viewsForm(array &$form, FormStateInterface $form_state) {
   +    $response = \Drupal::service($service_id)->respondToSelection(
   +      $selected_ids,
   +      $media_library_state
   +    );
   +    return $response->addCommand(new CloseDialogCommand());
   

   Nit: This can be one chained call.

#31:

1. ✅
2. Are you sure? Other places in core do this too for required-but-optional-because-BC reasons. I personally agree with you (and that's what I did originally), but core doesn't seem to agree with us :)
3. ✅
4. See 2 :).
5. Ditto.
6. ✅
7. ✅
8. ✅

Re #32.2 core is pretty confused on constructor argument change. @Berdir and I have talked about writing a docs page / policy issue for a while but we've never got round to it. I agree with @phenaproxima here let's document the correct usage not the deprecated usage.

Regarding hashing: let's add two new methods to MediaLibraryState: getOpenerParameter() and setOpenerParameter(). When invoked, they get or set values in a media_library_opener_parameters sub-array, which could be automatically serialized and included in the output of getHash(), and would be represented this way in the generated query strings:

/path/to/page?media_library_opener_id=whatever&media_library_opener_parameters[foo]=bar&media_library_parameters[baz]=wambooli

This would allow openers to easily get and set parameters which should be tamper-proofed by the hash.

Here's an implementation of that idea. We'll need some tests to ensure that the new methods work, and that any values therein are taken into account by getHash().

This fixes MediaLibraryTest on my machine. I fixed some incorrect method calls, and made sure the opener parameters are handled in a consistent way. Still needs explicit tests, though.

Improved documentation on MediaLibraryState.

A few more small documentation changes, and added explicit tests of the opener parameter methods on MediaLibraryState. I don't think the explicit test coverage needs to be particularly extensive; with the changes we're making in this issue, the functional tests will break hard if the opener parameters are buggy.

So, in re-reading #41, I realized that setOpenerParameter() is not necessary. Since MediaLibraryState::create() can take an array of opener-specific parameters, there is no need to introduce this additional mutability. So I removed it :)

#34: 👍

Patch review:

1. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -64,11 +71,18 @@ abstract class AddFormBase extends FormBase {
   +   * @param \Drupal\Core\DependencyInjection\ClassResolverInterface|null $class_resolver
   +   *   (optional) The class resolver.
   
   +++ b/core/modules/media_library/src/Form/FileUploadForm.php
   @@ -65,9 +66,11 @@ class FileUploadForm extends AddFormBase {
   +   * @param \Drupal\Core\DependencyInjection\ClassResolverInterface|null $class_resolver
   +   *   (optional) The class resolver.
   
   +++ b/core/modules/media_library/src/Form/OEmbedForm.php
   @@ -48,9 +49,11 @@ class OEmbedForm extends AddFormBase {
   +   * @param \Drupal\Core\DependencyInjection\ClassResolverInterface|null $class_resolver
   +   *   (optional) The class resolver.
   

   Nit: per #34, (optional) and |null should be removed.

2. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -724,20 +739,19 @@ public function updateWidget(array &$form, FormStateInterface $form_state) {
   +    // Allow the opener service to handle the selection.
   

   Nit: "handle" is pretty vague. Could be more specific.

3. +++ b/core/modules/media_library/src/MediaLibraryOpenerInterface.php
   @@ -0,0 +1,53 @@
   + * Openers that require special parameters or metadata should retrieve them from
   

   "special" sounds … scarily artsy. We want precision. So let's change it to "additional".

4. +++ b/core/modules/media_library/src/MediaLibraryState.php
   @@ -12,14 +12,13 @@
   + * - media_library_opener_id: The ID of a container service which implements
   

   Nit: do we have non-container services? 😛 Let's omit "container" here.

   EDIT: oh wow we apparently do this all over core. Weird. Never mind then!

5. +++ b/core/modules/media_library/src/MediaLibraryState.php
   @@ -31,6 +30,13 @@
   + * This object can also carry an optional array of arbitrary values, under the
   + * media_library_opener_parameters key. These values are included in the hash
   + * generated by ::getHash(), but not otherwise validated by this object. They
   + * may be validated or used in specialized ways by specific opener services.
   

   This wording really confused me. Clarified.

6. +++ b/core/modules/media_library/src/Plugin/views/field/MediaLibrarySelectForm.php
   @@ -53,10 +51,6 @@ public function viewsForm(array &$form, FormStateInterface $form_state) {
   -    // Add the view to the form state so the opener ID can be fetched from the
   -    // view request object in ::updateWidget().
   -    $form_state->set('view', $this->view);
   
   @@ -120,27 +114,23 @@ public function viewsForm(array &$form, FormStateInterface $form_state) {
   +   * @param \Symfony\Component\HttpFoundation\Request $request
   +   *   The current request.
   

   This seems like out-of-scope refactoring, but it's both minimal and makes things simpler, so I'm fine with keeping this in this patch.

So, basically only nits. Thanks for getting it to green.

Thanks all!

All feedback is fixed. With regard to #43.3, the answer is no, we don't need both; there is a hard technical reason why we need the plural getOpenerParameters() in \Drupal\media_library\MediaLibraryUiBuilder::buildMediaTypeMenu(), but the singular getOpenerParameter() is a convenience method. So I removed that with @seanB's blessing.

• Previous RTBC patch (#27): 9 files changed, 278 insertions, 56 deletions.
• Current patch (#45): 11 files changed, 225 insertions, 102 deletions.

So:

1. patch with an architecture that results in a narrower scope/API surface, per @alexpott's suggestions (yay!)
2. fewer insertions, more deletions (yay!)

1. This look much much nicer than the event system and cleaner too. Thank you for changing the solution.

2. +++ b/core/modules/media_library/src/MediaLibraryOpenerInterface.php
   @@ -0,0 +1,53 @@
   + * Openers that require additional parameters or metadata should retrieve them
   + * from the MediaLibraryState object.
   

   Add an @see to the MediaLibraryState

3. +++ b/core/modules/media_library/src/MediaLibraryOpenerInterface.php
   @@ -0,0 +1,53 @@
   +   *   The user for which to check access, or NULL to check access for the
   +   *   current user. Defaults to NULL.
   

   If the NULL ness is true that we need = NULL on the method.

4. +++ b/core/modules/media_library/src/MediaLibraryOpenerInterface.php
   @@ -0,0 +1,53 @@
   +  public function checkAccess(MediaLibraryState $state, AccountInterface $account);
   ...
   +  public function getSelectionResponse(array $selected_ids, MediaLibraryState $state);
   

   It's odd that the state argument is in different places.

5. +++ b/core/modules/media_library/src/Plugin/views/field/MediaLibrarySelectForm.php
   @@ -120,27 +114,23 @@ public function viewsForm(array &$form, FormStateInterface $form_state) {
   +    $service_id = $media_library_state->getOpenerId();
   +    return \Drupal::service($service_id)
   +      ->getSelectionResponse($selected_ids, $media_library_state)
   +      ->addCommand(new CloseDialogCommand());
   

   Should we more explicit about interfaces. I.e something like

   $service = \Drupal::service($media_library_state->getOpenerId());
   if (!($service instanceof MediaLibraryOpenerInterface)) {
     throw \RuntimeException('Some message');
   }
   

Excellent nits, I wish I spotted those too :)

All feedback from #47 is addressed in here.

Discussed with @phenaproxima and after looking at the access patch I think we should have a very very thin service that gets an MediaLibraryOpenerInterface service from the container when given a MediaLibraryState object. That way the class resolver won't end up being injected everywhere. And we can keep the interface checking in a central location.

Here ya go! Meet \Drupal\media_library\OpenerResolverInterface and its offspring.

+++ b/core/modules/media_library/src/OpenerResolver.php
@@ -0,0 +1,31 @@
+  /**
+   * {@inheritdoc}
+   */
+  public function get($service_id) {

Instead of a service ID i'd make this take an instance of MediaLibrabryState

Done.

Fixed a typo in the doc comment of MediaLibraryOpenerInterface. No need to change from RTBC, since it's a one-line change (albeit a significant one).

Committed and pushed ca0ac736fd to 8.8.x and 9a654bb02a to 8.7.x. Thanks!

Backported to 8.7.x since media library is experimental.

diff --git a/core/modules/media_library/src/Plugin/views/field/MediaLibrarySelectForm.php b/core/modules/media_library/src/Plugin/views/field/MediaLibrarySelectForm.php
index 4afac3cfb7..fcb8627ba7 100644
--- a/core/modules/media_library/src/Plugin/views/field/MediaLibrarySelectForm.php
+++ b/core/modules/media_library/src/Plugin/views/field/MediaLibrarySelectForm.php
@@ -6,7 +6,6 @@
 use Drupal\Core\Form\FormBuilderInterface;
 use Drupal\Core\Form\FormStateInterface;
 use Drupal\Core\Url;
-use Drupal\media_library\MediaLibraryOpenerInterface;
 use Drupal\media_library\MediaLibraryState;
 use Drupal\views\Plugin\views\field\FieldPluginBase;
 use Drupal\views\Render\ViewsRenderPipelineMarkup;

Fixed unused use on commit.

Backported to 8.7.x since media library is experimental.

Yay, this means #2998005: [PP-1] Support Drupal core's Media Library can also use this!