Allow MediaSource plugins provide default field form/view display settings

See #2831943-119: Use "rendered media" (not links) as default media field formatter; add modal to configure the used media view mode.

In #2863648: Consider providing a mechanism for media source field validation we decided against adding field settings to the annotation and rather use the existing API. So this should probably be solved by introducing some new methods on the MediaSourceInterface.

Here is a first draft

I think adding new methods to an interface should be possible by https://www.drupal.org/core/d8-allowed-changes#minor

setComponent calls \Drupal::service('plugin.manager.field.widget')->prepareConfiguration($field_type, []) internally. So it's save to provide an empty array.

This is a cool idea, and thanks for the initial patch!

It's definitely an API addition, but if policy allows it, then I think we can go for it. I can't imagine too many scenarios where media source plugins will not be extending MediaSourceBase, so as long as we add a base implementation to accompany the new interface method, I think we have our bases covered.

+++ b/core/modules/media/src/MediaSourceInterface.php
@@ -139,4 +139,24 @@ public function getSourceFieldDefinition(MediaTypeInterface $type);
+  public function getSourceFieldFormDisplayOptions();

I think we should pass the initial formatter settings ($component, in MediaTypeForm) to this method by reference and allow it to alter that, rather than ask it to return complete settings.

I also question if we need two methods. Maybe we can do something like this:

public function alterDisplayOptions(array &$component, $display_context)

...where $display_context is either 'form' or 'view'. What do you think?

Another thought occurs to me -- what if the media source wants to hide the component entirely?

My first thought is that the method(s) should return a boolean to indicate if the component should actually be visible. If it's false, we don't add the component at all; otherwise, we do. Thoughts?

I think then it’s fine to return type => hidden

I think then it’s fine to return type => hidden

That's not the same thing as hiding the component, though. Calling $display->removeComponent('foo') will add the component name to a special 'hidden' array, but completely remove it from the 'content' array. Returning ['type' => 'hidden'] would be quite ambiguous. If we're going to alter the component by reference, it seems reasonable to allow the return value to indicate the absolute status of the component.

I don't think we should alter the default options. Just return whats important for you and everything else will be added by $this->pluginManager->prepareConfiguration in EntityDisplayInterface::setComponent

I love this patch. I have only one request to make:

+++ b/core/modules/media/src/MediaSourceInterface.php
@@ -139,4 +139,18 @@ public function getSourceFieldDefinition(MediaTypeInterface $type);
+  /**
+   * Get the options for the source field in the form/view display.
+   *
+   * @param string $display_context
+   *   Possible parameters are 'view' or 'form'.
+   *
+   * @return array|bool
+   *   The display options as array or FALSE for removing this component from
+   *   the display.
+   *
+   * @see \Drupal\Core\Entity\Display\EntityDisplayInterface::setComponent()
+   */
+  public function getDisplayOptions($display_context);

Can we pass the media type entity into this method as well, so that it can perform more advanced logic if it wants to?

Apart from that, let's get a couple of tests in here! :)

Here are the tests

Fabulous work. I have nothing but minor things to point out.

1. +++ b/core/modules/media/src/MediaSourceInterface.php
   @@ -139,4 +139,20 @@ public function getSourceFieldDefinition(MediaTypeInterface $type);
   +   * Get the options for the source field in the form/view display.
   

   Nit: "Get" should be "Gets".

2. +++ b/core/modules/media/src/MediaSourceInterface.php
   @@ -139,4 +139,20 @@ public function getSourceFieldDefinition(MediaTypeInterface $type);
   +   * @param \Drupal\media\MediaTypeInterface $type
   +   *   A media type.
   

   Let's improve this description a bit -- maybe "The media type which is using this source"?

3. +++ b/core/modules/media/src/MediaSourceInterface.php
   @@ -139,4 +139,20 @@ public function getSourceFieldDefinition(MediaTypeInterface $type);
   +   * @param string $display_context
   +   *   Possible parameters are 'view' or 'form'.
   

   Needs to be explained a bit more. Maybe "The display context which will use these options. Can be 'view' or 'form'"?

4. +++ b/core/modules/media/src/MediaSourceInterface.php
   @@ -139,4 +139,20 @@ public function getSourceFieldDefinition(MediaTypeInterface $type);
   +   * @return array|bool
   +   *   The display options as array or FALSE for removing this component from
   +   *   the display.
   

   Let's remove "as array", and add a comma after "The display options".

