Add an interface to media_library.ui_builder so the service can be decorated properly

Added a patch file for 9.5

Hi akalam, I have read through the code. It makes sense. Looks good.

I tested MR !3972 using Tugboat:

1. logged in as admin
2. enabled media and media library modules
3. Added media field and enabled all media types to Basic page
4. Created a basic page with an image added to the media field
5. The page displayed okay with no errors

@akalam, The media library extend module is using a service decorator to extend the service. I agree that the decorator pattern should really be using an interface and this interface seems to be the answer.

Are you suggesting that by using an interface in the media library module that the media library extend module will be made unnecessary?

Are you suggesting that developers will be able to implement their own custom modules and decorate the MediaLibraryUiBuilder class/ service?

Where do the plugins mentioned by Berdir fit in with your vision?

Even though it's task believe test coverage may be needed.

And not sure changing the constructors like that will fly. Think they will have to be deprecated and made required in D11. For backwards compatibility.

Hi @smustgrave,
By 'test coverage' do you meaning running the existing tests or writing new ones? The existing tests cover the core functionality, i think, so if creating new tests, what operations would they need to cover over and above core functionality?

@andrew.farquharson regarding the media library extend module, adding the interface doesn't make the module obsolete, but can allow the maintainers of the module to refactor it to use the interface instead of extending the core class, making it compatible with other modules which also decorates the media_library.ui_builder. If you extend a class instead of using the interface to call the inner service, you introduce your functionality but block others to introduce other functionality as well.

Regarding the test, I agree that there's no new functionality so nothing to test at that level, but still can be phpunit tests, like testing that the service can be decorated for example.

@akalam Yes, it seems that currently the media library extend module code is not using the decorator pattern. They are using this technique:

if ($state->get('media_library_extend') !== '1') {
      return parent::buildLibraryContent($state);

}

That workaround seems like a poor substitute for proper implementation of the decorator pattern: using an interface.

FWIW, It would be much nicer if the media_library module would actually have a plugin system that allows to have multiple additional UI elements that can play nice together. A decorator pattern is hard to implement correctly so that you can actually combine more than one of them you need to inject and call the inner service.

@Berdir @akalam I can start scaffolding a new plugin type for the media type module. The plugin type will need an interface so maybe this ticket can be closed as complete since the interface seems complete and i think it can be now be re-used in the plugin type (if it is agreed that decorator solution is not the right path to take).

@akalam do you want to create a child ticket and close this one?

@Berdir, do you have any views on this?

I do agree that the plugin system would be a much nicer solution, but it will require some analysis and can take much longer to commit it in core. Having an interface in a service dont have a downside and wont impact in a new plugin system.

Removing tests tag from previous comments.

Sorry I forgot to answer @smustgrave regarding the change in the constructors. The only changes are replacing the type hint to use the interface instead of the class, so IMO doesn't have an impact on the future deprecation in favor of constructor property promotion.
However, if it can help I can refactor the touched classes to use constructor property promotion.

I Created an updated patch for Drupal 10.1.x