InvalidArgumentException when adding reference field without Media type

Patch is attached.

Needs to be tested against 8.7.x

+++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
@@ -282,6 +282,14 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
+      $element['empty_types'] = [
+        '#markup' => $this->t('There are no allowed media types available for this field.'),
+      ];

We can remove the word "allowed" from this sentence, I think.

Can we also get before/after screenshots of this, steps to reproduce, and tests?

Fixed #6 and added tests. IS updated with steps to reproduce and screenshots.

Thanks @Pancho! Nice write-up. Patch also looks good to me.

I don't have a strong opinion on whether we should hide the widget or show a message. I'll leave that up to the UX team.

Other entity reference widgets in core get rendered in this case, even though they are not useful at all. For example the 'Select' widget just shows the option 'None'. That might be a reason to at least show the widget, but since this is kind of an edge case anyway, I think the most important thing is to at least not break with a fatal error. I could live with it either way :)

Ideally we will land this for 8.7.x first, and then try to get it backported. :)

I believe this bug was introduced in the 8.7 branch, so fixing this in 8.6 is probably not necessary.

This has been discussed for the media library widget in the UX meeting today: https://www.youtube.com/watch?v=MG0SgDMWsH0

I think the general consensus was we should try to show a helpful message to users instead of hiding the widget. So for editor users without permissions to edit the field or create new bundles of the referenced entity type it should be something like:
There are no media types available. Please contact a site administrator.

For users with permissions to create new bundles of the entity type, it should be something like:
There are no media types available. Please <a href=":url">add a media type</a> and enable it for this field.

The UX feedback was pretty unanimous; we can change labels all the way through beta if we want to bikeshed the wording of the messages. I'm removing the "needs usability review" tag.

Started from patch in #8 since that was closest to what it should be. Added extra tests for the message variations.

+++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
@@ -287,6 +300,14 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
+    $allowed_media_type_ids = $this->getAllowedMediaTypeIdsSorted();
+    if (!$allowed_media_type_ids) {
+      $element['empty_types'] = [
+        '#markup' => $this->accessManager->checkNamedRoute('entity.media_type.add_form') ? $this->t('There are no media types available. Please <a href=":url">add a media type</a> and enable it for this field.', [':url' => Url::fromRoute('entity.media_type.add_form')->toString()]) : $this->t('There are no media types available. Please contact the site administrator.'),
+      ];
+      return $element;
+    }

Three things:

1) This is a very long ternary. I think it might be easier to read if it were done as an if/else structure.

2) I feel like we might be better off if we just check user permissions, rather than access to a specific route.

3) I'm not sure if we should switch on whether the user can add media types. My feeling is that we should instead look at whether the user has permission to configure the field, and link to its settings form if they do. Then, if they get there and see that there are no media types that can be referenced, they can navigate away on their own and add more. In other words, we shouldn't assume that no media types exist -- we should instead assume that the field is misconfigured.

#27
1. Fixed.
2. The permission does not guarantee access to create media types. Users might see the link and still not have access if we do that.
3. The problem we trying to solve here is that the installation does not contain media types. In all other cases the widget loads either the selected media types, or all existing ones (if the list of configured types is null or empty). The place where this could manifest itself already is the field configuration page. Creating a media type solves the issue, so that should be the action we link the user to. Added a comment to explain this a bit better.

Whoops, small typo. This patch should do it.

1. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -287,6 +300,26 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   +    // create a media type if they have permissions.
   

   We're not checking permissions, so let's say "...if they have access."

2. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -287,6 +300,26 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   +    $allowed_media_type_ids = $this->getAllowedMediaTypeIdsSorted();
   

   Um...the comment JUST said that this would return all media types if none are available. So doesn't this mean the message will never be displayed?

3. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -287,6 +300,26 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   +      if ($this->accessManager->checkNamedRoute('entity.media_type.add_form')) {
   +        $empty_message = $this->t('There are no media types available. Please <a href=":url">add a media type</a> and enable it for this field.', [
   +          ':url' => Url::fromRoute('entity.media_type.add_form')->toString(),
   +        ]);
   +      }
   

   There's no need to inject the access manager at all. We can just do this:

   $url = Url::fromRoute('...');
   if ($url->access()) { ... }
   

