Use the bundle label (e.g. "Media type") instead of "Bundles" in the Entity Reference field configuration

+1, I still have no idea what the "Sort by" is for. The label is an easy fix.

This is a problem in \Drupal\Core\Entity\Plugin\EntityReferenceSelection\DefaultSelection, not in Media. We can solve this generically.

Here's a first iteration.

Forgot to do this.

How about moving all secondary choices to a collapsed fieldset by default?

+++ b/core/lib/Drupal/Core/Entity/Plugin/EntityReferenceSelection/DefaultSelection.php
@@ -130,7 +130,7 @@ public function buildConfigurationForm(array $form, FormStateInterface $form_sta
         '#type' => 'checkboxes',
-        '#title' => $this->t('Bundles'),
+        '#title' => $entity_type->getBundleLabel(),

Fully agree with this, quite likely we have existing issues about it, actually.

What happens if an entity type doesn't do this? In ctools, I ended up adding a fallback like \Drupal\ctools\Plugin\Deriver\EntityBundle::getEntityBundleLabel(). The core replacement for that also contains that method, maybe we could inline it into getBundleLabel()?

Removing for "for Media fields", because this is generic, even if we tried we couldn't really improve it only for Media :)

I am going to mark this major, since Media is a big new functionality piece and most users who use it will be confronted with this difficult labeling.

My preference goes to the labeling in #4

#10.1 - Tried, but when we move out of handler_settings,
a. Can't move close to title, but above fieldset(ref attachment)
b. Saving bundle value to field config is not working.
#10.2 - Added description, but happy to update, if not clear.
#10.3 - Managed to remove bundle specific titles to be more consistent.

It looks like the patch in #12 contains some other patch that's not meant for this issue.

Looking at the @xjm's list from #10, it seems that we only need fix #10.3. There an existing one for #10.1 and we need to open a separate one for #10.2.

Also, #8 was not addressed.

Agree with the separation of concerns proposed in #10.

So:
1)
#10.1 will be solved in #1953568: Make the bundle settings for entity reference fields more visible.

2)
I have opened #2927819: Clarify what "Sort by" on EntityReference field configuration form means for #10.2

3)
The patch attached intends to solve #10.3.
Re: #8, sorry for my noobiness, but I'm not fully confident I understand what the proposal is there. I have tried to address the fallback in the patch the same way ctools does, but I have the feeling that this is not what was being proposed. Can't think of anything else, please let me know if the suggestion was for something different and I just didn't get it :)

Sorry for not providing an interdiff, #12 has a lot of unrelated changes and it wouldn't make sense. This patch iterates on #4, with the suggestions from #8 and #10.3.

Ah, *this* is the issue where I suggested to add that logic directly in getBundleLabel().

This is also needed by #1932810: Add entity bundles condition plugin for entities with bundles, which needs that same chunk of code *twice*. Which is why I suggested to implement the fallback directly inside of that method. Of course then that's again out of scope and we'd end up doing yet another separate issue and postponing this. Maybe we can get feedback from a core maintainer on whether we're allowed to smuggle that addition in here?

Looks like we're not really testing those custom labels anywhere, because the new ones are different (plural, Available prefix), which I think is OK, but we should add a few assertText()'s in some hopefully already existing tests.

Thanks for the feedback!

Something like this you mean?

