Provide default source_field when creating new media entity bundles

If anyone is interested in this video_embed_media creates a default field for the user when they create the bundle. They can then choose another one if they wish not to use the provided one. This works quite well and could be considered to solve this problem as well?

Yes, solution in VEF is definitely very nice UX-wise. I am not sure if we can solve this generally or will every type plugin need to solve it separately.

I think what's in VEF should be generalized and set up in Media Entity.

VEF current createVideoEmbedField pretty much match the getDefaultSourceField proposed in the IS ; to get something really nice, I think it should be split, and return an unsaved field. It'd then be MediaEntity responsibility to actually save those fields if the site contributor ticked the corresponding checkbox.

Fixed typo in title.

#5 sounds like a good solution. This would significantly improve site builder UX.

I think it's would be unusual that a site builder would want to delete the default field? Should the media provider enforce the creation of a field and not give the site builder the option to delete it? This kind of simplifies things, because we would no longer need any kind of config form for selecting which is the "source field", but it's a rather large BC break as well.

Idea was to keep this flexibility. Some media types support more than one field type for example. Sticking to auto-created fields would remove possibility to choose.

This is a WIP patch, with the corresponding modif also in the twitter plugin, as an example.

Can someone please give a look to see how this approach looks?

It is not functional yet, the field is created but the "source_field" plugin config value is not saved. Didn't figure out how to do that yet...

On the other hand, it would be great if we could avoid the hook_insert and the flag on the entity to carry on the user preference from the checkbox.. But I couldn't find a way either, any suggestion is welcome!

sorry, didn't name correctly one of them

Kept working, found some bugs...
This seems to be functional, please review this patch and not the previous one.

I seem to have a problem uploading patches.. :) sorry

Great work so far. More of a general approach kind of review for now.

