Fix the UX of Entity Reference fields

We need an issue for breadcrumbs too.

At the very least, this will take care of the sorting.

I have a solid use case for keeping configuration entities in the UI, or at least partly. Payment's payment content entities contain a reference to payment method configuration entities. At the moment this is still an integer field as I haven't had the time to convert it to an entity reference yet, but when I convert this, configuration through the UI would allow people to easily change the widget or the formatter.

I would go for 2). Do we have any sensible groupings we can get from elsewhere? For example our top level "Structure", "Content", "People", "Configuration" ?

We only have two groups, Content and Configuration.

Agree with Xano and Bojhan, it would be very nice to have all types of items to reference in the UI. But probably it will not be that difficult to make it's visibility configurable somewhere in Configuration window? By default all that "1% or less case" stuff will be hidden, but those guys who need it - and, hence understand what it is - will be able to to make it visible.

How about having a more option which would lead to additional options with in the list?

Here's using optgroups, works fine for me.

+++ b/core/lib/Drupal/Core/Entity/EntityManager.php
@@ -617,9 +617,13 @@ public function getAllBundleInfo() {
+      if (isset($definition['config_prefix'])) {

Everywhere else we check whether the class implements ConfigEntityInterface/ContentEntityInterface.

Also, this patch does not sort the labels.

What a horrible way of checking that :/

AFAIK we require entities to either implement ContentEntityInterface or ConfigEntityInterface. It's not very pretty, but it's consistent with the rest of core. Besides that, the config prefix is a storage controller setting, so we cannot use that outside the ConfigStorageController scope.

+++ b/core/lib/Drupal/Core/Entity/EntityManager.php
@@ -614,15 +614,28 @@ public function getAllBundleInfo() {
+      t('Content') => $content_labels,

This will conflict with the translation of the node entity label, which is also 'Content'. We need to specify a context here, and preferably the same for Configuration.

Another option would be to introduce a new entity info key and group on that. Something like group_label_that_makes_sense_for_humans = @Translation("You don't need to worry about these").

We are already introducing this pattern for block categories.

Edit: An alternative name for the new key would be 'category' :)

Yes, I was thinking about introducing groups/types too. Because there will be more than just those two in Contrib, I have e.g. modules that expose remote data as entities, those won't be config nor content, so it would be great if I could put them in my own group.

Another thing I briefly discussed with @dawehner was to support multiple annotation classes, that don't do anything else other than provide sane defaults for a given type/group of entity types. e.g. @ConfigEntityType and @ContentEntityType. wouldn't really be extendable to my custom group, though. If it works at all...

@Berdir Could you expand on that a little. What other groups should we support by default? (Media, Product?)

There are no other groups to support by default. But using an extensible group system instead of a config/content flag makes it easy for contrib to group themself by whatever they want.

Another option would be to introduce a new entity info key and group on that. Something like group_label_that_makes_sense_for_humans = @Translation("You don't need to worry about these").
We are already introducing this pattern for block categories.

I think there is an issue somewhere that intends to do the same for field types - e.g structure the list of available field types with optgroups (Text, Number, List, File...). This seems like a common (if not generic) and very reasonable need : structure an otherwise flat list of available plugin implementations for a given plugin type.
Not saying this should necessarily be moved up to the plugin system, but at least we should try to unify implementations for the plugin types where we add this.

There are no other groups to support by default. But using an extensible group system instead of a config/content flag makes it easy for contrib to group themself by whatever they want.

Agreed. In Rules 7.x I've been doing a 'group' key with arbitrary label, seems to me as good fit here as well.

I've another use-case, when integrating with remote systems and exposing their data as entities and makes a lot of sense to group them. E.g. we've been using remote entities to integrate with twitter (see fluxtwitter), grouping the different twitter entities (tweets, users, lists,..) below a "Twitter" or similar category would be helpful here.

Added #2147685: Fields settings page breadcrumb is not correct for issue reported in #1.

Since I'm "they"..

removing config entities from the list was one of the commit blockers for the initial ER patch due to this UX issue, and then somehow they sneaked back in again without this ever being addressed. :(

As far as I know config entities were never part of the list, because they were not technically supported. @amateescu and I added support for them a few months ago.

Here's a new approach which adds a generic a GroupablePlugin trait and uses it for entity types.

This should fix it.

This looks really good.

So, should we also introduce 'Other' - or something a like a put Menulink under that one ? It seems so lonely there at the top :)

I agree with swentel :)
That also would move menu link to the bottom. Makes more sense anyway...

Hmmm the list is not alphabetically...
I would love to see this order:

- Content
- Config
- Other

Add an "Other" category, and sorted alphabetically by group label. The popup looks a little weird due to their being a lot of options above the default...I think its an improvement though.

I wouldn't worry too much about menu link specifically, converting it to a content entity is a beta blocker. Contrib could have entities that are other, so having a general group is useful.

And IMHO, Content is way more important in that last. Not sure what to do there. It would IMHO be more important to sort the actual labels alphabetically, not the groups. You might think they are at first, but if you look closer, you can see that the only sorting there is the module/provider those entity types are in. Action for example is one the last config entities, because it's in system.module.

Content > Content is also pretty stupid, but I really have no idea to solve that, there are multiple issues already about the naming problems with Node/Entity/Content. Using Content made sense in Drupal 7 core, but it's becoming weirder and weirder, with node.module being optional and a lot of things now being Content (entities).

The only idea that I have there, and that would also eliminate the need for an Other group is to special case the Content group and add them (and possibly entities that are not Content like MenuLink right now too) without a group, before any of the groups.

Yes, this would require more code within ER, but it's something that might be worth it for better DX?

This issue might also provide a solution for #2194783: Rename EntityTypeInterface::isSubclassOf() to ::entityClassImplements() then, so that we could check for a specific group instead of an interface. I'm not sure if that is a better API than the current two suggestions there (isContent()/isConfig() or check the entity type class/interface) though. But it would be extensible, which both of those two aren't (contrib can't add a isMyGroup() or a MyGroupEntityType).

Edit: Also, re order, quoting from the issue summary: "so the "90% case" stuff is grouped together and at the top". So that agrees with my opinion above. I would argue that not grouping the 90% case at all and keeping them at the top still kind of qualifies as "grouped together" :)