5. +++ b/core/modules/media/src/MediaTypeForm.php
   @@ -375,4 +366,26 @@ public function save(array $form, FormStateInterface $form_state) {
   +   * Set's source display options to an entity display.
   

   "Set's" should be "Sets" (no apostrophe).

6. +++ b/core/modules/media/src/MediaTypeForm.php
   @@ -375,4 +366,26 @@ public function save(array $form, FormStateInterface $form_state) {
   +   * @param \Drupal\Core\Entity\Display\EntityDisplayInterface $entityDisplay
   

   As per convention, let's snake_case the parameter name, not camelCase.

7. +++ b/core/modules/media/tests/src/Kernel/MediaSourceTest.php
   @@ -442,4 +443,96 @@ public function testSourceConfigurationSubmit() {
   +    $form = MediaTypeForm::create(\Drupal::getContainer());
   +    $form->setEntity($type);
   +    $form->setModuleHandler(\Drupal::moduleHandler());
   +    $form->setEntityTypeManager(\Drupal::entityTypeManager());
   

   Can't we use the entity form builder service here, rather than creating the form object statically?

8. +++ b/core/modules/media/tests/src/Kernel/MediaSourceTest.php
   @@ -442,4 +443,96 @@ public function testSourceConfigurationSubmit() {
   +    $this->assertArrayHasKey($field_name, $display->getComponents());
   +    $this->assertSame('entity_reference_entity_id', $display->getComponents()[$field_name]['type']);
   

   Rather than call $display->getComponents() twice, can we re-use its return value?

9. +++ b/core/modules/media/tests/src/Kernel/MediaSourceTest.php
   @@ -442,4 +443,96 @@ public function testSourceConfigurationSubmit() {
   +    $this->assertArrayHasKey($field_name, $display->getComponents());
   +    $this->assertSame('entity_reference_autocomplete_tags', $display->getComponents()[$field_name]['type']);
   

   Same here.

10. +++ b/core/modules/media/tests/src/Kernel/MediaSourceTest.php
    @@ -442,4 +443,96 @@ public function testSourceConfigurationSubmit() {
    +    $display = entity_get_display('media', $id, 'default');
    +    $this->assertArrayNotHasKey($field_name, $display->getComponents());
    +
    +    $display = entity_get_form_display('media', $id, 'default');
    +    $this->assertArrayNotHasKey($field_name, $display->getComponents());
    

    Same here.

Thanks for reviewing @phenaproxima

I fixed all the nits.

One thing about #18.7: I was not able to use the entity form builder service, but i use $this->container->get('entity_type.manager')->getFormObject('media_type', 'add') instead now. This also simplified the code a bit.

Back to NR because of testbot hickup

Only two more things that I can see.

+++ b/core/modules/media/tests/src/Kernel/MediaSourceTest.php
@@ -497,20 +471,36 @@
+   * @param int $media_type_id
+   *   Media type id.

$media_type_id looks like it's the source plugin ID, so this should be $source_plugin_id or something. Also, not an int :)

+++ b/core/modules/media/src/MediaTypeForm.php
@@ -367,24 +367,24 @@
+   * Sets source display options to an entity display.

Should say "...options on an entity display".

Fixed the nits.

One last thing:

+++ b/core/modules/media/tests/src/Kernel/MediaSourceTest.php
@@ -484,14 +483,14 @@
+   * @param int $source_plugin_id

Should be a string, not an int.

Damn, you said that already before....

Code looks good to go. Thank you for all you've done, @chr.fritsch!

Since we're adding something to Media's API, I think a change record is in order here, so I'm kicking this back to NW for that. Once we have a change record, this is RTBC.

Here is the CR: https://www.drupal.org/node/2928238

I think we're ready.

We've got the change record :)

+++ b/core/modules/media/src/MediaSourceInterface.php
@@ -139,4 +139,20 @@ public function getSourceFieldDefinition(MediaTypeInterface $type);
+   * @return array|bool
+   *   The display options, or FALSE for removing this component from
+   *   the display.

This should at least define a little more clearly what "the display options" are. It looks like it requires knowledge of a particular array structure for the display options that would be defined elsewhere.

I also find myself if this interface is the right place to put this method. It seems to add a coupling that MediaSourceInterface didn't have previously?

I also find myself if this interface is the right place to put this method. It seems to add a coupling that MediaSourceInterface didn't have previously?

I'm not sure what a better place for it would be. As part of its interface, the media source is already responsible for defining the source field itself, which couples it to the Field API anyway. By design, media is quite coupled to the field system. So, to me, this does not introduce any new, significant coupling. I'm open to opinions on what might be better, though. Would an alter hook be preferable? Either way, this is definitely something we need to solve.

This should at least define a little more clearly what "the display options" are. It looks like it requires knowledge of a particular array structure for the display options that would be defined elsewhere.

Agreed.

I agree with @phenaproxima. We are already injecting the MediaTypeInterface into ::getSourceFieldDefinition() and ::createSourceField().
The new method ::getDisplayOptions() is in the same scope. It's used during the source field creation.

So i just updated the docs a bit.

+++ b/core/modules/media/src/MediaSourceInterface.php
@@ -150,6 +150,10 @@
+   *   The display options must have the same structure like the options for
+   *   EntityDisplayInterface::setComponent($name, array $options = []).
+   *   Returning an empty array means that the default display options of the
+   *   source field will be used.

This is good, but let's move it above the parameter descriptions, just below the short one-line description of the method. Also, we can remove the arguments from setComponent(), and EntityDisplayInterface should be fully qualified.

If we want to get even clearer, we can explain that if the $display_context is 'view', then the returned array should be a set of formatter options (i.e, for an entity view display), and if the $display_context is 'form', the returned array should be a set of widget options (i.e., for an entity form display). IMHO it can't hurt to document this stuff as clearly as possible, seeing as how view and form displays are already pretty confuzzling.

Additionally, I wonder if we should add an additional parameter to the method -- in this case, the view or form mode (which would default to 'default'). It would provide additional flexibility for implementations. What do you think? Something like this is what I'm envisioning:

public function getDisplayOptions(MediaTypeInterface $media_type, $display_context, $display_mode = 'default');

I added the view_mode to :: getDisplayOptions. Currently there is no benefit from it, but we are a bit saver for the future.

+++ b/core/modules/media/src/MediaSourceInterface.php
@@ -143,20 +143,26 @@
+  public function getDisplayOptions(MediaTypeInterface $type, $display_context, $view_mode = 'default');

Looks excellent! But let's rename $view_mode to $display_mode, since that's the more generic term. Also, we need to add the parameter to the default implementation in MediaSourceBase.

Ok, fixed that.

Looks good, but MediaTypeForm needs to call the method will that argument too :)

No, because we use the default 😉

OK, I guess we're back to RTBC.

That answers my question, thanks @phenaproxima and @chr.fritsch!

I agree this is needed, I just have a couple of observations that I think would make this more robust

1. +++ b/core/modules/media/src/MediaSourceInterface.php
   @@ -139,4 +139,30 @@ public function getSourceFieldDefinition(MediaTypeInterface $type);
   +   *   The display options, or FALSE for removing this component from
   

   This concerns me, mixed returns always do

2. +++ b/core/modules/media/src/MediaTypeForm.php
   @@ -4,6 +4,7 @@
   @@ -328,29 +329,19 @@ public function save(array $form, FormStateInterface $form_state) {
   

   What happens if someone submits it over REST? I suspect that the source plugin doesn't get the same opportunities as it does via the UI. We should avoid logic in the form. So that would probably indicate that it belongs in MediaType::postSave()

3. +++ b/core/modules/media/src/MediaTypeForm.php
   @@ -328,29 +329,19 @@ public function save(array $form, FormStateInterface $form_state) {
   +        $this->setDisplayOptions($display, $field_name, $source->getDisplayOptions($media_type, 'form'));
   +        $display->save();
   ...
   +        $this->setDisplayOptions($display, $field_name, $source->getDisplayOptions($media_type, 'view'));
   +        $display->save();
   

   In my opinion, it would be more powerful if the method on MediaSourceInterface was

   
   public function prepareDisplay(MediaTypeInterface $type, $field_name, EntityDisplayInterface $display);
   
   

   That way the plugin has the power to call ::removeComponent or ::setComponent as necessary.

   In addition the, $display_context argument isn't needed - as $display->getMode() is available.

   But it also opens up the possibility for more flexible options too, such as ensuring the label is output by default, or modifying the weight of other fields. Not saying there is a concrete use case for that, but we can leave the door open.

   Also, it gets rid of the mixed return and MediaTypeForm::setDisplayOptions can go too.

4. +++ b/core/modules/media/tests/modules/media_test_source/src/Plugin/media/Source/TestDifferentDisplays.php
   @@ -0,0 +1,43 @@
   +      case 'view':
   ...
   +      case 'form':
   

   There is also the case for splitting it into two methods. ::prepareViewDisplay and ::prepareFormDisplay which would avoid the need for a switch. But that is just my are you sure you really want a switch? filter kicking in and I realise this is contrived for a test....so ignore me unless you feel strongly about it

5. +++ b/core/modules/media/tests/src/Kernel/MediaSourceTest.php
   @@ -442,4 +442,79 @@ public function testSourceConfigurationSubmit() {
   +    $this->createMediaTypeViaForm($id, $field_name);
   

   Moving it to MediaType::postSave would also allow us to just use the ::create method here. So the fact we need to use a form in this test is another indicator that we have the code in the wrong place I think

What happens if someone submits it over REST? I suspect that the source plugin doesn't get the same opportunities as it does via the UI.

This is already the case. The source field is created automatically only in the UI; this display stuff should, IMHO, be handled similarly. That keeps a clean, or at least clear, separation between the way things work in UI (which will create/alter several things while creating a new media type) and the API (in which you have to create the source field, and prepare any displays, in separate calls). This was done because having the media source permanently alter config at the API level (i.e., in preSave or postSave) turned out to be fraught with problems, largely having to do with config dependencies. So I think we should stay away from that here.

That said, I agree that passing the display entity is a better idea than mixed returns, so let's do that. And I also agree that we can split this out into separate methods (which can then each receive the more specific variant of EntityDisplayInterface as their parameter).

Thanks for reviewing @larowlan.

I splitted up getDisplayOptions into prepareViewDisplay and prepareFormDisplay as you suggested and left the functionality in MediaTypeForm because of @phenaproxima's explanation.

I also think we could remove the $field_name from prepareViewDisplay and prepareFormDisplay because it could be retrieved directly inside. What's your opinions on that?

My only question:

+++ b/core/modules/media/src/MediaSourceBase.php
@@ -317,4 +319,18 @@ protected function getSourceFieldName() {
+  public function prepareViewDisplay(MediaTypeInterface $type, $field_name, EntityViewDisplayInterface $display) {
+    $display->setComponent($field_name);
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function prepareFormDisplay(MediaTypeInterface $type, $field_name, EntityFormDisplayInterface $display) {

Do we really need to pass $field_name? The source plugin is aware of its own source field, so can't it simply use $this->getSourceFieldDefinition($type)->getName()?

We'll need to update the change record before we can re-RTBC...

Since we are both asking this question, I think we should remove the $field_name.

I also updated the CR

Great work here! This will be a very much appreciated feature for modules providing source plugins.

I have manually tested this, tweaking the Image.php source plugin with something like:

  /**
   * {@inheritdoc}
   */
  public function prepareViewDisplay(MediaTypeInterface $type, EntityViewDisplayInterface $display) {
    $display->setComponent($this->getSourceFieldDefinition($type)->getName(), [
      'type' => 'image_url',
      'settings' => [
        'image_style' => 'thumbnail',
      ],
    ]);
  }

and it works beautifully.

I had some doubts about types/displays being created programmatically or through imported configuration, but I see that this concern was already answered in #43, and it is also true that if a module/distribution provides config for a display we should assume it is expected to be a "good" config for that plugin.

Also checked the change record. Fixed a minor typo and added an example snippet to make more explicit the new functionality.

IMHO this should be very close to ready.

The change record looks great. +1 for RTBC.

Testbot hickup

Postponed #2924631: Media sources for local video and audio support on this issue.

Added review credits

This is already the case. The source field is created automatically only in the UI; this display stuff should, IMHO, be handled similarly. That keeps a clean, or at least clear, separation between the way things work in UI (which will create/alter several things while creating a new media type) and the API (in which you have to create the source field, and prepare any displays, in separate calls). This was done because having the media source permanently alter config at the API level (i.e., in preSave or postSave) turned out to be fraught with problems, largely having to do with config dependencies. So I think we should stay away from that here.

Can you open a followup to explore that?

Committed as de9e2d7 and pushed to 8.5.x

Published the change record.

Nice work all.

Done: #2930181: Media types should create their source field during pre/postSave.

Thanks, @larowlan!