1. +++ b/media_entity.module
   @@ -82,3 +84,36 @@ function media_entity_copy_icons($source, $destination) {
   +function media_entity_media_bundle_insert(MediaBundle $media_bundle) {
   

   This could live on the bundle class as part of the postSave() callback.

   I prefer to have everything in one place and entity classes are that place in D8.

2. +++ b/src/Entity/MediaBundle.php
   @@ -230,6 +238,13 @@ class MediaBundle extends ConfigEntityBundleBase implements MediaBundleInterface
   +  public function getCreateSourceField() {
   +    return $this->create_source_field;
   +  }
   

   The reason why we didn't add this feature at the beginning is to prevent any assumptions.

   Who says that every type will be using just one field? There might be (very rare) situations where a type might need more fields to do its job.

   For that reason I'd try to avoid adding this assumption to the bundle. It would be much better if all this logic would live on base type class. This way most of types just extend the base class nad the ones that need more do their own crazy stuff.

3. +++ b/src/MediaBundleForm.php
   @@ -170,6 +170,22 @@ class MediaBundleForm extends EntityForm {
   +    // Add a checkbox to allow the field being created automattically on save.
   +    if (!empty($form['type_configuration'][$plugin->getPluginId()]['source_field']) && empty($plugin_configuration['source_field'])) {
   +      $has_source_field_options = !empty($form['type_configuration'][$plugin->getPluginId()]['source_field']['#options']);
   +      $form['type_configuration'][$plugin->getPluginId()]['create_source_field'] = [
   +        '#type' => 'checkbox',
   +        '#title' => $this->t('Create source field automatically when saving this form.'),
   +        '#description' => $this->t('If checked, a default field will be created and used as a source field. You can change this setting later on.'),
   +        '#default_value' => $bundle->getCreateSourceField(),
   +        // Try to place this checkbox close to the sourcefield select element.
   +        '#weight' => !empty($form['type_configuration'][$plugin->getPluginId()]['source_field']['#weight']) ? $form['type_configuration'][$plugin->getPluginId()]['source_field']['#weight'] + 0.01 : -2,
   +        '#access' => !$has_source_field_options,
   +      ];
   +      // If there are no options, the dropdown is meaningless.
   +      $form['type_configuration'][$plugin->getPluginId()]['source_field']['#access'] = $has_source_field_options || !$bundle->getCreateSourceField();
   +    }
   

   Checkbox could stay on the bundle. Maybe we could add another key in the type plugin annotation that would determine if it creates fields or not. This would allow us to control the display of the checkbox based on which type is being used.

Thank you for reviewing! The patches attached address the feedback.

There is still work to be done on DI, removing deprecates and writing tests, but let's build an agreement on the general approach first :)

Re

Maybe we could add another key in the type plugin annotation that would determine if it creates fields or not. This would allow us to control the display of the checkbox based on which type is being used.

Mmm.. do you think it is possible that a given plugin could have no source field at all? I was assuming that at least one source field would always be present, and then all plugins would need to implement the ::getDefaultSourceField() method (which is more or less the whole point of this issue, in order to assure a standard UX across all plugins). With this idea, the flag to create automatically a source field could be just another configuration value on the plugin config (as it is implemented in this patch). Or perhaps I just didn't understand your point correctly :) please indicate if that is the case.

Had some time to work a little more on this, removed some deprecates and started working on a test (not functional yet, couldn't figure out why at this point)

(Note: no twitter interdiff because it is the same patch as #21)

Great progress! Few more thoughts.

1. +++ b/src/Entity/MediaBundle.php
   @@ -241,4 +246,62 @@ class MediaBundle extends ConfigEntityBundleBase implements MediaBundleInterface
   +      if (!$this->isSyncing() && empty($this->getTypeConfiguration()['source_field']) && !empty($this->getTypeConfiguration()['create_source_field'])) {
   ...
   +        $default_field = $instance->getDefaultSourceField();
   

   Bundle should not assume which configuration variables are on the plugin. Could we rely on the return value of getDefaultSourceField(). If something is in there create fields and skip if not.

   We also have to figure out how we detect auto creation of fields and set "source_field" on the type configuration.

2. +++ b/src/MediaTypeInterface.php
   @@ -85,4 +85,26 @@ interface MediaTypeInterface extends PluginInspectionInterface, ConfigurablePlug
   +  /**
   +   * Provide a default source field.
   +   *
   +   * Plugins defining media bundles are suggested to implement this method and
   +   * create a field that will be used as default source field, on bundle
   +   * creation. Make sure this method returns unsaved entities, once these are
   +   * only going to be saved and used if the user marked the option to use the
   +   * default field provided.
   +   *
   +   * @return array
   +   *   An associative array containing the following keys:
   +   *   'storage' => \Drupal\field\Entity\FieldStorageConfig A config entity for
   +   *   this field storage.
   +   *   'field_name' => (string) The name to be used for the field instance.
   +   *   'label' => (string) The label to be used for the field instance.
   +   *   'field_widget' => (string) The id of the widget to use.
   +   *   'field_formatter' => (string) The id of the formatter to use.
   +   *   Note that the storage config entity need to be returned unsaved, and the
   +   *   main form will be responsible for saving them.
   +   */
   +  public function getDefaultSourceField();
   +
   

   Since this is on the main interface all plugins need to implement it. It covers 95% single source field problem, but it doesn't cover a hypothetical multiple fields situation. Could we change this function to return a list of fields instead of one. This should solve everything that we need.

Thank you Janez!

Tried to address all the feedback. Not sure though if the

        \Drupal::configFactory()
          ->getEditable('media_entity.bundle.' . $this->id())
          ->set('type_configuration', $configuration)
          ->save();

is the best approach to save the source field name into the plugin config...

Minor tweaks

1. +++ b/src/Entity/MediaBundle.php
   @@ -44,6 +48,7 @@ use Drupal\media_entity\MediaInterface;
   + *     "create_source_field",
   
   +++ b/tests/modules/media_entity_test_type/src/Plugin/MediaEntity/Type/TestType.php
   @@ -32,6 +48,7 @@ class TestType extends Generic {
   +      'create_source_field' => TRUE,
   

   I feel that we don't need this any more now that we rely on the return value for the function.

2. +++ b/src/Entity/MediaBundle.php
   @@ -241,4 +246,72 @@ class MediaBundle extends ConfigEntityBundleBase implements MediaBundleInterface
   +      $media_type_manager = \Drupal::service('plugin.manager.media_entity.type');
   +      $configuration = $this->getTypeConfiguration();
   +      /** @var \Drupal\media_entity\MediaTypeInterface $instance */
   +      $instance = $media_type_manager->createInstance($this->getType()->getPluginId(), $configuration);
   

   $this->getType() should be sufficient for this.

3. +++ b/src/Entity/MediaBundle.php
   @@ -241,4 +246,72 @@ class MediaBundle extends ConfigEntityBundleBase implements MediaBundleInterface
   +          // Save the field storage and create the instance.
   +          $field_details['storage']->save();
   +          FieldConfig::create([
   +            'entity_type' => 'media',
   +            'field_name' => $field_details['field_name'],
   +            'label' => $field_details['label'],
   +            'required' => TRUE,
   +            'bundle' => $this->id(),
   +          ])->save();
   +
   +          // Make the field visible on the form display.
   +          /** @var \Drupal\Core\Entity\Display\EntityFormDisplayInterface $form_display */
   +          $form_display = EntityFormDisplay::create([
   +            'targetEntityType' => 'media',
   +            'bundle' => $this->id(),
   +            'mode' => 'default',
   +            'status' => TRUE,
   +          ]);
   +          $form_display->setComponent($field_details['field_name'], [
   +            'type' => $field_details['widget'],
   +          ])->save();
   +
   +          // Make the field visible on the media entity itself.
   +          /** @var \Drupal\Core\Entity\Display\EntityViewDisplayInterface $display */
   +          $display = EntityViewDisplay::create([
   +            'targetEntityType' => 'media',
   +            'bundle' => $this->id(),
   +            'mode' => 'default',
   +            'status' => TRUE,
   +          ]);
   +          $display->setComponent($field_details['field_name'], [
   +            'type' => $field_details['formatter'],
   +          ])->save();
   

   Plugin should have control at least over field instance configuration if not also over view and form displays. If some custom field type would be used there might be configuration values we can not assume here.

4. +++ b/src/Entity/MediaBundle.php
   @@ -241,4 +246,72 @@ class MediaBundle extends ConfigEntityBundleBase implements MediaBundleInterface
   +          $configuration[$source_key] = $field_details['field_name'];
   +        }
   +
   +        // Save the source field(s) name(s) to the plugin type configuration.
   +        \Drupal::configFactory()
   +          ->getEditable('media_entity.bundle.' . $this->id())
   +          ->set('type_configuration', $configuration)
   +          ->save();
   

   Could plugin, instead of relying on array keys, return full configuration for itself if the default fields are going to be used.

   This was of saving is not the nicest. Something like

   $this->getType()->setConfiguration($foo);
   $this->save()
   

   would be best but I am not sure if we are supposed to save the entity again here. We might want to check with someone that is more familiar with Entity API.

5. +++ b/src/MediaTypeInterface.php
   @@ -85,4 +85,32 @@ interface MediaTypeInterface extends PluginInspectionInterface, ConfigurablePlug
   +  public function getDefaultSourceField();
   

   Would probably make sense to rename to getDefaultSourceFields()

Thanks! one doubt though:

Plugin should have control at least over field instance configuration if not also over view and form displays. If some custom field type would be used there might be configuration values we can not assume here.

In #5 the approach proposed was to make the plugin create an unsaved field and then Media Entity would be responsible for saving it, but now that the checkbox is on the plugin side, does it still make sense? Is there a problem in moving all this logic to the plugin method, and on the ::postSave() we would just call something like ::createDefaultSourceFields() (which would actually create the field(s) only if the checkbox is marked) ?

On the other hand, the $this->save() inside the postSave() doesn't work, I got exceptions when trying to do that :) so the only way I could come up with was the ->getEditable() way, but I agree that it is definitely not the best solution...

This comment made me think if we should rather rename this to onBundleCreate() or something along those lines and let type plugins do whatever they need to do (including creating fields). This way we avoid any assumptions re fields and prevent ourselves from adding another callback next time when we come across some other similar requirement.

This won't fix the saving problem, but we can go with the current approach in the worst case scenario I guess.

@slashrsm: What about using hook_ENTITY_TYPE_insert()?

I'd assume that we'd have same problem there since it is run just after the postSave() we're currently using.

@marcoscano: What kind of errors were you getting?

The exception I got with the ->save() approach is:

Drupal\Core\Config\ConfigNameException: The machine name of the 'Media bundle' bundle cannot be changed. in Drupal\Core\Config\Entity\ConfigEntityBundleBase->preSave() (line 90 of /var/www/d8media0/web/core/lib/Drupal/Core/Config/Entity/ConfigEntityBundleBase.php).

Concerning the comment int #30, yes.. perhaps leaving everything to be done on the plugin side will end up being the cleanest solution after all. The only drawback I see is that we cannot enforce a common UX accross all plugin types, because some may implement the auto-generation of the source field, and some others may not. But it is difficult to assure a standard workflow anyways, once in theory the plugins may have different behavior (more than one source fields, etc)

Starting over. This new approach delegates all the logic to the plugins.

Concerning the save of the source_field value, is it really necessary? This new patch uses the same strategy used currently in video_embed_field, where if no value is set on $this->configuration['source_field'], then we assume we have created a new field and its name is self::MEDIA_ENTITY_PLUGINNAME_DEFAULT_FIELD_NAME. Functionally it works, but I'm unsure of any side effects or edge cases we might miss by doing this.

Also, the test needs some more love, still WIP.

Twitter patch needed a reroll (the media_entity one is just a copy of 35). There were conflicts in the use clause of Twitter.php. Besides some actual conflicts (that didn't seem to hard to resolve) I noticed the previous version of the patch moved the line:

use Drupal\Component\Serialization\Json;

while it was removed entirely in the solution to #2650976: Use SVG to generate thumbnails for text-only tweets. The reroll would add it back at the top, but the removal seems legitimate (although unrelated to that specific issue) so I haven't added it back here.

Hi all. @chr.fritsch asked me to have a look at this issue for the #dcmuc16 code sprint.

I would hesitate to make assumptions about the source field based on the value being empty; as soon as the default is created, I would revert back to the "old" behaviour where the field is just configured as the source field, as any other field. I would actually advocate for an empty value meaning "no source field selected yet", with an appropriate label in the dropdown. This might make sense when the user opted to not create a default field, but has not yet created the field they intend to use.

I do wonder about storing the create_source_field value, it doesn't really do anything once the bundle has been created. As it stands, it would be fine if it just triggers the creation of the default field and then is forgotten. I would also go one step further and nog actually show the source field dropdown on initial creation, since there won't be anything to be selectable there for sure.

When it might make sense to store the create_source_field value (although the naming might need to be reconsidered) is if we want to allow the user to change this afterwards (or at least as some sort of indication that the bundle was created with the default field, initially). However, I think this will be a scenario filled with headaches surrounding what exactly to do when the user actually starts switching the setting. Should we force it back on if the user doesn't actually choose another field? What if another field was selected, and the setting to use the default is then switched on again? Do we need to create the default field again if it was removed? It would seem to be a lot more manageable if the switch just handles initial creation, and afterwards just reverts back to the standard behaviour with the dropdown. Full circle: in that case we do not need to actually store it anywhere.

With regard to storing the source field, this seems to happen already, although I am not sure how, exactly. However, when I generate a new bundle and immediately export the configuration afterwards, there it is.

Attached is a new patch for m_e_twitter and another copy of the same m_e patch, just to have the complete set. Changes I've implemented:

- Do not show the dropdown on initial form creation
- Add a "- None -" option to the dropdown, just so the dropdown is not empty when there are no fields to select yet
- Show the checkbox exclusively based on whether the bundle is new

WRT where this should live, I want to have a look to see if this can be isolated into a trait. It seems a bad idea to have each plugin implement practically the same logic just because we're not sure it is appropriate for *all* plugins.

I did not dig further into not storing the create_source_field config value. Once it's in the form, it will get stored, so it would take effort to not save it, which seems unnecessary. If anything, it records the fact that the bundle was initially created with the default field. However, feel free to disagree.

Here's a stab at moving most stuff from the Twitter class into a trait. I'm curious to hear whether this makes any sense as an approach, or whether I am overlooking something as to why this wouldn't work (such as the field definition stuff is not generic enough).

From the UX workflow, this is much better compared with what we have currently in media_entity.

+++ b/src/MediaTypeDefaultFieldTrait.php
@@ -0,0 +1,161 @@
+  abstract public function defaultSourceFieldName();

I think we don't need that. Source field name could be generated from 'field_media' . providerId

+++ b/src/MediaTypeDefaultFieldTrait.php
@@ -0,0 +1,161 @@
+    if (!$storage = FieldStorageConfig::loadByName('media', $this->defaultSourceFieldName())) {
+      $storage = FieldStorageConfig::create([
+        'field_name' => $this->defaultSourceFieldName(),
+        'entity_type' => 'media',
+        'type' => $this->defaultSourceFieldType(),
+      ]);
+      $storage->save();
+    }

+++ b/tests/modules/media_entity_test_type/src/Plugin/MediaEntity/Type/TestType.php
@@ -32,6 +51,7 @@ class TestType extends Generic {
+      'create_source_field' => TRUE,

+++ b/tests/modules/media_entity_test_type/src/Plugin/MediaEntity/Type/TestType.php
@@ -45,7 +65,96 @@ class TestType extends Generic {
+    $options = [];
+    $allowed_field_types = ['string'];
+    /** @var \Drupal\media_entity\MediaBundleInterface $bundle */
+    $bundle = $form_state->getFormObject()->getEntity();
+    foreach ($this->entityFieldManager->getFieldDefinitions('media', $bundle->id()) as $field_name => $field) {
+      if (in_array($field->getType(), $allowed_field_types) && !$field->getFieldStorageDefinition()->isBaseField()) {
+        $options[$field_name] = $field->getLabel();
+      }
+    }
+
+    $form['source_field'] = array(
+      '#type' => 'select',
+      '#title' => $this->t('Field with source information'),
+      '#description' => $this->t('Field on media entity that stores the source information. You can create a bundle without selecting a value for this dropdown initially. This dropdown can be populated after adding fields to the bundle.'),
+      '#default_value' => empty($this->configuration['source_field']) ? self::MEDIA_ENTITY_TEST_TYPE_DEFAULT_FIELD_NAME : $this->configuration['source_field'],
+      '#options' => $options,
+    );
+
+    // Add a checkbox to allow the field being created automatically on save.
+    if (empty($this->configuration['source_field'])) {
+      $form['create_source_field'] = [
+        '#type' => 'checkbox',
+        '#title' => $this->t('Create a source field automatically when saving this form.'),
+        '#description' => $this->t('If checked, a default field will be created and used as a source field. You can change this setting later.'),
+        '#default_value' => $this->configuration['create_source_field'],
+        '#access' => $bundle->isNew(),
+      ];
+    }
+

I implemented these changes in the attached patches. One other change I did is remove the createDefaultSourceField() method from the MediaTypeInterface again. I think it is overly restrictive now that we have the reactToBundleCreate(); its point is that a media type *might* need to do something different than create a single default source field. The trait seems to provide enough guidance for those that do not wish to use it but do their own thing.

I am including interdiffs, but it is actually quite illustrative to just look at the patches; the changes to the Twitter class as well as the TestType class are quite minimal with most of the heavy lifting going into the trait.

I have an implementation for media_entity_image ready to go as well, I'll create an issue over there.

Small tweak, the trait was not doing one essential thing; not creating the field when the checkbox is disabled. So it as always creating the field, regardless of the setting of the checkbox.

Great work! General direction looks very nice. It might seem like I had comments on everything, but they are mostly cosmetic. Overall approach look fine to me.

1. +++ b/src/Entity/MediaBundle.php
   @@ -241,4 +242,17 @@ class MediaBundle extends ConfigEntityBundleBase implements MediaBundleInterface
   +    if (!$update) {
   +      // Inform the plugin that this bundle has just been created.
   +      $this->getType()->reactOnBundleCreated($this->id(), $this->getEntityTypeId());
   +    }
   

   This is happening on save and since create is different operation it would make sense to rename this to reactOnBundleSave()

2. +++ b/src/MediaTypeBase.php
   @@ -158,4 +158,14 @@ abstract class MediaTypeBase extends PluginBase implements MediaTypeInterface, C
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function reactOnBundleCreated($bundle_name, $entity_type_id) {}
   

   Entity type should always be media bundle. I don't think that we need this argument.

3. +++ b/src/MediaTypeBase.php
   @@ -158,4 +158,14 @@ abstract class MediaTypeBase extends PluginBase implements MediaTypeInterface, C
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function createDefaultSourceField($bundle_name) {}
   

   This function won't make much sense if the trait is not used. Since it is defined there I don't think that we need to define it here too.

4. +++ b/src/MediaTypeDefaultFieldTrait.php
   @@ -0,0 +1,214 @@
   +  abstract public function allowedSourceFieldTypes();
   ...
   +  abstract public function defaultSourceFieldType();
   ...
   +  abstract public function defaultSourceFieldLabel();
   ...
   +  abstract public function defaultSourceFieldFormatter();
   ...
   +  abstract public function defaultSourceFieldWidget();
   

   All this functions should be protected IMO. I'd also consider joining them into one that would return all information as an array.

5. +++ b/src/MediaTypeDefaultFieldTrait.php
   @@ -0,0 +1,214 @@
   +    $options = ['' => $this->t('- None -')];
   

   #empty_value/#empty_key can be used on #type => select.

6. +++ b/src/MediaTypeDefaultFieldTrait.php
   @@ -0,0 +1,214 @@
   +      if (in_array($field->getType(), $allowed_field_types) && !$field->getFieldStorageDefinition()
   +          ->isBaseField()
   +      ) {
   

   No need to break. Let's make this condition one-liner.

7. +++ b/src/MediaTypeDefaultFieldTrait.php
   @@ -0,0 +1,214 @@
   +    $form['source_field'] = array(
   

   Consistently use short array syntax.

8. +++ b/src/MediaTypeDefaultFieldTrait.php
   @@ -0,0 +1,214 @@
   +      '#access' => !$bundle->isNew(),
   

   Would it make sense to use #disabled instead? To make this configuration visible but not alterable?

9. +++ b/src/MediaTypeDefaultFieldTrait.php
   @@ -0,0 +1,214 @@
   +    if (isset($this->defaultSourceFieldName)) {
   

   This sounds like "I am checking if name was set" and say nothing about when and how it should be. It would be nicer if it would say "Should field be created on create"?

   Can we add a comment and @see to determineDefaultSourceFieldName().

10. +++ b/src/MediaTypeDefaultFieldTrait.php
    @@ -0,0 +1,214 @@
    +  public function createDefaultSourceField($bundle_name) {
    

    This should be protected I think.

11. +++ b/src/MediaTypeDefaultFieldTrait.php
    @@ -0,0 +1,214 @@
    +    $storage = FieldStorageConfig::create([
    +      'field_name' => $field_name,
    +      'entity_type' => 'media',
    +      'type' => $this->defaultSourceFieldType(),
    +    ]);
    +    $storage->save();
    +
    +    // Create the field instance.
    +    FieldConfig::create([
    +      'entity_type' => 'media',
    +      'field_name' => $field_name,
    +      'label' => $this->defaultSourceFieldLabel(),
    +      'required' => TRUE,
    +      'bundle' => $bundle_name,
    +    ])->save();
    

    Both could have custom settings depending on the type used.

    If we do what I commented on above we could allow plugins to include this settings into that unified function as well.

12. +++ b/src/MediaTypeDefaultFieldTrait.php
    @@ -0,0 +1,214 @@
    +    // Make the field visible on the media entity itself.
    +    /** @var \Drupal\Core\Entity\Display\EntityViewDisplayInterface $display */
    +    $display = EntityViewDisplay::create([
    +      'targetEntityType' => 'media',
    +      'bundle' => $bundle_name,
    +      'mode' => 'default',
    +      'status' => TRUE,
    +    ]);
    +    $display->setComponent($field_name, [
    +      'type' => $this->defaultSourceFieldFormatter(),
    +    ])->save();
    

    I wouldn't make any assumptions about that (I think that we can skip it).

13. +++ b/src/MediaTypeDefaultFieldTrait.php
    @@ -0,0 +1,214 @@
    +    return $this->defaultSourceFieldName = $field_name_candidate;
    

    This is correct and it works, but it is very non-conventional. Can we assign first and then return?

14. +++ b/tests/modules/media_entity_test_type/src/Plugin/MediaEntity/Type/TestType.php
    @@ -32,6 +73,7 @@ class TestType extends Generic {
    +      'create_source_field' => TRUE,
    

    Is this intended change? We are not storing this value anywhere else.

15. +++ b/tests/modules/media_entity_test_type/src/Plugin/MediaEntity/Type/TestType.php
    @@ -45,6 +87,10 @@ class TestType extends Generic {
    +    $form = $this->defaultFieldConfigurationForm($form, $form_state);
    +
    +    $form['source_field']['#description'] = $this->t('Field on media entity that stores the source information.');
    

    It would be nice to have more test coverage. Not just form but also actual creation of the field.

EDIT: Feel free to send every patch through Drupal CI. There is no reason not to do that.

Thanks for all that, glad you like the approach. I am currently working on the test coverage (I noticed late yesterday I had inadvertently removed @marcoscano's preliminary tests, I am in the process of restoring it and expanding), but am getting stuck running Javascript tests, unfortunately. Seems to be something with my environment, because BrowserTestBase's drupalLoging method is failing, for some reason.

Here's most of the above points addressed. I couldn't get tests to work correctly yet. I do have all greens running test on dev, though, so my test env is OK now, but with this patch the test environment is running into a 500 when saving the bundle. Maybe the testbot over here will produce some helpful hints.

Some comments to specific points from #48. In all cases, don't hesitate to let me know if you disagree.

1. I actually went one step further and renamed it to "onBundleSave". on[Event] is a pretty common naming pattern, so I suppose it is appropriate here too.
4. For now, I have done as suggested, this was a point I was uncertain of myself. However, it now struck me this seems to be the sort of thing annotations were invented for. I dipped my toe in getting something going for that, but didn't go very deep because I couldn't quickly figure out what is the best way to read annotation information for the class itself. Any comments?
6. I pulled it apart as suggested in Coding Standards > Line length and wrapping
8. Not sure. I did consider doing something like it, but what should the select contain? Should it show the field label for the default field as it would create it? Or should it just be "- None -"? If not, it should get an ajax update when the user unchecks the checkbox. This seems to have some UX implications, while at the code sprint last week it was suggested the flow might actually be turned upside down. Maybe this needs some more UX scrutiny that we may want to pull into another issue.
12. I think not adding the field into the display will have many puzzled for a moment or two. If the field is not displayed at all, the user will quickly find their way to change it. @marcoscano, you originally added this, do you have any input?

Something went wrong uploading the patches. Another try.

One thing I forgot to mention, I renamed some stuff from DefaultField to SourceField. E.g. the trait (now MediaTypeSourceFieldTrait) and the method that generates the configuration form fragment (sourceFieldConfigurationForm()). Because basically, this implements everything for configuring a single source field.

Forgot to rename the configuration form in the test type.

Unassigning for now, not sure when I might find time to continue with this. Maybe next weekend. Anyone feel free to pick this up further, especially getting the tests going properly. Also, input on the annotation-idea (point 4 above) is very welcome.

Here is try to unstuck tests. Interdiff available to check changes.

Awesome. I think we should also have a test to make sure the field is *not* created when the checkbox is unchecked. That's a mistake I made (and caught before creating a patch) while working on this.

@eelkeblok thank you for feedback. I agree that it would be nice to check that too and here is patch with testing of creation of bundle without default field.

It simply checks that there are no fields created. I thought to add checking of combobox for source field selection, but it looks to me like duplicate (pointless) check, since there is nothing to be displayed in combobox if there are no fields.

I have also checked that test is compatible with PhantomJS and Selenium.

1. +++ b/src/MediaTypeSourceFieldTrait.php
   @@ -0,0 +1,228 @@
   +  protected $defaultSourceFieldName;
   

   Missing docblock.

2. +++ b/src/MediaTypeSourceFieldTrait.php
   @@ -0,0 +1,228 @@
   +  abstract protected function t($string, array $args = array(), array $options = array());
   

   Please be consistent and stick with short array syntax.

Thanks everyone for moving this forward!

I have updated some of the last CS-related feedbacks, but not sure if I missed some more important point that is still pending.

Hopefully we will be able to get this over the finish line next week in Berlin.

Consulting with @slashrsm, I decided to refactor this so it's not doing the onBundleCreate() thing -- that's never been seen in core. I decided instead to have type plugins implicitly declare their support for source fields by implementing a new SourceFieldInterface, which is responsible for guaranteeing the field definition of the type's source field. The MediaBundle entity, on the other hand, is responsible for actually saving the field definition and updating all relevant entity displays.

Oh for Zeus' sake. I meant to upload the interdiff, not patch #64.

Fixed a load of bugs. Hopefully this will at least run the tests, if not pass them.

This will hopefully fix the tests; I also added the ability to re-use existing field storage definitions that are present on the Media entity type. Creating a new bundle will always necessitate a new field (which is no different from the way things already work), but we'll want to be able to re-use existing storages.

Schema error, eh? Okay, let's see if this helps.

This. This will pass the tests. At least, it passes on localhost. Works on my machine. Signed, sealed, and delivered.

Special thanks to @dawehner for figuring out a particularly tricky failure.

+++ b/src/MediaTypeBase.php
@@ -138,7 +140,28 @@ abstract class MediaTypeBase extends PluginBase implements MediaTypeInterface, C
+    // Select the source field. Only show it when the bundle is not new, so
+    // there will potentially be fields to select.
+    $form['source_field'] = [
+      '#type' => 'select',
+      '#title' => $this->t('Field with source information.'),
+      '#default_value' => $this->configuration['source_field'],
+      '#empty_option' => $this->t('- create -'),
+      '#empty_value' => NULL,
+      '#options' => $options,
+      '#disabled' => empty($options),
+    ];

Lets hide the field, if its not actionable. People don't necessarily see the real fields vs. the usually light gray disabled fields, and they click on it anyway but it does not work :) If we are to create it anyway, we can hide it.

Also lets have Create instead of lowercasing it like these:

core/lib/Drupal/Core/Render/Element/Select.php:          '#empty_option' => $required ? t('- Select -') : t('- None -'),
core/modules/content_moderation/src/Form/ModerationStateTransitionForm.php:      '#empty_option' => $this->t('-- Select --'),
core/modules/content_moderation/src/Form/ModerationStateTransitionForm.php:      '#empty_option' => $this->t('-- Select --'),
core/modules/field_ui/src/Form/FieldStorageAddForm.php:      '#empty_option' => $this->t('- Select a field type -'),
core/modules/field_ui/src/Form/FieldStorageAddForm.php:        '#empty_option' => $this->t('- Select an existing field -'),
core/modules/image/src/Form/ImageStyleDeleteForm.php:        '#empty_option' => $this->t('- No replacement -'),

Good call. Fixed!

Just a couple of comments ...

1. +++ b/src/Entity/MediaBundle.php
   @@ -241,4 +244,57 @@ class MediaBundle extends ConfigEntityBundleBase implements MediaBundleInterface
   +  public function preSave(EntityStorageInterface $storage) {
   ...
   +  public function postSave(EntityStorageInterface $storage, $update = TRUE) {
   

   Maybe having a quick comment what we do and why would be neat.

2. +++ b/src/MediaTypeBase.php
   @@ -138,7 +140,27 @@ abstract class MediaTypeBase extends PluginBase implements MediaTypeInterface, C
   +      $allowed_type = in_array($field->getType(), $this->pluginDefinition['allowed_field_types']);
   

   For string comparisons its alawys just better to use strict checking, because of this niceness: https://3v4l.org/9k0Bt

3. +++ b/src/MediaTypeBase.php
   @@ -158,4 +180,85 @@ abstract class MediaTypeBase extends PluginBase implements MediaTypeInterface, C
   +    $id = 'media.' . $bundle->id() . '.' . $this->configuration['source_field'];
   +
   +    return $this->entityTypeManager
   +      ->getStorage('field_config')
   +      ->load($id)
   +      ?:
   ...
   +  protected function getSourceFieldStorage() {
   +    if ($this->configuration['source_field']) {
   +      $id = 'media.' . $this->configuration['source_field'];
   +
   +      return $this->entityTypeManager
   +        ->getStorage('field_storage_config')
   +        ->load($id);
   +    }
   +    else {
   

   Can't you use the entity field manager instead? \Drupal\Core\Entity\EntityFieldManagerInterface::getFieldDefinitions and get it from there? There are more areas in this particular patch

4. +++ b/src/Plugin/MediaEntity/Type/Generic.php
   @@ -50,4 +54,32 @@ class Generic extends MediaTypeBase {
   +        // Strings are harmless, inoffensive puppies: a good choice for a
   +        // generic media type.
   +        'type' => 'string',
   

   Hehe,

5. +++ b/src/SourceFieldInterface.php
   @@ -0,0 +1,20 @@
   +interface SourceFieldInterface {
   

   Should this extend the existing media type interface?

6. +++ b/tests/modules/media_entity_test_type/src/Plugin/MediaEntity/Type/TestType.php
   @@ -11,7 +11,8 @@ use Drupal\media_entity\Plugin\MediaEntity\Type\Generic;
     *   label = @Translation("Test type"),
   - *   description = @Translation("Test media type.")
   + *   description = @Translation("Test media type."),
   + *   allowed_field_types = {"string"}
     * )
   

   If you are here you could add a trailing comma, so future changes will require less diff size

7. +++ b/tests/src/FunctionalJavascript/BundleCreationTest.php
   @@ -0,0 +1,106 @@
   +    // Create a new bundle, which should create a new field we can reuse.
   +    MediaBundle::create([
   +      'id' => 'pastafazoul',
   +      'label' => 'Pastafazoul',
   +      'type' => 'generic',
   +    ])->save();
   +
   

   Can we use the UI here, less issues ...

All fixed. Thanks, @dawehner!

Thank you @phenaproxima

For me this looks pretty good, it would be neat to get another opinion, but I'm totally fine with it.

1. +++ b/src/Entity/MediaBundle.php
   @@ -241,4 +244,71 @@ class MediaBundle extends ConfigEntityBundleBase implements MediaBundleInterface
   +      // Store the field name.
   +      $configuration = $type_plugin->getConfiguration();
   +      $configuration['source_field'] = $storage->getName();
   +      $this->setTypeConfiguration($configuration);
   

   Won't this be done in the plugin already?

   Not relevant.

2. +++ b/src/MediaTypeBase.php
   @@ -158,4 +180,77 @@ abstract class MediaTypeBase extends PluginBase implements MediaTypeInterface, C
   +    return isset($fields[$field]) ? $fields[$field] : $this->createSourceFieldStorage();
   

   What will happen if field that does not exist is saved in 'source_field'? This will create it, but won't necessarily respect the name.

   In which situations can this execution patch happen?

3. +++ b/tests/modules/media_entity_test_type/src/Plugin/MediaEntity/Type/TestType.php
   @@ -39,12 +40,16 @@ class TestType extends Generic {
   +    $form['source_field']['#description'] = $this->t('Field on media entity that stores the source information.');
   

   Could this be added to the base class? Would be nice to have a description anyway?

+++ b/src/Entity/MediaBundle.php
@@ -241,4 +244,71 @@ class MediaBundle extends ConfigEntityBundleBase implements MediaBundleInterface
+    if ($type_plugin instanceof SourceFieldInterface) {
+      $storage = $type_plugin->getSourceField($this)->getFieldStorageDefinition();
+      // If the field storage is a new (unsaved) config entity, save it.
+      if ($storage instanceof FieldStorageConfigInterface && $storage->isNew()) {
+        $storage->save();
+      }
+      // Store the field name.
+      $configuration = $type_plugin->getConfiguration();
+      $configuration['source_field'] = $storage->getName();
+      $this->setTypeConfiguration($configuration);
+    }
...
+    if ($type_plugin instanceof SourceFieldInterface) {
+      $field = $type_plugin->getSourceField($this);
+
+      // If the field is new, save it and add it to this bundle's view and form
+      // displays.
+      if ($field->isNew()) {
+        // Ensure the field is saved correctly before adding it to the displays.
+        $field->save();
+
+        $entity_type = $field->getTargetEntityTypeId();
+        $bundle = $field->getTargetBundle();
+
+        if ($field->isDisplayConfigurable('form')) {
+          // Use the default widget and settings.
+          $component = \Drupal::service('plugin.manager.field.widget')
+            ->prepareConfiguration($field->getType(), []);
+
+          entity_get_form_display($entity_type, $bundle, 'default')
+            ->setComponent($field->getName(), $component)
+            ->save();
+        }
+        if ($field->isDisplayConfigurable('view')) {
+          // Use the default formatter and settings.
+          $component = \Drupal::service('plugin.manager.field.formatter')
+            ->prepareConfiguration($field->getType(), []);
+
+          entity_get_display($entity_type, $bundle, 'default')
+            ->setComponent($field->getName(), $component)
+            ->save();
+        }
+      }
+    }

Let's wrap this two in isNew()

Discussed with @slashrsm, and we decided to not really change preSave() or the field handling stuff in MediaTypeBase. Although we did decide to add test coverage.

The reason we decided to leave things be is because the code, as written, is (by design) hyper-defensive. This means a few things:

• Bundles should always behave consistently, regardless of whether they are manipulated programmatically or via a UI.
• Creating or updating a bundle without a specified source field should create a field with an auto-generated name.
• Creating or updating a bundle with a specified source field should use the field if it exists, or create it with the specified name if it doesn't.

So, for example, if you have an existing bundle that used an automatically generated, automatically named field, and you change it to use a field that already existed, it should switch to the new field without trying to create it anew. If you did the opposite -- switch from an existing field to an unspecified one -- it should automatically create a new field for you. If you do the same thing, but specify the field's name, it should automatically generate the field with that name.

It's trying to account for a lot of edge cases, and the test coverage is now expanded to account for those. I wouldn't put anything past our users, and therefore prefer to write paranoid logic.

A minor change -- made getSourceFieldName() use the entity field manager's field map, rather than trying to load fields from storage.

Committed #82. Thank you all!

I'd still like to make the changes in #83, but I'll target those for the core version of Media Entity, which might appreciate the changes a little more ;-)

I decide to revert that since it breaks existing plugins. The plan for now is to keep this a core-only feature. Plugins will need to upgrade to the core version of the media entity anyway.

Will leave this open until #2831274: Bring Media entity module to core as Media module lands.

This doesn't seem to be happening in this module anymore.