Show field/field-storage summary instead of field type on field listings

Quoting myself from #1847596-268: Remove Taxonomy term reference field in favor of Entity reference

One approach could be to change the "field type" column in the "Manage Fields" overview to contain a summary , like we have on “Manage Display“. Would need a new method in FieldItemInterface though

Consistency++ ?

Forgot to say it in the other issue but I think #2 is a great idea!

Also, this is more a Field UI thing, with ER being the early adopter.. as usual :)

Just an additional note : turning the column into a summary would also mean it's not a link (to the "storage settings" form) anymore - which is fine IMO, since it currently duplicates the "Storage settings" link present in the "operations" dropdown in the last column.

Rerolled first.

First of all this means changes for APIs so need to go on 8.1 or later. Next will post cleanup patch.

Fixed some minor docs stuff. Looked into reinstantiating the field type + settings links. However the new summary column is based on field settings not field storage settings while the old link/name is based on storage settings. So is this new behavior is what we want/need?

Rerolled, and changed array() by [].

Can we bring this back to life?

"Media fields" would definitely benefit from this information exposed there.

The attached patch:
- Fixes $field_storage typehinting in FieldItemInterface::storageSettingsSummary()
- Implements a default summary on the base class with something like "Field type: @type"
- Implemnets an extention to that for EntityReferenceFieldItem fields that shows the target entity type as well.

Before:

After:

Tests were not updated, the fail is expected.
If the approach looks good I can update the tests.

We discussed this at the weekly UX meeting today. I think that @yoroy was taking notes, but here are the suggestions that I remember.

Instead of using a column header like "Details" or "Summary", neither of which contains a lot of information, keep the current header "Field type". Then you can remove the repeated "Field type: " on each row.

You can distinguish between the default information (field type) and the additional information by making the default information bold. For example,

Entity reference
Referenced entity type: Media

If that formatting draws too much attention to this column, then perhaps make the field type normal and make the additional information italic.

I think everyone agreed that using unlinked text for this column is a good idea.

Thanks @benjifisher for the feedback!

This is the result after the changes mentioned above:

In this patch I have also modified / added some tests.