1. +++ b/core/lib/Drupal/Core/Entity/EntityType.php
   @@ -677,7 +677,15 @@ public function getBundleOf() {
   +    // If there is no bundle label defined, try to provide some sensible
   +    // fallbacks.
   +    if (!empty($this->bundle_label)) {
   +      return (string) $this->bundle_label;
   +    }
   +    elseif ($bundle_entity_type_id = $this->getBundleEntityType()) {
   +      return (string) \Drupal::entityTypeManager()->getDefinition($bundle_entity_type_id)->getLabel();
   +    }
   +    return (string) $this->getLabel();
   

   Something like that, yes, but the last should be "$this_entity_type_label bundle", as a translatable string. E.g. if node would not define a bundle label or bundle entity type, we'd fall back to "Node bundle", your code would just return "Node".

2. +++ b/core/modules/field/src/Tests/EntityReference/EntityReferenceAdminTest.php
   @@ -94,6 +94,9 @@ public function testFieldAdminHandler() {
        // The base handler settings should be displayed.
        $entity_type_id = 'node';
   +    // Check that the type label is correctly displayed.
   +    $entity_type = \Drupal::entityTypeManager()->getDefinition($entity_type_id);
   +    $this->assertText($entity_type->getBundleLabel());
        $bundles = $this->container->get('entity_type.bundle.info')->getBundleInfo($entity_type_id);
   

   we hardcode the entity type, so maybe we should also hardcode the expected label, makes the test easier to read IMHO?

Oh I see. Let's try this one then.

Thanks!

1. This is an annotation class (=the plugin definition, not the plugin), we can't inject things there. This is also not the first case where we have this, see \Drupal\Core\Entity\EntityType::getBundleConfigDependency() for example. The only thing we could possible do is add a $this->getEntityTypeManager() wrapper, but it will still need to call out to \Drupal.

We looked at this issue during the weekly UX meeting. I missed most of the discussion, but I was able to make some screenshots.

Here are the screens for adding/editing a taxonomy term or a Media reference without any patch:

Here are the screens for adding/editing a taxonomy term or a Media reference with the patch from #20:

Thanks @xjm, @Berdir and @benjifisher!

Here an updated patch with some tests.
I don't know how to test our intermediate fallback, suggestions welcome :)
But I was wondering that once Drupal\Core\Entity\EntityType::getLabel() is already covered by Drupal\Tests\Core\Entity\EntityTypeTest::testGetLabel(), and the bundle will also be an entity type, maybe that's implicitly covered?

@benjifisher uploaded before/after screenshots for Taxonomy Terms and Media in #23, so I'm just uploading here the remaining ones (Nodes and Users):

Nodes HEAD:

Nodes with the patch:

Users HEAD and with the patch (no change):

1. +++ b/core/lib/Drupal/Core/Entity/EntityType.php
   @@ -677,7 +677,15 @@ public function getBundleOf() {
   -    return (string) $this->bundle_label;
   ...
   +    return t('@type_label bundle', ['@type_label' => $this->getLabel()]);
   

   The return value of this method is documented to be a string (in \Drupal\Core\Entity\EntityTypeInterface::getBundleLabel) but now we might also return a TranslatableMarkup object, so the fallback is missing a (string) cast.

   Also, we should be using new TranslatableMarkup() directly instead of t() in non-procedural code.

2. +++ b/core/modules/field/src/Tests/EntityReference/EntityReferenceAdminTest.php
   @@ -94,6 +94,8 @@ public function testFieldAdminHandler() {
   +    // Check that the type label is correctly displayed.
   +    $this->assertText('Content type');
   
   +++ b/core/modules/node/src/Plugin/EntityReferenceSelection/NodeSelection.php
   @@ -22,15 +22,6 @@ class NodeSelection extends DefaultSelection {
   -    $form['target_bundles']['#title'] = $this->t('Content types');
   

   The previous code was displaying the plural form of the bundle label, and the new code is displaying the singular form. Is that a concern that we might need to address?

@amateescu thanks for the feedback!

This addresses #26.1.

Concerning

The previous code was displaying the plural form of the bundle label, and the new code is displaying the singular form. Is that a concern that we might need to address?

I was assuming that this was an intended (or at least expected) change, in favor of standardization and removing "special cases". IMHO the singular form is as understandable as the plural (and checkboxes already indicate you can select more than one), but that's just my opinion, I'm no UX expert :)

Thanks!

Ok, we have passing tests now so the patch looks ready to me.

The question from #26.2 is still outstanding, but maybe a committer can decide..

+++ b/core/modules/taxonomy/src/Plugin/EntityReferenceSelection/TermSelection.php
@@ -38,8 +38,6 @@ public function defaultConfiguration() {
 
-    $form['target_bundles']['#title'] = $this->t('Available Vocabularies');
-

Slightly different variations of plural forms even, yes :)

That change is fine with me, we all know that generic plural forms of labels are crazy complicated :)

  /**
   * Gets the label for the bundle.
   *
   * @return string|null
   *   The bundle label, or NULL if none exists.
   */
  public function getBundleLabel();

It can't be NULL anymore so this doc block needs update.

Ouch. Too late for this?

Please create a followup.

Follow-up: #2929076: Fix wrong \Drupal\Core\Entity\EntityTypeInterface::getBundleLabel() docblock