Problem/Motivation

Media source instances carry a getMetadata() method. As this is not pluginified, extending it requires us to decorate the class. (Remember: decorating is not swapping a subclass in, as that will break other decorators(todo: link issue)). As simpler solutions did not work out (todo: link to documentation), we first had to decorate the manager.

In the Media Imagick module i implemented that successfully (and in a reusable way), but get a WSOD as at least 1 place in core programs against the class and not an interface:
TypeError: Argument 1 passed to Drupal\media\MediaTypeForm::__construct() must be an instance of Drupal\media\MediaSourceManager, instance of Drupal\plugindecorator\PluginManagerDecorator

Proposed resolution

Create a DefaultPluginManagerInterface and use it.

Daring to target 8.6 and raise prio as this blocks a stable release of a contrib module and is important to the media system.

Remaining tasks

Code, test, commit.

User interface changes

None.

API changes

Interface added.

Data model changes

None.

CommentFileSizeAuthor
#6 drupal-2987414-6-Decorating-media-source-manager.patch1.79 KBgeek-merlin
#3 drupal-2987414-3-Decorating-media-source-manager.patch3.96 KBgeek-merlin
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

axel.rutz created an issue. See original summary.

geek-merlin’s picture

geek-merlin
they/omg
Primary language German
Location Freiburg, Germany
CreditAttribution: geek-merlin at MachbarMacher commented
Priority: Normal » Major
geek-merlin’s picture

geek-merlin
they/omg
Primary language German
Location Freiburg, Germany
CreditAttribution: geek-merlin at MachbarMacher commented
Status: Active » Needs review
FileSize
drupal-2987414-3-Decorating-media-source-manager.patch3.96 KB

Straightforward patch flying in. Gonna test it now.

geek-merlin’s picture

geek-merlin
they/omg
Primary language German
Location Freiburg, Germany
CreditAttribution: geek-merlin at MachbarMacher commented
Status: Needs review » Reviewed & tested by the community

Tested. This fixes the WS for me and makes the module work. Daring to RTBC to kindly ping captain picard.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Reviewed & tested by the community » Needs work

@axel.rutz I'm pretty sure you know the community convention that you shouldn't rtbc your own work. 

+++ b/core/lib/Drupal/Core/Plugin/DefaultPluginManagerInterface.php
@@ -0,0 +1,15 @@
+interface DefaultPluginManagerInterface extends PluginManagerInterface, CachedDiscoveryInterface, CacheableDependencyInterface {

+++ b/core/modules/media/src/MediaTypeForm.php
@@ -37,12 +38,12 @@ class MediaTypeForm extends EntityForm {
-  public function __construct(MediaSourceManager $source_manager, EntityFieldManagerInterface $entity_field_manager) {
+  public function __construct(DefaultPluginManagerInterface $source_manager, EntityFieldManagerInterface $entity_field_manager) {

This is not needed. We should typehint against Drupal\Component\Plugin\PluginManagerInterface and be done. No need for the new Core/Plugin code. For prior art look at all the other plugin managers in core eg. interface LocalTaskManagerInterface extends PluginManagerInterface

geek-merlin’s picture

geek-merlin
they/omg
Primary language German
Location Freiburg, Germany
CreditAttribution: geek-merlin at MachbarMacher commented
Status: Needs work » Needs review
FileSize
drupal-2987414-6-Decorating-media-source-manager.patch1.79 KB

Thanks @alexpott for the quick reply and guidance! ❤

> We should typehint against Drupal\Component\Plugin\PluginManagerInterface and be done.

All the better. Updated patch implements that.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Status: Needs review » Reviewed & tested by the community

MediaSourceManager does not implement any methods that don't already exist on PluginManagerInterface, so this change makes sense and does not break the API or have BC implications. If it will help contrib, let's go for it. RTBC once green on all backends.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Version: 8.6.x-dev » 8.7.x-dev
Issue tags: +Media Initiative

Targeting 8.7.x with intent to (hopefully) backport.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Title: Decorating media source manager leads to WSOD » MediaTypeForm should type hint PluginManagerInterface, not MediaSourceManager
Issue tags: +Contributed project blocker

Re-titling for clarity.

geek-merlin’s picture

geek-merlin
they/omg
Primary language German
Location Freiburg, Germany
CreditAttribution: geek-merlin at MachbarMacher commented

@phenaproxima:
Thanks a lot for the review! ❤

I can confirm that this makes media_imagick work.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed d67d9f3248 to 8.7.x and 3ef46a6c24 to 8.6.x. Thanks!

  • alexpott committed d67d9f3 on 8.7.x 
    Issue #2987414 by axel.rutz: MediaTypeForm should type hint...

  • alexpott committed 3ef46a6 on 8.6.x 
    Issue #2987414 by axel.rutz: MediaTypeForm should type hint...
tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler as a volunteer commented
Version: 8.7.x-dev » 8.6.x-dev

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.