1. +++ b/core/lib/Drupal/Core/Field/FieldItemBase.php
   @@ -7,6 +7,7 @@
    use Drupal\Core\TypedData\DataDefinitionInterface;
    use Drupal\Core\TypedData\Plugin\DataType\Map;
    use Drupal\Core\TypedData\TypedDataInterface;
   +use Drupal\field\FieldStorageConfigInterface;
   ...
   +  public static function storageSettingsSummary(FieldStorageConfigInterface $field_storage) {
   +    return [];
   +  }
   

   This looks like a problem: I do not think that core/lib/Drupal/Core classes are supposed to depend on modules. (Maybe it is allowed to depend on System and User, since they are rquired, but Field is not.)

   But then I see

   +++ b/core/lib/Drupal/Core/Field/FieldTypePluginManager.php
   @@ -124,6 +124,30 @@ public function getDefaultFieldSettings($type) {
      /**
       * {@inheritdoc}
       */
   +  public function getStorageSettingsSummary(FieldStorageDefinitionInterface $field_definition) {
   +    $plugin_definition = $this->getDefinition($field_definition->getType(), FALSE);
   +    if (!empty($plugin_definition['class'])) {
   +      $plugin_class = DefaultFactory::getPluginClass($field_definition->getType(), $plugin_definition);
   +      return $plugin_class::storageSettingsSummary($field_definition);
   +    }
   +    return [];
   +  }

   So maybe the type hint should be FieldStorageDefinitionInterface instead of FieldStorageConfigInterface?

2. This is not required, just a question of style. In the previous snippet, I would switch the two parts of the if block. This would remove a double negative (!empty()), exit earlier, and deal with the simpler case first, all of which I like.

3. +++ b/core/modules/field_ui/src/FieldStorageConfigListBuilder.php
   @@ -124,6 +125,19 @@ public function buildRow(EntityInterface $field_storage) {
          '#items' => $usage,
          '#context' => ['list_style' => 'comma-list'],
        ];
   +    $summary = $this->fieldTypeManager->getStorageSettingsSummary($field_storage);

   I think that the type hint has to be updated.

4. +++ b/core/modules/field_ui/tests/src/Functional/ManageFieldsTest.php
   @@ -5,7 +5,7 @@
    use Drupal\Tests\BrowserTestBase;
    
    /**
   - * Tests the Manage Display page of a fieldable entity type.
   + * Tests the Manage Fields page of a fieldable entity type.

   and

    -  public function testFieldDropButtonOperations() {
   +  /**
   +   * Tests the Manage Fields page of a fieldable entity type
   +   */
   +  public function testFieldUiManageFields() {

   These look like unrelated fixes. If you think they belong here, then please say why, for the record.

5.  @@ -50,12 +57,52 @@ public function testFieldDropButtonOperations() {
          ->getStorage('field_config')
          ->create([
            'field_storage' => $storage,
   -        'bundle' => $node_type->id(),
   +        'bundle' => $bundle,
          ])
          ->save();
    
   -    $this->drupalGet('/admin/structure/types/manage/' . $node_type->id() . '/fields');
   +    $this->drupalGet('/admin/structure/types/manage/' . $bundle . '/fields');

   Now that you have introduced the extra variable, this can be simplified to "/admin/structure/types/manage/$bundle/fields".

6. +++ b/core/modules/system/tests/src/Functional/Update/UpdatePathRC1TestBaseFilledTest.php
   @@ -228,24 +230,24 @@ public function testUpdatedSite() {
        $this->drupalGet('admin/structure/types/manage/test_content_type/fields');
    
        // Make sure fields are the right type.
   -    $this->assertLink('Text (formatted, long, with summary)');
   -    $this->assertLink('Boolean');
   -    $this->assertLink('Comments');
   ...
   +    $assert_session->pageTextContains('Text (formatted, long, with summary)');
   +    $assert_session->pageTextContains('Boolean');
   +    $assert_session->pageTextContains('Comments');
   

   Can we make these tests more specific? We used to be looking for a link with the exact text "Boolean", and now we are just looking for that string anywhere on the page. The same thing happens in UpdatePathTestBaseFilledTest.

I will have another look once these points are addressed.

Rerolled to 8.6.x and addressed most points of #28

About #28.1:
Is this a hard requirement? :) I can see at least one example of a class requiring the node module (\Drupal\Core\Menu\DefaultMenuLinkTreeManipulators), and an argument could be made that it's more likely to have a site that doesn't depend on node than a site that doesn't depend on field... :)

Is #28.1 a hard requirement? I am not sure.

As I said there, it does not look as though FieldStorageConfigInterface is even the correct type hint for storageSettingsSummary(). If the right interface is \Drupal\Core\Field\FieldStorageDefinitionInterface, then the problem goes away.

Looking quickly at the interdiff, it looks as if you are replacing FieldStorageDefinitionInterface with FieldStorageConfigInterface in many places. I was hoping to do the opposite. Does that not work?

On the bright side (after the quick look) the assertions in the tests look like a big improvement.

@marcoscano: I think that replacing FieldStorageConfigInterface with FieldStorageDefinitionInterface everywhere (the opposite of what you did in the latest patch) is the right thing to do.

@shubhangi1995, have you made any progress on this? I am un-assigning this for now, but you can take it back if you are still working on it. If you do that, then please leave a comment with a progress report.

I'm at the Code Sprint, DrupalCon Nashville and will take a look at this now...

Followed @benjifisher's suggestion in #33 and created a new patch.

Verified by adding a entity reference field and the correct output appeared in /admin/structure/types/manage/article/fields:
"Entity reference
Referenced entity type: Taxonomy term"

@leanderl: I am glad to see some progress on this issue. I think it will be a big improvement in site building!

The test failed, but I do not know whether that means my suggestion in #28.1 was misguided or if you just missed one of the places where the class has to be changed. Looking at the results on the testbot should help us understand what the problem is.

On /admin/reports/fields I get the following error.
TypeError: Argument 1 passed to Drupal\Core\Field\FieldTypePluginManager::getStorageSettingsSummary() must be an instance of Drupal\field\FieldStorageDefinitionInterface, instance of Drupal\field\Entity\FieldStorageConfig given, called in /var/www/d8core/core/modules/field_ui/src/FieldStorageConfigListBuilder.php on line 128 in Drupal\Core\Field\FieldTypePluginManager->getStorageSettingsSummary() (line 128 of core/lib/Drupal/Core/Field/FieldTypePluginManager.php).

Yes, that is the same error that the testbot shows.

@leanderl:

It helps to review your changes if you attach an interdiff. There are instructions for creating one here: https://www.drupal.org/documentation/git/interdiff. I have done that for you. If you update the patch, it might be more helpful to provide an interdiff comparing to the patch in #130, ignoring the intermediate patch #138.

I am still not sure that my suggestion will work, but this looks like a problem:

+++ b/core/lib/Drupal/Core/Field/FieldTypePluginManager.php
@@ -9,6 +9,7 @@
 use Drupal\Core\Plugin\CategorizingPluginManagerTrait;
 use Drupal\Core\Plugin\DefaultPluginManager;
 use Drupal\Core\TypedData\TypedDataManagerInterface;
+use Drupal\field\FieldStorageDefinitionInterface;

We want use Drupal\Core\Field\FieldStorageDefinitionInterface; instead.

I see the same problem in the interface, too.

See my review in #26: the reason I requested this change is that Core (and Component) classes are not supposed to depend on modules (such as the Field module). So use Drupal\field\... is something we want to avoid.

As long as you are fixing these, please make sure that the use statements are in alphabetical order. It makes it more maintainable.

Also review the codesniffer_fixes.patch provided by the testbot.

From #28 above:

This looks like a problem: I do not think that core/lib/Drupal/Core classes are supposed to depend on modules.

From #30:

About #28.1: Is this a hard requirement?

Yes, it is a requirement: according to core/lib/Drupal/Core/README.txt,

Code in the Drupal\Core namespace represents Drupal Subsystems provided by the
base system.  These subsystems MAY depend on Drupal Components and other
Subsystems, but MAY NOT depend on any code in a module.

• Swapped FieldStorageConfigInterface for FieldStorageDefinitionInterface, to remove dependency on module code and keep it in core lib
• Addresses 28.3 - not 100% sure if this was the right thing
• Addresses 28.6

@cwells:

Thanks for working on this! I would love to see this issue get committed. I have not tested yet, but looking at the interdiff I see one minor issue:

--- a/core/lib/Drupal/Core/Field/FieldTypePluginManagerInterface.php
+++ b/core/lib/Drupal/Core/Field/FieldTypePluginManagerInterface.php
@@ -80,14 +79,14 @@ public function getDefaultStorageSettings($type);
   /**
    * Returns the summary of storage-level settings for a field type.
    *
-   * @param \Drupal\field\FieldStorageConfigInterface $field_definition
+   * @param FieldStorageDefinitionInterface $field_definition

According to Drupal coding standards (https://www.drupal.org/docs/develop/coding-standards/api-documentation-a...),

Always prefix types with the fully-qualified namespace for classes and interfaces (beginning with a backslash). If the class/interface is in the global namespace, prefix by a backslash.

Also see the documentation for @param elements on the same page.

P.S. Once the testbot is done, please see whether CodeSniffer finds any problems.

This should pass now, even though the hinting is a little dodgy.

OK with that point proven, this checks that it also implements the necessary Interface to build the settings summary. Also refactored into a separate private so buildRow() didn't get too out of control (per comments from Benji in person at D4D).

Feel bad for the testbot today.

Fixed the point from #46 re: fully-qualified namespace.

OK srsly now this one actually does ALL the right changes, not just some. :)

@cwells:

Thanks, this looks really promising! I have not yet tested manually nor checked out the test failures.

Looking at the interdiff, I notice just a couple of nit picks:

1. --- a/core/lib/Drupal/Core/Field/FieldItemInterface.php
   +++ b/core/lib/Drupal/Core/Field/FieldItemInterface.php
   @@ -4,6 +4,7 @@
    
    use Drupal\Core\Form\FormStateInterface;
    use Drupal\Core\TypedData\ComplexDataInterface;
   +use Drupal\Core\Field\FieldStorageDefinitionInterface;
   

   Can we keep these in alphabetical order?

2. --- a/core/modules/field_ui/src/FieldStorageConfigListBuilder.php
   +++ b/core/modules/field_ui/src/FieldStorageConfigListBuilder.php
   @@ -95,6 +97,7 @@ public function buildHeader() {
       * {@inheritdoc}
       */
      public function buildRow(EntityInterface $field_storage) {
   +    /** @var \Drupal\Core\Field\FieldStorageDefinitionInterface $field_storage */
   

   Please remove that line. It is only at the end of the function, in the new lines added for this issue, that we check whether this EntityInterface object also implements FieldStorageDefinitionInterface.

3. --- a/core/modules/field_ui/tests/src/Functional/ManageFieldsTest.php
   +++ b/core/modules/field_ui/tests/src/Functional/ManageFieldsTest.php
   @@ -34,9 +34,12 @@ protected function setUp() {
   ...
   -    /** @var \Drupal\field\FieldStorageConfigInterface $storage */
   +    /** @var \Drupal\Core\Field\FieldStorageDefinitionInterface $storage */
        $storage = $this->container->get('entity_type.manager')
          ->getStorage('field_storage_config')
          ->create([
   

   This code is inside the field_ui module, so we no longer have to avoid using classed from the field module. Did you check what type $storage actually is? Do we have to change this type hint?

4. +    // Add an entity reference field, and check that its summary is custom.
   +    /** @var \Drupal\Core\Field\FieldStorageDefinitionInterface $storage */
   +    $storage = $this->container->get('entity_type.manager')
   +      ->getStorage('field_storage_config')
   +      ->create([
   +        'type' => 'entity_reference',
   +        'field_name' => 'downlander',
   +        'entity_type' => 'node',
   +        'settings' => [
   +          'target_type' => 'node',
   +        ],
   +      ]);
   

   Same question.

(OK, I started by looking at the interdiff, but then I looked at the whole patch.)

Hi,

here is a new patch to adress #53

for the point 3 : i corrected it. actuallay the class is "Drupal\field\Entity\FieldStorageConfig"

hope it'll be green :)

Thanks

hi,
idk what happened with yesterday's patch but here is a new one.
Thanks

applied the codesniffer_fixes.patch from #57 (so it's the interdiff)

Patch rerolled.

@jibran:

Thanks for updating this issue.

I am not sure that the current approach is the best one. I think we could implement this in the field_ui module without touching anything in the Drupal\Core namespace. But for now I will keep the same approach and try to get a working patch that passes tests ... or at least does not have as many failures.

+++ b/core/lib/Drupal/Core/Field/FieldItemBase.php
@@ -43,11 +43,11 @@
-  public static function fieldSettingsSummary(FieldStorageDefinitionInterface $field_config) {
+  public static function fieldSettingsSummary(FieldStorageDefinitionInterface $storage_config) {

I changed the variable name so that it matches the type hint better and avoids confusion with the variable $field_config in the calling function.

-        '@type' => $definitions[$field_config->getFieldStorageDefinition()->getType()]['label'],
+        '@type' => $definitions[$storage_config->getType()]['label'],

This should fix many of the failing tests. We cannot call getFieldStorageDefinition() on an object that implements FieldStorageDefinitionInterface. Instead, we want to call $field_config->getFieldStorageDefinition() in the calling function and pass the result to this function.

+++ b/core/lib/Drupal/Core/Field/FieldTypePluginManagerInterface.php
@@ -91,14 +91,14 @@
-  public function getFieldSettingsSummary(FieldConfigInterface $field_config);
+  public function getFieldSettingsSummary(FieldDefinitionInterface $field_config);

I may still be confused: I expect to get errors without this change, but I do not. I think there is some confusion between the unrelated classes FieldConfigInterface in the namespaces Drupal\field and Drupal\Core\Field. At any rate, we are passing an object of class Drupal\field\Entity\FieldConfig, which implements FieldDefinitionInterface, and then applying two methods of that interface (getType() and getFieldStorageDefinition()), so I think this is the correct type hint.

I have not run tests locally, but at least I get something like what we want on /admin/structure/types/manage/recipe/fields (testing on the Umami installation profile), which is an improvement over the patch in #62.

Hurray, that fixed all of the tests!

I think this patch needs some serious cleanup, but I do not have the heart to set it back to NW after getting the first patch to pass tests since #50.

Here is a new patch. Compared to the one in #64, it makes three changes:

1. Change the variable name back to $field_config
2. Restore an unused use statement
3. Remove an unneeded helper function

There are two reasons for (1): I want to make it easier to compare to the patch in #30 (interdiff attached), and I realized that the patch (this one or the one in #64) introduces a lot of variables with the type hint FieldStorageDefinitionInterface: $field_storage, $field_config, and $field_definition. I plan to make all of these consistent in a later patch.

Removing an unused use statement is a good idea, but it is out of scope for this issue.

The helper function was added in an attempt to get the types to work out correctly. Now that I have sorted out the type hints in #64, it is no longer needed.

Comparing to the patch in #30, the changes are mostly type hints and use statements, plus the change I called out in #64, moving the call to getFieldStorageDefinition() from fieldSettingsSummary() to the calling function. The net result is to fix the problem I called out in #28.1 (see also #44): code in the Drupal\Core namespace should not depend on code in modules.

I think I found a better solution. I reverted the change of where I was calling getFieldStorageDefinition(), so the field config (not the field-storage config) is passed to fieldSettingsSummary(), as it was in earlier patches.

The two FieldConfigInterface interfaces are not unrelated: both of these interfaces extend Drupal\Core\Field\FieldDefinitionInterface. I think that is what we should use for type hints. This interface declares both getType() and getFieldStorageDefinitions().

Compared to the patch in #30 (interdiff attached), my latest patch just makes the following changes to type hints:

• storageSettingsSummary() and getStorageSettingsSummary() use \Drupal\Core\Field\FieldStorageDefinitionInterface instead of \Drupal\field\FieldStorageConfigInterface
• fieldSettingsSummary() and getFieldSettingsSummary() use \Drupal\Core\Field\FieldDefinitionInterface instead of \Drupal\Core\Field\FieldConfigInterface

Applied patch and worked as described by @benjifisher
Screenshot embedded

This patch just makes two kinds of changes:

1. Change variable names to be more consistent with existing usage. In particular,
   • FieldDefinitionInterface $field_config becomes $field_definition.
   • FieldStorageDefinitionInterface $field_definition becomes $field_storage.
2. Update the documentation blocks.
   • Make parameter descriptions more consistent with existing usage.
   • Mention when @return array means a renderable array.

I am updating the issue summary. Thanks to @mcdwayne for providing the screenshot!

Other candidates for implementing the new methods are Media fields, List fields, and perhaps Layout Section fields.

I forgot to mention: the new data also show up on the Field list (/admin/reports/fields). I will add that to the issue summary.

I think this patch is doing a Good Thing -- the proposed UI is, in my opinion, a lot nicer and more useful than what is currently in core. However, this introduces two new methods to heavily-used, and often-extended, pieces of Drupal's API. That means it needs framework manager review in addition to subsystem maintainer sign-off. So, tagging accordingly.

Godspeed.

This patch has some code cleanup. It should not affect the results.

Some of the changes are just to make the code more consistent with existing code in the same file. In particular,

1. I changed $field_storage back to $field_definition, reverting one of the changes I made in #69.
2. I rewrote the new functions in Drupal\field_ui\src\FieldConfigListBuilder so that they do not use an "exit early" strategy.

I noticed a couple of things that I would like to address in follow-up issues. The first is related to my second point above.

1. Add a helper function to avoid having four nearly identical functions (DRY principle).
2. Clean up buildRow() in Drupal\field_ui\src\FieldConfigListBuilder: remove the unused variable $route_parameters and move $field_storage to later in the function, where it can be eliminated.

I did not notice the test failure for #69. The failing test may be related to #3059022: If Vimeo is down our tests break. I do not understand the coding-standards message: yes, that use statement is unneeded, but my patch did not touch it.

I am setting back to NR to see what the testbot does.

I am rewriting the "Proposed solution" section of the summary (again) and moving what used to be there into the "API changes" section.

I am also adding a screenshot of the Field list page.

We discussed this issue at the start of today's Drupal usability meeting 2019-07-02.

We discussed removing the link from entries in the "Field type" column, and agreed it was a good idea.

We agreed to open a follow-up issue(s) to consider reformatting labels such as "Text (formatted, long)" from other core modules.

We discussed the formatting and decided that

1. The markup should use Twig templates so that it can be controlled by the admin theme.
2. The default styling should be normal weight (not bold), and the second line should be in a smaller font than the first line. Be consistent with styling on the "Manage form display" tab, where the third column has a smaller font than the first column.

I am setting the issue back to NW for that.

This turns out to be harder than I expected. I hope you will forgive me for skipping one of the suggestions from #78: I do add CSS classes so that the theme can control the display, but I do not use Twig templates. In part, this is because I looked at the code in Drupal\field_ui\Form\EntityDisplayFormBase, which creates the markup on the "Manage form display" tab. We want to have some consistency with this markup, the existing code (including the use of inline_template) seems to be based on it, and this code does not use Twig templates.

Let me discuss a few snippets from the interdiff:

1. +++ b/core/lib/Drupal/Core/Field/FieldItemBase.php
   @@ -46,7 +46,7 @@
      public static function fieldSettingsSummary(FieldDefinitionInterface $field_definition) {
        $definition = \Drupal::service('plugin.manager.field.field_type')
          ->getDefinition($field_definition->getType())['label'];
   -    return [t('<b>@type</b>', ['@type' => $definition])];
   +    return ['#markup' => $definition];
      }
       

   That gets rid of the hard-coded <b> tags. It also removes a call to the (global) t() function. I do not love the call to the service container, but I already lost a lot of time trying to figure out a better way. Maybe someone else can give me a hint.

2. +++ b/core/modules/field_ui/src/FieldConfigListBuilder.php
   @@ -154,11 +155,10 @@
        $summary = $this->fieldTypeManager->getFieldSettingsSummary($field_config);
        $settings_summary = empty($summary) ? '' : [
          'data' => [
   -        '#type' => 'inline_template',
   -        '#template' => '<div class="field-plugin-summary">{{ summary|safe_join("<br />") }}</div>',
   -        '#context' => ['summary' => $summary],
   -        '#cell_attributes' => ['class' => ['field-plugin-summary-cell']],
   +        '#theme' => 'item_list',
   +        '#items' => $summary,
          ],
   +      'class' => ['field-settings-summary-cell'],
        ];
       

   and similar changes in core/modules/field_ui/src/FieldStorageConfigListBuilder.php. Instead of pasting together a bunch of lines with <br /> tags, I format them as a list. I think this is the best way to make it so that we can apply different styles to the first item.

3. +++ b/core/modules/field_ui/src/FieldConfigListBuilder.php
   @@ -108,6 +108,7 @@
        $build = parent::render();
        $build['table']['#attributes']['id'] = 'field-overview';
        $build['table']['#empty'] = $this->t('No fields are present yet.');
   +    $build['#attached']['library'][] = 'field_ui/drupal.field_ui';
    
        return $build;
      }
   +++ b/core/modules/field_ui/src/FieldStorageConfigListBuilder.php
   @@ -91,0 +92,9 @@
   +  public function render() {
   +    $build = parent::render();
   +    $build['#attached']['library'][] = 'field_ui/drupal.field_ui';
   +    return $build;
   +  }
       

   Curiously, although the comments in the CSS files say that they apply to the "Manage fields" page, they do not until we make this change.

4. +++ b/core/modules/field_ui/css/field_ui.admin.css
   @@ -10,6 +10,17 @@
    .field-ui-overview .region-message td {
      font-style: italic;
    }
   +.field-settings-summary-cell > .item-list > ul > li,
   +.storage-settings-summary-cell > .item-list > ul > li {
   +  list-style-type: none;
   +  margin: 0;
   +}
   +.field-settings-summary-cell > .item-list > ul > li {
   +  font-size: 0.9em;
   +}
   +.field-settings-summary-cell > .item-list > ul > li:first-child {
   +  font-size: 1.0em;
   +}
       

   I also made the same change in the identical file in the stable theme. I am not proud of chaining together four selectors, but I do not see any other way to avoid affecting markup generated by the plugins, which may contain HTML lists.

The markup for the "Field type" column on the "Manage fields" page now looks like this:

  <td class="field-settings-summary-cell">
    <div class="item-list">
      <ul>
        <li>Entity reference</li>
        <li>Referenced entity type: Taxonomy term</li>
      </ul>
    </div>
  </td>

The difference in font size is subtle, at least on my system (Firefox on Linux). Here is a screen shot:

Here is a screenshot of the "Field list" page:

The attached patch updates the tests to match the updated markup. These tests are added as part of this patch, so I do not feel bad about changing them.

I also removed the unused use statement. This line was removed in a previous patch, but I restored it in #66, saying it was out of scope. I was wrong. Since we are making the "Field settings" column plain text instead of a link, we no longer use Url::fromRoute(), which is the only reference to the Url class.

I just tested this patch on a site that uses a lot of Paragraph types (and ERR fields).

Good news: Since the ERR (entity reference revision) field type extends the ER (entity reference) field type, the patch works without any changes to the Paragraphs nor ERR modules.

Bad news: All it tells me is "Entity reference revisions // Referenced entity type: Paragraph".

Conclusion: If the entity type has bundles, then we should add bundle information to the output.

> Conclusion: If the entity type has bundles, then we should add bundle information to the output.

There's no API to get that however, jsonapi is struggling with the same thing. While ERR's configuration keys basically match ER, it for example has the negate setting, which actually turns the whole thing around and then it would be wrong. Also, both ER and specifically also ERR/paragraph fields often allow *a lot* of bundles, which would make for very long lists..

There's no API to get that

I am not sure what you are saying: there is no API to find out whether an entity type has bundles? (I think this is what you mean. Looking at NodeTypeListBuilder, I see that /admin/structure/types lists content types because they are defined by configuration. But not all entity types have bundles described by configuration, right?)

Also, both ER and specifically also ERR/paragraph fields often allow *a lot* of bundles, which would make for very long lists..

Good point. Listing the bundles should not be the default. Maybe it should not even be in Drupal core. If not, then the API we introduce in this issue should provide an alter hook or an event listener so that a contrib module could provide the option to list bundles.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Rerolled #82

Opened an MR that applies patch #99, fixes the failed test, and added the new styling to the claro theme as well

Updating the screenshot in the issue summary with a recent screenshot that uses Claro, and shows the bundle information that was just made visible by recent commits to the MR. 😇

This infra commit fixed the composer issue

I brought this up with the usability group to discuss the "Target entity type" text. I had the impression that we generally avoid using the term "entity" in the UI whenever that's possible. It seems like that is the case, and for that reason the group recommends to use "Reference type" instead.

Something else that the group suggested was looking into alternative ways of presenting the different summaries. Currently, they are displayed as a list and each item is on it's own line. However, from usability standpoint, it might be better to display them differently. Within the group there was consensus that this issue is a net win even without addressing this concern and therefore addressing these concerns could be left for a follow-up issue.

This has also reviewed by the Usability group in the past, as documented in #78. As part of this, the group had reviewed the removal of the link.

At the moment it is unclear to where the link is pointing, rendering it useless for most users. Removing the link may cause some friction for users who are who are accustomed to using the link for checking information about the field storage (i.e. cardinality). However, there is a pre-existing link to the same destination under the operations, with a much clearer link text. We may have to look into what other information users need from the field storage page as an additional follow-up.

We discussed this issue at #3340893: Drupal Usability Meeting 2023-02-17. That issue will have a link to a recording of the meeting. (See also #109.)

For the record, the attendees at the usability meeting were @AaronMcHale, @BlackBamboo, @benjifisher, @gaurav mahlawat, @lauriii, @rkoller, @shaal, and @simohell.

We specifically discussed the new text for an entity reference (ER) field. From the screenshot in the issue description:

Entity reference
Referenced entity type: Media
Media type: Image, Video

For this issue, we would like to change "Referenced entity type" to "Reference type", as @lauriii said in #109. Either as part of this issue or as a follow-up, we would like to combine the first two lines. Some possibilities are "Media reference", "Reference: Media", or "Reference (Media)". Depending on which modules are enabled, some possibilities are two words or longer ("Taxonomy term", "Content moderation state", ...).

One guideline is to be consistent with the forms for adding a new field and for editing the field-storage settings. When adding a field, the select list has "Reference" as a group header and Content, File, Image, ... as options in that group. When editing the field-storage settings, the text is "Type of item to reference".

This seems really close but this one feature seemed off

Don't think these fields need a reference type right?

Pushed commit which should address #111 👍

#111 has been fixed so +1 from me for RTBC.

Not moving it for subsyste, framework manager, and usability review.

Think after that happens change record can be written.

Also was tagged for follow up 4 years ago if that still needs to happen.

Updating issue summary since this has received multiple rounds of usability review.

Reviewed the MR and I think it looks great! Posted a few minor points, should be an easy RTBC after that :)

Opened follow-up issues:

1. #3346506: [PP-1] Remove unused $entity_type_manager constructor argument from \Drupal\field_ui\FieldConfigListBuilder
2. #3346509: [PP-1] Some of the field summaries are excessively verbose

Based on #115, moving to RTBC for a framework manager review.

This issue is required by #3346682: Improve re-use an existing field user experience.

This probably needs a non FE framework manager signoff as that's where most of the changes are, but this is looking good to me. It introduces some smaller than usual fonts in the details, but the .9rem is well above the 9px needed for AA compliance. I also tried this out with Paragraphs, and paragraphs fields in content types look great, as do fields within paragraphs.

As unlikely a use case as this might be, may be worth if the CR includes info on the array returned by \Drupal\field_ui\FieldConfigListBuilder::buildHeader as it no longer returns a field_type key and now has settings_summary. I went through https://git.drupalcode.org/search to see if there's any evidence of this impacting contrib in any way, and it certainly doesn't look like it... it just seems like an easy enough base to cover.

Hiding patches since there's an MR.

Left a comment on the MR about the module stylesheet. If that's addressed with something simple like a selector change I'd say go ahead and switch back to RTBC after the change.

Addressed the feedback regarding the CSS selector.

After many high quality reviews from many excellent contributors, I've committed this to 10.1.x, and opting to not backport since it is a UI change that isn't addressing a bug.

For context: This ticket was filed two months before the release of "Avengers: Age of Ultron" and "Mad Max: Fury Road" (which I saw right after returning from Drupalcon LA). The number one movie in the US on the actual date was "The Divergent Series: Insurgent". Nice work everyone, great to see this UX improvement added to core!

Published change record