MediaLibraryWidget should use the media_library.ui_builder service when fetching modal dialog options

The class was added as a service in #3020716: Add vertical tabs style menu to media library, but it was contested whether this code should be overrideable, see #19:

Marked internal, but I do see lot's of ways developers can extend the library in the future. Swapping out the media type vertical tabs for a list of taxonomy terms for example. Let's not make it part of the API for now, but I do think brave souls should be able to work with this (even though we don't encourage or support it).

If the class is declared as a service, it should be called as a service, so this change makes sense.

1. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -770,8 +770,9 @@ public static function removeItem(array $form, FormStateInterface $form_state) {
   -    $dialog_options = MediaLibraryUiBuilder::dialogOptions();
   ...
   +    $library_ui = $ui_builder->buildUi($triggering_element['#media_library_state']);
   

   The MediaLibraryUiBuilder is now an unused use statement.

2. Let's also update the call in \Drupal\media_library\Plugin\CKEditorPlugin\DrupalMediaLibrary for consistency.

Patch looks good now, thanks!

Tagging for subsystem maintainer review. I think we should probably just take internal off the service if we're going this direction.

I've had more questions about changing the width and height of the modal. We have discussed things like an alter hook before but since the class is also used in a lot of javascript we should not make it easy for people to change this.

As a first step I can see making this method non-static and fetch the options via the service is a good step to help with this. Calling the static method on the class is definitely not developer friendly. I guess if we make the width and height configurable this would even help other people from not needing to override the service as well, but that can be a followup.

For the patch, I like the change. Can we add a comment to dialogOptions() that the class name is used in the javascript to warn people that removing/changing the class breaks the library?

I think we should probably just take internal off the service if we're going this direction.

I don't really agree with this; IMHO, we should remove the internal designation when and if we add an interface for the service. That would put it, in my view, firmly into the API realm.

However, the proposed change is pretty harmless and increases the flexibility available to mad scientists, while keeping the media library "just working" out of the box. So, +1 for this change. And I agree with this:

Can we add a comment to dialogOptions() that the class name is used in the javascript to warn people that removing/changing the class breaks the library?

Setting "needs work" for that.

Should this maybe get a follow up to not use a class for that but a data-attribute or at least as js- prefixed class that makes it clearer that this is a non-styling class?

Yes, this should be a js- prefixed class if it's required for functionality. Tagging for a follow-up.

Thank you! This is ready, IMHO.

This seems like a sound change.

+++ b/core/modules/media_library/src/Plugin/CKEditorPlugin/DrupalMediaLibrary.php
@@ -151,7 +151,7 @@ public function getConfig(Editor $editor) {
-      'DrupalMediaLibrary_dialogOptions' => MediaLibraryUiBuilder::dialogOptions(),
+      'DrupalMediaLibrary_dialogOptions' => \Drupal::service('media_library.ui_builder')->dialogOptions(),

+++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
@@ -770,8 +769,9 @@ public static function removeItem(array $form, FormStateInterface $form_state) {
-    $library_ui = \Drupal::service('media_library.ui_builder')->buildUi($triggering_element['#media_library_state']);
-    $dialog_options = MediaLibraryUiBuilder::dialogOptions();
+    $ui_builder = \Drupal::service('media_library.ui_builder');

Is there a particular reason we're using \Drupal::service() here rather than injecting the dependency?

Also, it sounds like we need a followup to discuss #7 (in addition to the js-prefixing stuff).

Thanks!

EDIT: Never mind, misinterpreted the diff @xjm posted.

Filed #3167992: Make the media library modal's dimensions configurable to address making the modal dimensions configurable.

OK, this should address @xjm's feedback (injected the dependency into the non-static places that it's used). No interdiff because for some reason it's misbehaving (showing the same diff twice).

Fixing test.

Thanks, @vsujeetkumar, for cleaning that up! We do need to keep the default value of the $media_library_ui_builder parameter as NULL for now, so that we don't break classes which might be extending that one. :) Otherwise, looks good.

@phenaproxima As per your comment #20, I have set the NULL as default value of $media_library_ui_builder.

Replacing patches with a merge request.

@xjm's feedback seems to have been addressed, and follow-ups have been filed. IMHO this is ready, but since I've worked on it I don't think I can RTBC. :(

The dialog options are pretty well tested since the JavaScript relies on the .media-library-widget-modal class. We also assert the dialog title Add or select media, which is set by the options, A LOT! For that reason I'm convinced this little helpful change is good to go. Feedback seems to be all addressed.

Left a minor comment regarding the change notice link on the MR

I realise this is all to facilitate easier overriding of the defaults - but don't we have other ways to alter the output - e.g alter hooks, widget plugins etc etc - I.e. I'm not totally convinced on this change. I don't feel strongly about it though

but don't we have other ways to alter the output - e.g alter hooks, widget plugins etc etc

I'm not aware of any alter hook which can change these options. They are set in an internal AJAX callback and stored in an AJAX command, which has an API to modify the options after the fact, but it's not an API that can be accessed at a useful time (i.e., when the AJAX response is being built). Overriding the defaults can be done without this change, as you can see in this merge request against Media Library Extras: https://www.drupal.org/project/media_library_extras/issues/3228196

But that way is hacky. It requires the module to root around in the guts of an AJAX command, which is probably 95% reliable, but still not an API. Making the UI builder into a service will at least help mitigate this, but ideally we'll open a follow-up that sets the dialog options in the widget form (maybe attached to the "open media library" button as a property), which would allow the existing hook_field_widget_single_element_form_alter() to influence them. I'm open to do doing that in this issue, though, if you think that's a preferable approach.

Nonetheless, I don't think we should be getting the dialog options statically. The UI builder class is explicitly marked as internal anyway, so IMHO we should minimize the need for other classes to call it directly.

Here's a patch version of the MR. Not running tests; I just want to use this on a project.

I'm also happy to help get this over the finish line. Seems like the deprecation message is the only thing holding this up. The deprecation message is out of date at this point, but I'm not sure what versions the deprecation would happen in now. The MR target also needs to be updated and I'm don't think I have the proper permissions for doing that.

I've updated the IS just a bit so that none of the sections are blank.

#28.1 Updated the target Drupal version and link to point to the change record

#28.2 This feedback is addressed in #29

Discussed with @danflanagan8 and @catch. We agreed that since this issue is doing some refactoring, it should be deferred to 10.1.x, and the deprecations adjusted accordingly. Thanks!

Hi Guys

I have rerolled the patch #32 since it does not apply for the latest drupal 9.5.x

Since the ckeditor plugin updates moved from

"core/modules/media_library/src/Plugin/CKEditorPlugin/DrupalMediaLibrary.php" to "core/modules/ckeditor/src/Plugin/CKEditorPlugin/DrupalMediaLibrary.php"

Please review

Thanks

As per #34, changing this issue to target 10.1.x.

Hey, @akshay_d!

The re-roll looks fine for the ckeditor plugin, but can you add the changes to the FieldWidget plugin as well?

Could you also label the patch with 9.5.x in the file name? Since ckeditor is removed in D10, we'll want to communicate that the patch is specifically for 9.5.x.

Thanks!

Thanks @danflanagan8 for reviewing the patch

I have updated the patch with missing FieldWidget updates

Please review

That looks like a good re-roll, @akshay_d. Thanks!

Just for clarity in the community, I wanted to reiterate that the patches in #32 and #38 are so this work-in-progress can be used on a project by including the patch through composer.

The real work should still happen on the MR. I'll set this back to NW because the MR needs to be updated to account for the removal of the DrupalMediaLibrary ckeditor(4) plugin. I'm not sure if the analogous ckeditor5 plugin would benefit from any related changes, but maybe?

Hi

I am updating the patch #38 for using ckeditor5 on drupal 9.5.x.

As part of this i have removed the ckeditor4 updates from #38. But the status will be still needs work from the comment #39.

Thanks

Patch for 10.4.9

Fix Patch for 10.4.9