Add an alter hook for the pre-configured field UI options and implement it in the Media module

Looks great to me! Just needs tests.

+++ b/core/modules/media/media.module
@@ -96,3 +98,42 @@ function template_preprocess_media(array &$variables) {
+  $fields = \Drupal::service('entity_field.manager')->getFieldStorageDefinitions($display->getTargetEntityTypeId());
+  $fields = array_filter($fields, function (FieldStorageDefinitionInterface $field) {
+    return $field->getType() == 'entity_reference' && $field->getSetting('target_type') === 'media';
...
+  if (isset($display->original)) {
+    $new_components = array_diff_key($display->getComponents(), $display->original->getComponents());

I'm just wondering, can we change the order of checking for new fields and entity reference field of type media?

I think the check for media fields is a little more expensive. We could probably return faster when there are no new components.

1. +++ b/core/modules/media/media.module
   @@ -96,3 +98,42 @@ function template_preprocess_media(array &$variables) {
   +    return $field->getType() == 'entity_reference' && $field->getSetting('target_type') === 'media';
   

   In general we try not to hardcode the 'entity_reference' field type, but use the class and its descendants instead. See field_field_config_presave() for an example.

2. +++ b/core/modules/media/media.module
   @@ -96,3 +98,42 @@ function template_preprocess_media(array &$variables) {
   +      'settings' => [
   +        'view_mode' => 'default',
   +        'link' => FALSE,
   +      ],
   

   These are automatically provided by setComponent() so they can be removed.

3. +++ b/core/modules/media/media.module
   @@ -96,3 +98,42 @@ function template_preprocess_media(array &$variables) {
   +      'region' => 'content',
   

   Is there any reason for not using the region that's already configured in the view display?

However, there is a much bigger problem with the current patch: doing it like this in a presave hook means that we always override the formatter for a newly added component, even when some code sets a specific formatter via ->setComponent($field_name, ['type' => 'my_custom_formatter']).

Which makes we wonder.. why are we not doing what was proposed in #2831943-123: Use "rendered media" (not links) as default media field formatter; add modal to configure the used media view mode instead?

I like the overall approach in #9. However, I wonder if we need to do it by way of yet another alter hook.

What about this -- could we add a 'common_reference_options' array to the Media entity type annotation, and change EntityReferenceItem::getPreconfiguredOptions() to use that instead?

That way, the common options are still alterable in hook_entity_type_build() or hook_entity_type_alter(), and we don't need to introduce a new hook.

Yes, in the pargraphs module, we have this in the annotation:

*   default_reference_revision_settings = {
 *     "field_storage_config" = {
 *       "cardinality" = -1,
 *       "settings" = {
 *         "target_type" = "paragraph"
 *       }
 *     },
 *     "field_config" = {
 *       "settings" = {
 *         "handler" = "default:paragraph"
 *       }
 *     },
 *     "entity_form_display" = {
 *       "type" = "entity_reference_paragraphs"
 *     },
 *     "entity_view_display" = {
 *       "type" = "entity_reference_revisions_entity_view"
 *     }
 *   }

And that just works, thanks to \Drupal\entity_reference_revisions\Plugin\Field\FieldType\EntityReferenceRevisionsItem::getPreconfiguredOptions. The only problem is with re-using existing fields, but there is an issue for that: #2717319: Provide better default configuration when re-using an existing field.

To be honest, I'd prefer the hook approach. #11 shows that's quite a lot of stuff to put in the entity annotation. But I don't have any strong feelings about it so I'll defer to whatever is preferred by the majority :)

#11 shows that's quite a lot of stuff to put in the entity annotation.

Yes, in the worst-case scenario. But I imagine that most uses of this functionality would be for very small tweaks that would not add much to the entity annotation at all. So I think it might ultimately be less verbose than a hook.

Also, to be clear, the approach I suggest can still be hook-based (hook_entity_type_build() and hook_entity_type_alter()). I simply don't favor the idea of adding a new hook just for this. But I can probably be convinced otherwise.

I don't favor to add a new hook for this, too. And I would go with the annotations if it doesn't blow them up heavily.

For most media sources there would be no special settings, for the cases there are, having some extra annotations would be acceptable for me. It's very clear to have everything together.

Looking at https://www.drupal.org/files/issues/referencing_media_entities.mp4 this is shaping up very nicely.

For an even better default image media view mode I would suggest to order media before name. First show the cat, then show its name :)

thank you @marcoscano, that's very much its own issue then indeed.

As I said before, I'm a fan of the alter hook :) I should have done it myself when I wrote the "preconfigured options" stuff years ago, but there were too many things to do/fix at that time..

I'm convinced, let go with the alter hook. It's useful for at least 2 media related issue at the moment. As mentioned in slack, changing the default form/view display for entity references doesn't sound like an edge case at all. Thanks for taking the time to discuss this marcoscano!

This looks good and straightforward; just a few small cleanliness things.

1. +++ b/core/modules/field/field.api.php
   @@ -59,6 +59,24 @@ function hook_field_info_alter(&$info) {
   + * @param array $options
   + *   Array of options as retrieved from ::getPreconfiguredOptions().
   + *
   + * @see \Drupal\Core\Field\PreconfiguredFieldUiOptionsInterface::getPreconfiguredOptions()
   + */
   +function hook_field_preconfigured_options_alter(array &$options, array $field_definition) {
   

   $field_definition should also be mentioned in the doc block.

2. +++ b/core/modules/field_ui/src/Form/FieldStorageAddForm.php
   @@ -312,14 +312,19 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
   +        \Drupal::moduleHandler()->alter('field_preconfigured_options', $options, $field_definition);
   

   Is it possible to inject the module handler?

3. +++ b/core/modules/field_ui/src/Form/FieldStorageAddForm.php
   @@ -349,8 +356,16 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
   +        $this->configureEntityFormDisplay($values['field_name'], $widget_options);
   +        $formatter_options = $formatter_id ? ['type' => $formatter_id] : [];
   

   Nit: Can there be a blank line between these two "paragraphs"?

4. +++ b/core/modules/field_ui/src/Form/FieldStorageAddForm.php
   @@ -416,14 +431,14 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
   +  protected function configureEntityFormDisplay($field_name, $options = []) {
   

   $options should be type hinted.

5. +++ b/core/modules/field_ui/src/Form/FieldStorageAddForm.php
   @@ -434,14 +449,14 @@ protected function configureEntityFormDisplay($field_name, $widget_id = NULL) {
   +  protected function configureEntityViewDisplay($field_name, $options = []) {
   

   Ditto.

6. +++ b/core/modules/media/media.module
   @@ -96,3 +96,22 @@ function template_preprocess_media(array &$variables) {
   +  if (!empty($field_definition['id']) && $field_definition['id'] === 'entity_reference') {
   

   Under what circumstances would $field_definition['id'] be empty?

Kickass. Back to the land of RTBC we go.

Very nice progress, this is looking pretty good already. Here's a few points:

1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
   @@ -647,8 +647,8 @@ public static function getPreconfiguredOptions() {
   -          ]
   -        ]
   +          ],
   +        ],
   

   Unrelated changes :)

2. +++ b/core/modules/field/field.api.php
   @@ -59,6 +59,25 @@ function hook_field_info_alter(&$info) {
   + *   Array of options as returned from ::getPreconfiguredOptions().
   

   We need to use the fully qualified method name here, ::someMethodName() doesn't make too much sense in the context of an .api.php file.

3. +++ b/core/modules/field/field.api.php
   @@ -59,6 +59,25 @@ function hook_field_info_alter(&$info) {
   + * @param array $field_definition
   + *   A field plugin definition array, as returned from
   + *   \Drupal\Component\Plugin\Discovery\DiscoveryInterface::getDefinition().
   

   I'm not sure it's very useful to pass the entire $definition array, I think passing the field type is enough.

4. +++ b/core/modules/field/field.api.php
   @@ -59,6 +59,25 @@ function hook_field_info_alter(&$info) {
   +function hook_field_preconfigured_options_alter(array &$options, array $field_definition) {
   

   "Pre-configured options" is a concept of the Field UI module, not the generic Field system, so the hook name should be hook_field_ui_preconfigured_options_alter.

5. +++ b/core/modules/field/field.api.php
   @@ -59,6 +59,25 @@ function hook_field_info_alter(&$info) {
   +  if ($field_definition['id'] === 'entity_reference' && !empty($options['media'])) {
   

   Like I mentioned previously, we shouldn't hardcode the name of the entity reference field type, but get the actual class of the field type and check if the class is an instance of EntityReferenceItem.

   See field_field_config_presave() for an example.

6. +++ b/core/modules/field_ui/src/Form/FieldStorageAddForm.php
   @@ -55,6 +56,14 @@ class FieldStorageAddForm extends FormBase {
   +  protected $moduleHandler;
   +
   +
   +  /**
   

   Extra empty line here.

7. +++ b/core/modules/field_ui/src/Form/FieldStorageAddForm.php
   @@ -312,14 +325,19 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
   +        $this->moduleHandler->alter('field_preconfigured_options', $options, $field_definition);
   

   Not very happy that we need to fire the alter hook in two places.. this probably means we should have a method on FieldTypePluginManagerInterface which gets the pre-configured options for a field type and, fires the alter hook and then returns the result.

8. +++ b/core/modules/field_ui/src/Form/FieldStorageAddForm.php
   @@ -416,14 +445,14 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
   -   * @param string|null $widget_id
   -   *   (optional) The plugin ID of the widget. Defaults to NULL.
   +   * @param array $options
   +   *   (optional) An array of options for this component. Defaults to an empty
   +   *   array.
   ...
   -  protected function configureEntityFormDisplay($field_name, $widget_id = NULL) {
   +  protected function configureEntityFormDisplay($field_name, array $options = []) {
   

   How about sending a new $widget_settings parameter instead?

   Same for configureEntityViewDisplay().

Also, we should have dedicated test coverage for the new hook. We already have a dedicated field type for testing pre-configured options (\Drupal\field_test\Plugin\Field\FieldType\TestItemWithPreconfiguredOptions), so we should simply be able to alter that by implementing a hook in field_test.module and test it in \Drupal\field_ui\Tests\ManageFieldsTest::testPreconfiguredFields.

Thanks, @marcoscano, this looks much better now! Here's a few more points that could be improved:

1. +++ b/core/lib/Drupal/Core/Field/FieldTypePluginManagerInterface.php
   @@ -85,6 +85,30 @@ public function getDefaultStorageSettings($type);
   +   * Returns preconfigured field options for a field type, after being altered.
   ...
   +   *   and after being exposed for alteration by modules.
   ...
   +  public function getAlterablePreconfiguredOptions($class, $field_type);
   

   We don't really mentioned anywhere in core that a method invokes an alter hook, so I think we should remove all the mentions of 'alterable' from the method name and its documentation.

2. +++ b/core/lib/Drupal/Core/Field/FieldTypePluginManagerInterface.php
   @@ -85,6 +85,30 @@ public function getDefaultStorageSettings($type);
   +   * @param string $class
   +   *   The class implementing
   +   *   \Drupal\Core\Field\PreconfiguredFieldUiOptionsInterface::getPreconfiguredOptions().
   

   Is there any reason to have the field type class as an argument here? We're in the field type plugin manager so we can easily get the field type definition from its ID.

3. +++ b/core/lib/Drupal/Core/Field/FieldTypePluginManagerInterface.php
   @@ -85,6 +85,30 @@ public function getDefaultStorageSettings($type);
   +   *   The field type plugin id.
   

   We need to capitalize 'id' here: ".. plugin ID."

4. +++ b/core/lib/Drupal/Core/Field/PreconfiguredFieldUiOptionsInterface.php
   @@ -16,6 +16,11 @@
   +   * Note that if you want to give modules an opportunity to alter the result
   +   * of this method, you should call
   +   * \Drupal\Core\Field\FieldTypePluginManagerInterface::getAlterablePreconfiguredOptions()
   +   * instead.
   

   As a continuation to the point above, this documentation is actually useful, so let's keep it :)

5. +++ b/core/modules/field/field.api.php
   @@ -59,6 +59,35 @@ function hook_field_info_alter(&$info) {
   +  if ($class !== $item_class && !is_subclass_of($class, $item_class)) {
   

   Instead two conditions we can use is_a() here.

6. +++ b/core/modules/field_ui/src/Form/FieldStorageAddForm.php
   @@ -4,6 +4,7 @@
   +use Drupal\Core\Extension\ModuleHandlerInterface;
   
   @@ -55,6 +56,13 @@ class FieldStorageAddForm extends FormBase {
   +  protected $moduleHandler;
   
   @@ -63,11 +71,14 @@ class FieldStorageAddForm extends FormBase {
   -  public function __construct(EntityManagerInterface $entity_manager, FieldTypePluginManagerInterface $field_type_plugin_manager, ConfigFactoryInterface $config_factory) {
   +  public function __construct(EntityManagerInterface $entity_manager, FieldTypePluginManagerInterface $field_type_plugin_manager, ConfigFactoryInterface $config_factory, ModuleHandlerInterface $module_handler) {
   ...
   +    $this->moduleHandler = $module_handler;
   
   @@ -84,7 +95,8 @@ public static function create(ContainerInterface $container) {
   +      $container->get('module_handler')
   

   Since the alteration is done by the field type manager, we don't need to inject the module handler anymore here.

7. +++ b/core/modules/field_ui/src/Form/FieldStorageAddForm.php
   @@ -418,12 +435,20 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
   +        'settings' => $widget_settings,
   
   @@ -436,12 +461,20 @@ protected function configureEntityFormDisplay($field_name, $widget_id = NULL) {
   +        'settings' => $formatter_settings,
   

   Adding $widget_settings to this array should be handled in its own if condition, based whether a non-empty array was received by the method.

8. +++ b/core/modules/media/media.module
   @@ -96,3 +96,23 @@ function template_preprocess_media(array &$variables) {
   +  if ($class !== $item_class && !is_subclass_of($class, $item_class)) {
   

   Same as above, let's use is_a().

9. +++ b/core/modules/media/tests/src/Functional/MediaEntityReferenceFieldTest.php
   @@ -0,0 +1,29 @@
   +  public function testRenderedEntityReferencedMedia() {
   

   Can't we put this new test inside the existing \Drupal\Tests\media\Functional\MediaUiFunctionalTest test class?

Looks quite good! Just a few very small things.

1. +++ b/core/lib/Drupal/Core/Field/FieldTypePluginManagerInterface.php
   @@ -85,6 +85,26 @@ public function getDefaultStorageSettings($type);
   +   * allowing modules to alter the result of this method by implementing the
   +   * alter hook "hook_field_ui_preconfigured_options_alter()".
   

   I don't think we need to say "...the alter hook". We can just say "implementing hook_field_ui_preconfigured_options_alter()".

2. +++ b/core/modules/field/field.api.php
   @@ -59,6 +59,34 @@ function hook_field_info_alter(&$info) {
   + * Perform alterations on field preconfigured options.
   

   Should say "preconfigured field options".

3. +++ b/core/modules/field/field.api.php
   @@ -59,6 +59,34 @@ function hook_field_info_alter(&$info) {
   + *   The id of the field type plugin implementing
   

   Can this just say "The field type plugin ID"?

4. +++ b/core/modules/field_ui/src/Form/FieldStorageAddForm.php
   @@ -418,12 +422,20 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
   +      if (!empty($widget_settings)) {
   +        $options['settings'] = $widget_settings;
   +      }
   

   Why do we need the if (!empty()) check?

5. +++ b/core/modules/field_ui/src/Form/FieldStorageAddForm.php
   @@ -436,12 +448,20 @@ protected function configureEntityFormDisplay($field_name, $widget_id = NULL) {
   +      if (!empty($formatter_settings)) {
   +        $options['settings'] = $formatter_settings;
   +      }
   

   Same here -- why do we need to check if $formatter_settings is not empty?

Okay, cool. I re-read the patch and can find nothing to complain about :) Let's go back to RTBC.

Looks good to me as well now. Great work, Marcos!

I'm changing some metadata for this issue because the actual Media part is just the hook implementation. This makes me wonder if we also need a change record..

We're adding a hook, so I suspect we do.

1. +++ b/core/lib/Drupal/Core/Field/FieldTypePluginManager.php
   @@ -135,7 +135,8 @@ public function getUiDefinitions() {
   +        $options = $this->getPreconfiguredOptions($definition['id']);
   +        foreach ($options as $key => $option) {
   

   do we need the $options local variable here, can we just inline it?

2. +++ b/core/modules/media/media.module
   @@ -96,3 +96,22 @@ function template_preprocess_media(array &$variables) {
   +    $options['media']['entity_view_display']['type'] = 'entity_reference_entity_view';
   

   Do we want to add settings here too?

Testbot is happy, so back to RTBC.

+1 to RTBC. As the issue introducing new alter hook, we might need a change record?

my bad, CR looks good.

This is blocking #2862458: [META] Once media is enabled, having the File, Image and Media reference fields all listed is confusing as well.

Quick note: +1 to the hook over the large annotation.

Adding review credits for @yoroy (manual testing with screencast), @phenaproxima and @amateescu (reviews) and @seanB (clearly involved in the design and scope as evidenced by IRC conversation)

Committed as cf805af and pushed to 8.5.x.

Unpostponed the postponed issue.

Published the change record.