Changed the approach a bit since I found #3038245: [PP-1] Don't allow all media types if no target bundle is configured. This patch uses the media types returned in getAllowedMediaTypeIdsSorted() to determine whether or not to show a message. As it turns out, there is undesirable behavior in getAllowedMediaTypeIdsSorted(). When the list of target bundles for a field is an empty array, we currently return all media types, while we should return none.

I think we should solve both issues at the same time here, since I don't really see how we can separate these 2 issues. New patch is attached.

Forgot to remove the access manager. We don't need that anymore.

1. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -118,10 +120,15 @@ protected function getAllowedMediaTypeIdsSorted() {
   +    $allowed_media_type_ids = isset($handler_settings['target_bundles']) ? $handler_settings['target_bundles'] : NULL;
   

   I don't think this needs to be a ternary. I believe the target_bundles setting will always exist, and NULL is a valid value for it. So we can just say $allowed_media_type_ids = $handler_settings['target_bundles'].

2. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -118,10 +120,15 @@ protected function getAllowedMediaTypeIdsSorted() {
   +    if (is_array($allowed_media_type_ids) && empty($allowed_media_type_ids)) {
   

   Nit: This could just be $allowed_media_type_ids === [].

3. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -118,10 +120,15 @@ protected function getAllowedMediaTypeIdsSorted() {
   +    if ($allowed_media_type_ids === NULL) {
   

   This should probably be an elseif.

4. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -287,6 +294,31 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   +      $route_parameters = [
   +        'field_config' => $this->fieldDefinition->id(),
   +      ] + FieldUI::getRouteBundleParameter($this->entityTypeManager->getDefinition($entity_type_id), $this->fieldDefinition->getTargetBundle());
   +      $url = new Url('entity.field_config.' . $entity_type_id . '_field_edit_form', $route_parameters);
   +      if ($url->access()) {
   +        $empty_message = $this->t('There are no allowed media types configured for this field. Edit the <a href=":url">field settings</a> to select the allowed media types.', [
   +          ':url' => $url->toString(),
   +        ]);
   +      }
   

   Based on my quick research, we have no actual dependency on Field UI. So we'll need to check if field_ui is enabled before generating this URL. Also, can we move all of this message generation logic to a protected helper method?

5. +++ b/core/modules/media_library/tests/modules/media_library_test/config/install/field.storage.node.field_null_types_media.yml
   @@ -0,0 +1,19 @@
   +langcode: en
   +status: true
   +dependencies:
   +  module:
   +    - media
   +    - node
   +id: node.field_null_types_media
   +field_name: field_null_types_media
   +entity_type: node
   +type: entity_reference
   +settings:
   +  target_type: media
   +module: core
   +locked: false
   +cardinality: -1
   +translatable: true
   +indexes: {  }
   +persist_with_no_fields: false
   +custom_storage: false
   

   Shouldn't we also be including a field.field.node.TYPE.field_null_types_media.yml too?

I discussed this with Sean a bit on Slack. Here's what I think we need to do if no media types are available for the field:

If the user has the "administer HOST ENTITY TYPE fields" permission, we show this message: "There are no allowed media types configured for this field. Edit the field settings to select the allowed media types." If the Field UI module is enabled, we link the words "Edit the field settings" to the appropriate place (assuming the user has access to the generated URL).

If the user does NOT have the "administer HOST ENTITY TYPE fields" permission, we just say: "There are no allowed media types configured for this field. Please contact the site administrator."

It's more important that we get the logic correct than the actual wording. Strings can be changed in beta.

#33
1. Fixed.
2. Fixed.
3. I think the intention clearer and the a little bit less complexity if we don't use an elseif.
4. Fixed.
5. The patch shows it as a copy, but it's there :)

#34 Fixed.

Getting there!!

1. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -50,6 +52,20 @@ class MediaLibraryWidget extends WidgetBase implements ContainerFactoryPluginInt
   +  /**
   +   * The current active user.
   +   *
   +   * @var \Drupal\Core\Session\AccountProxyInterface
   +   */
   +  protected $currentUser;
   

   This should be using Drupal\Core\Session\AccountInterface, not AccountProxyInterface.

2. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -72,10 +88,24 @@ class MediaLibraryWidget extends WidgetBase implements ContainerFactoryPluginInt
   +  public function __construct($plugin_id, $plugin_definition, FieldDefinitionInterface $field_definition, array $settings, array $third_party_settings, EntityTypeManagerInterface $entity_type_manager, AccountProxyInterface $current_user, ModuleHandlerInterface $module_handler) {
   

   $current_user and $module_handler should both default to NULL, and their doc comment should begin with (optional).

3. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -504,6 +523,38 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   +   * Get the message when there are no allowed media types for the field.
   

   Should be "Gets the message displayed when..."

4. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -504,6 +523,38 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   +   * @return \Drupal\Core\StringTranslation\TranslatableMarkup
   +   *   The empty message.
   

   Technically this should be MarkupInterface. And the description should probably be something like "The message to display when there are no allowed media types for this field."

5. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -504,6 +523,38 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   +  protected function getEmptyMessage() {
   

   I think we should rename this method. getEmptyMessage() sounds like nothing is referenced, but in fact this is the message we generate when there are no media types available. So how about getNoMediaTypesAvailableMessage()?

6. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -504,6 +523,38 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   +    $route_parameters = [
   +        'field_config' => $this->fieldDefinition->id(),
   +      ] + FieldUI::getRouteBundleParameter($this->entityTypeManager->getDefinition($entity_type_id), $this->fieldDefinition->getTargetBundle());
   

   Nit: This is hard to read, can it be organized a bit? Maybe something like this:

   $route_parameters = FieldUI::getRouteBundleParameter(...);
   $route_parameters['field_config'] = $this->fieldDefinition->id();
   $url = Url::fromRoute('...', $route_parameters);
   

7. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -504,6 +523,38 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   +    return $this->t('There are no allowed media types configured for this field. <a href=":url">Edit the field settings</a> to select the allowed media types.', [
   +      ':url' => $url->toString(),
   +    ]);
   

   We only want to link the text if $url->access() checks out. So let's be sure to check that as well.

8. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -249,6 +249,26 @@ public function testWidgetWithoutMediaTypes() {
   +    $field_ui_uninstalled_message = 'There are no allowed media types configured for this field. Edit the field settings to select the allowed media types.';
   +
   +    // Assert a properly configured field still shows a message.
   +    $assert_session->elementContains('css', '.field--name-field-twin-media', $field_ui_uninstalled_message);
   +    $assert_session->elementNotExists('css', '.media-library-open-button[name^="field_twin_media"]');
   +    // Assert that the message is shown when the target bundles setting for the
   +    // entity reference field is an empty array.
   +    $assert_session->elementContains('css', '.field--name-field-empty-types-media', $field_ui_uninstalled_message);
   +    $assert_session->elementNotExists('css', '.media-library-open-button[name^="field_empty_types_media"]');
   +    // Assert that the message is shown when the target bundles setting for the
   +    // entity reference field is null.
   +    $assert_session->elementContains('css', '.field--name-field-null-types-media', $field_ui_uninstalled_message);
   +    $assert_session->elementNotExists('css', '.media-library-open-button[name^="field_null_types_media"]');
   

   Once Field UI is uninstalled, we should also assert that the "Edit the field settings" link does not exist.

Fixed #36

More nits!

1. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -287,6 +326,18 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   +      $element['empty_types'] = [
   

   Nit: Can this key be 'no_types_message', not 'empty_types'?

2. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -473,6 +523,45 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   +    // Show a message for administrator users to configure the field if the
   

   s/administrator/privileged

3. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -473,6 +523,45 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   +    // field UI module is not enabled.
   

   Nit: "field UI" should be "Field UI", since that is the proper name of the module.

4. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -473,6 +523,45 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   +    // Add a link to the message to configure the field if the field UI module
   

   Same here.

5. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -473,6 +523,45 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   +    $url = new Url('entity.field_config.' . $entity_type_id . '_field_edit_form', $route_parameters);
   

   Nit: Let's use Url::fromRoute(), so it's a bit clearer what we're creating here.

6. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -473,6 +523,45 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   +    if ($url->access()) {
   

   We can pass $this->currentUser to $url->access().

7. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -473,6 +523,45 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   +    // If the user for some reason doesn't have access to the field UI, fall
   

   Field UI

8. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -141,6 +143,142 @@ public function testAdministrationPage() {
   +    // Visit a node create page.
   +    $this->drupalGet('node/add/basic_page');
   

   I don't think this comment adds much. :)

9. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -141,6 +143,142 @@ public function testAdministrationPage() {
   +    $assert_session->linkExists('Edit the field settings');
   

   I think we might want to assert that the link appears _within_ the field wrapper. To that end, we'd need to do something like this:

   $assert_session->elementExists('named', ['link', 'Edit the field settings'], $wrapper_element);

10. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
    @@ -141,6 +143,142 @@ public function testAdministrationPage() {
    +    // Assert that the message is shown when the target bundles setting for the
    

    Should be 'target_bundles'.

11. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
    @@ -141,6 +143,142 @@ public function testAdministrationPage() {
    +    // Assert the messages are also shown in the default value section of the
    +    // field edit form.
    +    $this->drupalGet($field_empty_types_url);
    +    $assert_session->elementContains('css', '.field--name-field-empty-types-media', $field_empty_types_message);
    +    $assert_session->linkExists('Edit the field settings');
    +    $assert_session->elementNotExists('css', '.media-library-open-button[name^="field_empty_types_media"]');
    +    $this->drupalGet($field_null_types_url);
    +    $assert_session->elementContains('css', '.field--name-field-null-types-media', $field_null_types_message);
    +    $assert_session->linkExists('Edit the field settings');
    +    $assert_session->elementNotExists('css', '.media-library-open-button[name^="field_null_types_media"]');
    

    This is going above and beyond. Nice. :)

12. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
    @@ -141,6 +143,142 @@ public function testAdministrationPage() {
    +    // Uninstall the field UI and check if the link is removed from the message.
    

    "Field UI"

13. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
    @@ -141,6 +143,142 @@ public function testAdministrationPage() {
    +    $assert_session->elementContains('css', '.field--name-field-null-types-media', $field_ui_uninstalled_message);
    +    $assert_session->linkNotExists('Edit the field settings');
    +    $assert_session->elementNotExists('css', '.media-library-open-button[name^="field_null_types_media"]');
    

    So it occurs to me that these assertions are pretty repetitive. We could abstract them into a helper method. Something like this:

    private function assertNoMediaTypesMessage($field_name, AccountInterface $current_user) {
      // Assert that the field element wrapper exists. Then, based on the current user permissions and the status of Field UI, assert the proper text and links are found in the wrapper.
    }
    

14. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
    @@ -245,9 +383,10 @@ public function testWidget() {
    +    // Assert that the media type menu is available when the target bundles
    

    Should be target_bundles.

#38

1. Fixed.
2. Fixed.
3. Fixed.
4. Fixed.
5. Fixed.
6. Fixed.
7. Fixed.
8. Fixed.
9. See 13.
10. Fixed.
11. Yay!
12. Fixed.
13. Changes the test a bit. We were actually asserting the link as part if the message twice. So removed the duplicate assertions. This helps make the test feel less repetitive. The helper method would need quite a bit of parameters and logic (field name, field config, user permissions, message, available media) to figure out what the state should be of each field, so I think the current state makes the tests quite a bit easier.
14. Fixed.

1. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -473,6 +523,45 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   +   * Get the message to display when there are no allowed media types.
   

   Nit: Should be "Gets", not "Get".

2. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
   @@ -473,6 +523,45 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   +    // Show a default message if the user does not have the permissions to
   

   Should be "Show the default message..."

3. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -141,6 +143,135 @@ public function testAdministrationPage() {
   +    $assert_session->elementTextNotContains('css', '.field--name-field-twin-media', $default_message);
   

   Let's just assert the absence of the text "There are no allowed media types configured for this field." If we assert the entire default message, the test would still pass if the user has permission to administer the fields.

4. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -141,6 +143,135 @@ public function testAdministrationPage() {
   +    $assert_session->elementTextContains('css', '.field--name-field-empty-types-media', $default_message);
   

   Same here. We should assert "There are no allowed media types configured for this field."

5. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -141,6 +143,135 @@ public function testAdministrationPage() {
   +    $assert_session->elementTextNotContains('css', '.field--name-field-null-types-media', $default_message);
   

   And here too. Let's make the same change for everywhere else in the test.

RTBC after this. Nice work!!

Fixed #40.

Let's just assert the absence of the text "There are no allowed media types configured for this field." If we assert the entire default message, the test would still pass if the user has permission to administer the fields.

I think we should only assert part of the message in the cases where we do not expect a message. If we do expect a message, we probably want to make sure it is the right one. So only updated the tests for elementTextNotContains.

Makes sense to me. Thanks!

Crediting folks from the UX call back in #21.

The bug fix looks great; I'm really happy that we give the user the most actionable feedback possible, given that fixing this requires finding a very, VERY buried settings form somewhere.

Committed and pushed to 8.8.x, back-ported to 8.7.x.