Problem/Motivation

Followup for and postponed on #2831274-372: Bring Media entity module to core as Media module.

As createSourceField() only creates the field_config but does not save it, there's no way to change the MediaSource entity form display for this field to a more sane default from code (like a custom derivative of inline_entity_form in my case, see below). As this method is called from MediaTypeForm::save() (and not from anything in MediaSourceBase), there is no way to even intervene. There's not even a hook there. In the contrib era, this was doable via hook_media_bundle_insert()/hook_ENTITY_TYPE_insert(), but this isn't available either.

We should allow the MediaSource plugin (or at least the module providing it) to change the form display settings so they can be "good enough". In the contrib era, brightcove.module solved this by changing the form display settings from hook_ENTITY_TYPE_insert():

function media_entity_brightcove_media_bundle_insert(MediaBundle $bundle) {
  $field_name = $bundle->type_configuration['source_field'];
  $form_display_settings = [
    'type' => 'brightcove_inline_entity_form_complex',
    'settings' => [
      'form_mode' => 'default',
      'allow_new' => 1,
      'allow_existing' => 1,
      'match_operator' => 'CONTAINS',
    ],
    'third_party_settings' => [],
    'weight' => 0,
  ];
  // Create (or update) the entity form display for this new media bundle to
  // include this new field with some more sane defaults.
  /** @var \Drupal\Core\Config\Entity\ConfigEntityStorage $entity_form_display_storage */
  $entity_form_display_storage = \Drupal::getContainer()->get('entity_type.manager')->getStorage('entity_form_display');
  /** @var \Drupal\Core\Entity\Entity\EntityFormDisplay $entity_form_display */
  $entity_form_display = $entity_form_display_storage->load('media.' . $bundle->id() . '.default');
  if (!$entity_form_display) {
    $values = [
      'status' => TRUE,
      'targetEntityType' => 'media',
      'bundle' => $bundle->id(),
      'mode' => 'default',
      'content' => [
        $field_name => $form_display_settings,
      ],
    ];
    $entity_form_display = $entity_form_display_storage->create($values);
  }
  else {
    $entity_form_display->setComponent($field_name, $form_display_settings);
  }
  $entity_form_display_storage->save($entity_form_display);
}

As you can see, that approach wasn't the best one either, because the field wasn't created with appropriate form display settings (honestly, I don't really know if it's even possible), but those settings were only changed afterwards.

OTOH, it's clearly visible that it's only about providing that $form_display_settings.

Proposed resolution

Add a new method to MediaSourceInterface which can provide default display settings for a new media type, for both the form and view entity displays of said media type.

Remaining tasks

  • Write a patch
  • Review it
  • Smile upon it
  • Commit it

User interface changes

None.

API changes

MediaSourceInterface will get a new method, and MediaSourceBase will provide the default implementation.

Data model changes

None.

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

Boobaa created an issue. See original summary.

Boobaa’s picture

Title: [PP-1] Allow MediaSource plugins provide default field form display settings » [PP-1] Allow MediaSource plugins provide default field form/view display settings
Issue summary: View changes

Oh, and the same thing could be done for the field view display settings as well.

Boobaa’s picture

I did some more investigation and I found hook_ENTITY_TYPE_insert()/hook_media_type_insert(). However, it doesn't really help us to solve the problem, because this hook runs during the parent::save() call in MediaTypeForm::save() (which is the very first line of it, of course). Once the source field is created and saved (later in MediaTypeForm::save()), its form and view displays are immediately overwritten with the defaults for the actual field type.

So it looks like we can't use this hook for changing the default display modes, so it looks like OP is still the best proposed resolution (so far, with the current state of the core patch).

Boobaa’s picture

Title: [PP-1] Allow MediaSource plugins provide default field form/view display settings » Allow MediaSource plugins provide default field form/view display settings
Status: Postponed » Active

Media patch is in, so this is no longer postponed.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

chr.fritsch’s picture

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.

chr.fritsch’s picture

Status: Active » Needs review
FileSize
3.15 KB

Here is a first draft

Boobaa’s picture

Status: Needs review » Needs work

First, this is an API change, and IDK if it's acceptable in this phase.

Second, there might be field widgets/formatters that MUST have non-empty default settings. IOW, the fallback should be the default settings provided by the widget/formatter, not an empty array.

chr.fritsch’s picture

Status: Needs work » Needs review

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.

phenaproxima’s picture

Issue tags: -D8Media +Media Initiative

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?

phenaproxima’s picture

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?

chr.fritsch’s picture

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

phenaproxima’s picture

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.

chr.fritsch’s picture

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

phenaproxima’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

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! :)

chr.fritsch’s picture

Status: Needs work » Needs review
FileSize
11.67 KB
10.31 KB

Here are the tests

phenaproxima’s picture

Status: Needs review » Needs work
Issue tags: -Needs 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.

chr.fritsch’s picture

Status: Needs work » Needs review
FileSize
11.08 KB
7.01 KB

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.

Status: Needs review » Needs work

The last submitted patch, 19: 2865184-19.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

chr.fritsch’s picture

Status: Needs work » Needs review

Back to NR because of testbot hickup

phenaproxima’s picture

Status: Needs review » Needs work

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".

chr.fritsch’s picture

Status: Needs work » Needs review
FileSize
10.89 KB
2.43 KB

Fixed the nits.

phenaproxima’s picture

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.

chr.fritsch’s picture

Damn, you said that already before....

phenaproxima’s picture

Status: Needs review » Needs work
Issue tags: +Needs change record

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.

chr.fritsch’s picture

Status: Needs work » Needs review
phenaproxima’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

I think we're ready.

phenaproxima’s picture

Issue tags: -Needs change record

We've got the change record :)

xjm’s picture

Status: Reviewed & tested by the community » Needs review
+++ 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?

phenaproxima’s picture

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.

chr.fritsch’s picture

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.

phenaproxima’s picture

Status: Needs review » Needs work
+++ 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');
chr.fritsch’s picture

Status: Needs work » Needs review
FileSize
11.51 KB
1.77 KB

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

phenaproxima’s picture

Status: Needs review » Needs work
+++ 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.

chr.fritsch’s picture

Status: Needs work » Needs review
FileSize
11.62 KB
2.85 KB

Ok, fixed that.

The last submitted patch, 34: 2865184-34.patch, failed testing. View results

phenaproxima’s picture

Status: Needs review » Needs work

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

chr.fritsch’s picture

Status: Needs work » Needs review

No, because we use the default 😉

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

OK, I guess we're back to RTBC.

xjm’s picture

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

larowlan’s picture

Status: Reviewed & tested by the community » Needs work

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

phenaproxima’s picture

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).

chr.fritsch’s picture

Status: Needs work » Needs review
FileSize
12.63 KB
8.99 KB

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?

phenaproxima’s picture

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()?

phenaproxima’s picture

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

chr.fritsch’s picture

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

I also updated the CR

marcoscano’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs change record updates

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.

phenaproxima’s picture

The change record looks great. +1 for RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 47: 2865184-47.patch, failed testing. View results

chr.fritsch’s picture

Status: Needs work » Reviewed & tested by the community

Testbot hickup

marcoscano’s picture

larowlan’s picture

Added review credits

  • larowlan committed de9e2d7 on 8.5.x
    Issue #2865184 by chr.fritsch, phenaproxima, larowlan, marcoscano, xjm:...
larowlan’s picture

Status: Reviewed & tested by the community » Fixed

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.

phenaproxima’s picture

Status: Fixed » Closed (fixed)

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