Updated to sort labels as well as group labels. I am not so sure on using ksort - I suspect this will not sort internationalized strings (UTF-8) correctly.

I could look at special casing the Content group but it maybe feels a little hacky.

We probably need a test for group labels in addition to EntityManagerTest::testGetEntityTypeLabels(). While fixing the test fail, I noticed that $this->entityManager->getEntityTypeLabels() returns nothing on a second or subsequent calls - is this a mocking thing?

Not so sure on this...but it's something.

Woops

That looks afwul, why can't we just special case that group to be on top?

OK.

This will only float Content types to the top if grouping is specified. It does look much nicer :)

keeps the problem of "Content > Content", though.

1. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   @@ -467,13 +467,34 @@ public function getAllBundleInfo() {
   +    // Sort labels in alphabetical order.
   +    ksort($definitions);
   

   This doesn't sort the labels, it sorts the keys?

   Should probably sort on the label, using a callback function? So that the order also makes sense if labels are translated.

2. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   @@ -467,13 +467,34 @@ public function getAllBundleInfo() {
   +        $group_label = $definition->getGroupLabel() ?: \Drupal::translation()->translate('Other', array(), array('context' => 'Entity type group'));
   

   Should we set Other somewhere else, for example in the EntityType parent annotation class, or the EntityManger, when processing found plugins? Otherwise, Other would just exist for this list...

3. +++ b/core/lib/Drupal/Core/Entity/EntityType.php
   @@ -178,6 +178,16 @@ class EntityType implements EntityTypeInterface {
   +   * The machine name of the entity type group.
   +   */
   +  protected $group;
   +
   +  /**
   +   * The human-readable name of the entity type group.
   +   */
   +  protected $group_label;
   

   What exactly is the point of the trait, it doesn't seem to be used as we define it manually here?

@yched: Discussed with @Bojhan, he is apparently not worried about that.

1. Fixed.
2. Fixed as well.
3. That's because EntityType is a special flower and uses an object to represent the definition, all other plugins that use arrays for their definition should be fine with the trait only.

+++ b/core/lib/Drupal/Core/Entity/EntityManager.php
@@ -469,32 +469,29 @@ public function getAllBundleInfo() {
+      $content = \Drupal::translation()->translate('Content', array(), array('context' => 'Entity type group'));

+++ b/core/lib/Drupal/Core/Entity/EntityType.php
@@ -609,7 +609,7 @@ public function getGroup() {
+    return !empty($this->group_label) ? $this->group_label : \Drupal::translation()->translate('Other', array(), array('context' => 'Entity type group'));

I thought we couldn't use the translate method like this, and needed to have t()/$this->t()

Like this?

51: 2116551-entity_reference_ux-51.patch queued for re-testing.

Still green . Could someone update issue summary? I think we can go to RTBC after that.

Rerolled and updated the issue summary

I reviewed the code and and issue summary and both look good. Bot is happy so lets do this.

Good stuff, so yes, lets do this

1. +++ b/core/lib/Drupal/Core/Annotation/GroupablePlugin.php
   @@ -0,0 +1,29 @@
   +trait GroupablePlugin {
   

   First, I think we decided to suffix these with "Trait".

   That said, we have attempted to decouple "Plugins" from "Annotations", at least where possible. There are annotation classes with Plugin in the name, and EntityType currently extends one, but that might not be true for long.

   So I'm not too keen on the naming here. And I don't necessarily think a trait is even a big help here. I appreciate the push for something generic, but I think this could be better served as a part of EntityType directly.

2. +++ b/core/lib/Drupal/Core/Annotation/GroupablePlugin.php
   @@ -0,0 +1,29 @@
   +  public $group = 'default';
   
   +++ b/core/lib/Drupal/Core/Entity/Annotation/ContentEntityType.php
   @@ -19,4 +19,18 @@ class ContentEntityType extends EntityType {
   +  public $group = 'content';
   

   Isn't this supposed to be an error?
   http://3v4l.org/t3aS0

Ok, removed the trait. No interdiff because the patch didn't apply.

1. +++ b/core/lib/Drupal/Core/Entity/Annotation/ConfigEntityType.php
   @@ -19,4 +19,18 @@ class ConfigEntityType extends EntityType {
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public $group = 'configuration';
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function get() {
   +    $this->definition['group_label'] = $this->t('Configuration', array(), array('context' => 'Entity type group'));
   +
   +    return parent::get();
   +  }
   

   What's the reason for doing this here and not on the actual EntityType subclass?

2. +++ b/core/lib/Drupal/Core/Entity/EntityType.php
   @@ -602,4 +612,28 @@ public function setUriCallback($callback) {
   +  public function getGroupLabel() {
   +    return !empty($this->group_label) ? $this->group_label : $this->t('Other', array(), array('context' => 'Entity type group'));
   +  }
   

   This could just be $this->t('Other') and let the subclasses fight it out. Same goes for group I guess.

1. Isn't this how we provide defaults?
2. That would force a new WhateverEntityType to override this method for no reason, when it could use this code from the parent. Why would we do that?

Any other concerns? :)

I can only find one nitpicky thing to complain about. Manually tested and works correctly.

1. +++ b/core/lib/Drupal/Core/Entity/Annotation/EntityType.php
   @@ -46,4 +60,13 @@ public function get() {
   +  /**
   +   * Translates a string to the current language or to a given language.
   +   *
   +   * See the t() documentation for details.
   +   */
   +  protected function t($string, array $args = array(), array $options = array()) {
   +    return \Drupal::translation()->translate($string, $args, $options);
   +  }
   

   Shouldn't this use the recently added StringTranslationTrait instead?

   See #2079797: Provide a trait for $this->t() and $this->formatPlural().

2. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   @@ -793,4 +811,13 @@ protected function getDisplayModeOptions($display_type, $entity_type_id, $includ
   +  protected function t($string, array $args = array(), array $options = array()) {
   

   Same here.

3. +++ b/core/lib/Drupal/Core/Entity/EntityType.php
   @@ -602,4 +612,28 @@ public function setUriCallback($callback) {
   +  protected function t($string, array $args = array(), array $options = array()) {
   

   And here.

Here we go :) Thanks for the review!

Missed one.

Nice!. What are the best follow-ups here? Shall we change the taxonomy reference to an entity reference. What about the User field on nodes? Or the roles field on user entity.

The UID field on nodes uses entity reference already. Term references are preconfigured entity references. For roles there is #2044859: Convert user roles to entity_reference_field.

+1

This added getGroup() and getGroupLabel() to EntityType with no docs and no interface backing them. Opened a follow-up.
#2549017: Add getGroup() and getGroupLabel() to an interface and add docs