The "rendered entity" formatter breaks for entity types with out a ViewBuilder

can you leave a little more info and detail about this problem.

1. +++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/Field/FieldFormatter/EntityReferenceEntityFormatter.php
   @@ -95,12 +95,12 @@ public function viewElements(FieldItemListInterface $items) {
   -      if (!empty($item->target_id)) {
   +      if (!empty($item->target_id) && $item->entity->getEntityType()->hasViewBuilderClass()) {
   

   This should have test coverage, by referencing some config_test entity that doesn't have a view builder.

2. +++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/Field/FieldFormatter/EntityReferenceEntityFormatter.php
   @@ -95,12 +95,12 @@ public function viewElements(FieldItemListInterface $items) {
   -        if (empty($links) && isset($result[$delta][$target_type][$item->target_id]['links'])) {
   +        if (empty($links) && isset($elements[$delta][$target_type][$item->target_id]['links'])) {
              // Hide the element links.
              $elements[$delta][$target_type][$item->target_id]['links']['#access'] = FALSE;
   

   I'm guessing this is for the links that are added onto nodes, and it was broken as you said. So we need coverage for that as well.

See #1857422: Add a ViewBuilder for Views for changing the views module.

The 'links' functionality in this formatter is way more broken than this, so let's fix it separately with #2274975: Display/hide links on Rendered Entity formatter very broken.

I feel like we should be offering a solution a little higher-up, and not in the viewItems() method. If you think about it, the user shouldn't even be able to select this as a formatter if the entity type in the field settings doesn't support a view controller. Why offer a choice that will just never work?

True, but I don't we currently have an API that allows to check if a formatter is available for a given field type?

Ideally formatters would have some kind of access() method, but I wonder if for now, we should just add drupal_set_message warning in the formatter settings form if a view controller isn't available.

Another great use case for having some kind of access() method is EntityReferenceTaxonomyTermRssFormatter - should only be visible to entity reference fields which have a target type of 'taxonomy_term' and not anything else.

Another use case: I shouldn't be able to use the core/modules/responsive_image/src/Plugin/Field/FieldFormatter/ResponsiveImageFormatter.php formatter if I don't have any responsive image mappings created.

access() method would also be useful for any contrib formatter that works on entity_reference but might be limited to a single entity_type. Can definitely see this kind of use-cases in media ecosystem.

Would it also make sense to do the same on field widgets?

I think so yes.

Attached patch implements the basics to make this work, manually tested by adding a entity reference to a config entity without view builder, rendered entity and the taxonomy thing no longer shows up.

Will start writing the missing tests for this patch.

Created the test for the available entity reference field formatters.

Also fixed a bug in the assertFieldSelectOption and getAllOptionsList methods.
assertFieldSelectOption failed to correctly check for identical arrays, instead it was checking for identical sort() return values which were both true most of the time.
getAllOptionsList failed to correctly return all the option if the select element was using option groups, now loops trough all items in all option groups.

1. +++ b/core/modules/entity_reference/src/Tests/EntityReferenceAdminTest.php
   @@ -101,6 +102,101 @@ public function testFieldAdminHandler() {
   +    // Create a new vocabulary
   +    Vocabulary::create(array('vid' => 'tags', 'name' => 'tags'))->save();
   

   missing .

2. +++ b/core/modules/entity_reference/src/Tests/EntityReferenceAdminTest.php
   @@ -101,6 +102,101 @@ public function testFieldAdminHandler() {
   +    // Create a taxonomy term Entity Reference Field.
   +    $taxonomy_term_field_name = $this->createEntityReferenceField('taxonomy_term', 'tags');
   +
   +    // Create a node Entity Reference Field.
   +    $node_field_name = $this->createEntityReferenceField('node', $this->type);
   +
   +    // Create a data format Entity Reference Field.
   +    $date_format_field_name = $this->createEntityReferenceField('date_format');
   

   This reads a bit strange. What about "Create entity reference field with X as a target."?

3. +++ b/core/modules/entity_reference/src/Tests/EntityReferenceAdminTest.php
   @@ -101,6 +102,101 @@ public function testFieldAdminHandler() {
   +    // Display all newly created Entity Reference Fields.
   +    $this->drupalGet('admin/structure/types/manage/' . $this->type . '/display');
   +
   

   Not displaying fields but display configuration.

4. +++ b/core/modules/entity_reference/src/Tests/EntityReferenceAdminTest.php
   @@ -101,6 +102,101 @@ public function testFieldAdminHandler() {
   +    // Check for Taxonomy Term select box values
   +    // Test if Taxonomy Term Entity Reference Field has the correct formatters.
   +    $this->assertFieldSelectOptions('fields[field_' . $taxonomy_term_field_name . '][type]',
   

   Missing .

5. Should we also test field widgets?

Modified comments & Added tests for field widgets.

Drupal username for attribution:
https://www.drupal.org/u/arla

1. +++ b/core/modules/field/tests/modules/field_test/src/Plugin/Field/FieldWidget/TestFieldWidgetMultiple.php
   @@ -95,4 +96,12 @@ public static function multipleValidate($element, &$form_state) {
   +  public static function isApplicable(FieldDefinitionInterface $field_definition) {
   +    // Returns false if machine name equals field_onewidgetfield.
   +    return $field_definition->getName() != "field_onewidgetfield";
   +  }
   

   I suggest you add a reference here to the test where this is used for. like See/Used in full\class\name::testMethod().

2. +++ b/core/modules/field_ui/src/Tests/ManageDisplayTest.php
   @@ -222,6 +222,21 @@ public function testWidgetUI() {
   +
   +    // Creates a new field through the Field UI.
   +    $edit = array(
   +      'fields[_add_new_field][label]' => 'One Widget Field',
   +      'fields[_add_new_field][field_name]' => 'onewidgetfield',
   +    );
   

   Explain here why you are creating this, "Creates a new field that can not be used with the multiple formatter. And add a back-reference to the code in the widget that checks for it.

Changed a few comments and made references back and forward to and from the test field widget class.

This was the main reason I argued for "one separate entity_ref field type for each target entity type" instead of "one single entity_ref field, whith target type as a setting" in #1801304: Add Entity reference field : granularity of applicable formatters and widgets.

We sure can add an API for dynamic eligibility at runtime, but the pitfalls are (from memory from the discussion back then, there might be others raised in the issue over there) :

- How do we make that predictable for site admins ?
"Why is this formatter not available on this field while it is for that other field ? What do I have to do to make that formatter available ?"
The current solution gives no feedback and only lets site admins click and guess.

- What if a display has been configured for a formatter that was eligible, and the field is then modified so that the formatter is no longer eligible ? What's the good, non confusing, predictable behavior for that ?
(Widget|Formatter)PluginManager::getInstance() implement a fallback to the 'default formatter' for the field type if the configured plugin has been uninstalled, do we do the same ? That would imply that the 'default formatter' needs to always be eligible, how do we enforce that ?

[min-rant]
The current model for widgets / formatters, which provided a structuring stable ground since CCK D4.7, is "formatters are eligible per field type, model you field types accordingly". The decision in #1801304: Add Entity reference field was "meh, we'll figure it out". So let's :-p.
[/mini-rant]

While I agree that there are some pitfalls with this, per-target-type entity reference fields would a) be waaaay to many by now, with the possibility to have config entity references and b) would not really solve the problem here, we wouldn't want to hardcode the list of entity types that have a view builder?

I don't think the problems you mentioned are that problematic, the widgets/formatters would be responsible themself to have logic that makes sense to users, the specific examples we have are on the target_type, which is not something you can change and it makes more sense to me to not have the option "Rendered entity" for something like a filter format reference instead of having it and it then resulting in errors/not doing anything.

we wouldn't want to hardcode the list of entity types that have a view builder?

Would be determined at runtime in hook_field_formatter_info_alter().

But anyway, yeah, I'm *not* saying we should go back to "one entity_ref field type per target entity type" at this point :-)

the specific examples we have are on the target_type, which is not something you can change

Sure, that happens to be true for specific the use case we're discussing here; but we're introducing a generic API that can be used for many other cases. I'm trying to point the fallbacks of the general concept.

Oops, unfortunate typo - I'm *not* saying we should go back to "one entity_ref field type per target entity type" at this point :-p
Fixed the previous post.

Sure, that happens to be true for specific the use case we're discussing here; but we're introducing a generic API that can be used for many other cases. I'm trying to point the fallbacks of the general concept.

Sure, but that is true for many generic concepts, like most alter hooks we have ;) But what can we do about this other than document it with "Don't be evil."? I'm fine with adding something like "The default widget/formatter of a given field_type must always be available." And we currently only apply this to the UI. We either need a generic concept or hook_form_alter() and I really don't want to go there :)

Sure, but that is true for many generic concepts, like most alter hooks we have

I don't think I get the analogy. hook_field_formatter_info_alter() lets you change the fields types for which a formatter is eligible, but doesn't change the fact that eligibility is currently defined purely by the field type, and is thus not "sometimes yes sometimes no, for reasons that will remain untold" ?

And we currently only apply this to the UI. We either need a generic concept or hook_form_alter() and I really don't want to go there :)

Not sure I get the end of the comment either :-)

Other than that, the current patch keeps the 'field_types' entry in the annotations as a 1st eligibility criteria, and adds a new, more granular step as a static method on top of that. Is that really what we want ? Shouldn't we go with only a method then ? Other than history, is there a reason to go with a 2-level eligibility system ?

Sorry, with the first part, I meant that without an API to do this in a generic way, entity_reference.module would have to implement hook_form_alter() on the field_ui manage display form to hide the options that shouldn't be there for this case.

And with the alter hooks, yes you can't make it dynamic, but you can do enough crazy things with most of our alter hooks (like removing or changing a default formatter for a field type). My point was that we already trust developers enough to give them alter hooks, so I don't see the difference to this.

hook_form_alter(): ah, right - yes, agreed that we don't want that.

Rerolled last patch to current tip of core.

High-level feedback: I also encountered this problem while working on the various "[entity type] cache tags" test classes (all subclasses of EntityCacheTagsTestBase). I fixed all of the content entity types in the process (IIRC) that should have a view builder, and added conditional logic to not render the entity but link to the entity if the entity if it doesn't have a view builder (for some entity types it simply does not make sense), but this indeed should be fixed in a generic way.

+++ b/core/lib/Drupal/Core/Field/FormatterInterface.php
@@ -87,4 +87,16 @@ public function view(FieldItemListInterface $items);
+  public static function isApplicable(FieldDefinitionInterface $field_definition);

This could theoretically have interesting consequences for Quick Edit, but since this relies only on the field definition, not on the values of a field instance, this is fine.

Nitpicks:

1. +++ b/core/lib/Drupal/Core/Field/FormatterInterface.php
   @@ -87,4 +87,16 @@ public function view(FieldItemListInterface $items);
   +   *
   +   */
   
   +++ b/core/lib/Drupal/Core/Field/WidgetInterface.php
   @@ -153,4 +153,16 @@ public function errorElement(array $element, ConstraintViolationInterface $viola
   +   *
   +   */
   

   Extraneous newlines.

2. +++ b/core/modules/entity_reference/src/Tests/EntityReferenceAdminTest.php
   @@ -101,6 +102,101 @@ public function testFieldAdminHandler() {
   +    // Check for Taxonomy Term select box values.
   +    // Test if Taxonomy Term Entity Reference Field has the correct formatters.
   +    $this->assertFieldSelectOptions('fields[field_' . $taxonomy_term_field_name . '][type]',
   +      array(
   +        'entity_reference_label',
   +        'entity_reference_entity_id',
   +        'entity_reference_rss_category',
   +        'entity_reference_entity_view',
   +        'hidden',
   +      ));
   +
   +    // Test if Node Entity Reference Field has the correct formatters.
   +    // RSS Category should not be available for this field.
   +    $this->assertFieldSelectOptions('fields[field_' . $node_field_name . '][type]',
   +      array(
   +        'entity_reference_label',
   +        'entity_reference_entity_id',
   +        'entity_reference_entity_view',
   +        'hidden',
   +      ));
   +
   +
   +    // Test if Date Format Reference Field has the correct formatters
   +    // RSS Category & Entity View should not be available for this field.
   +    // This could be any field without a ViewBuilder.
   +    $this->assertFieldSelectOptions('fields[field_' . $date_format_field_name . '][type]',
   +      array(
   +        'entity_reference_label',
   +        'entity_reference_entity_id',
   +        'hidden',
   +      ));
   

   Strange things here: double newline, incorrect indentation (unless this is a new standard), consecutive //-style comments that possibly should be /* … */, a missing period.

3. +++ b/core/modules/field_ui/src/Tests/ManageDisplayTest.php
   @@ -143,10 +143,13 @@ function testFormatterUI() {
   +    // Creates a new field that can be used with multiple formatter.
   

   s/formatter/formatters/

4. +++ b/core/modules/field_ui/src/Tests/ManageDisplayTest.php
   @@ -204,8 +207,7 @@ public function testWidgetUI() {
   +    // Assert that the field added in field_test_field_widget_third_party_settings_form() is present.
   

   80 cols.

5. +++ b/core/modules/field_ui/src/Tests/ManageDisplayTest.php
   @@ -222,6 +224,22 @@ public function testWidgetUI() {
   +    $this->assertFieldSelectOptions('fields[field_onewidgetfield][type]', array('test_field_widget', 'hidden'));
   +
   

   Extraneous newline.

Fixed the nitpicks :)

+++ b/core/modules/entity_reference/src/Tests/EntityReferenceAdminTest.php
@@ -101,6 +102,97 @@ public function testFieldAdminHandler() {
+    // Generate a random field name, must be only lowercase characters.
+    $field_name = strtolower($this->randomName());
+
+    // Create the initial entity reference.
+    $this->drupalPostForm($bundle_path . '/fields', array(
+      'fields[_add_new_field][label]' => $this->randomName(),
+      'fields[_add_new_field][field_name]' => $field_name,
+      'fields[_add_new_field][type]' => 'entity_reference',
+    ), t('Save'));
+

This is randomMachineName() now..

Changed to randomMachineName().

Looks good. Found a typo.

+++ b/core/modules/entity_reference/src/Tests/EntityReferenceAdminTest.php
@@ -101,6 +102,97 @@ public function testFieldAdminHandler() {
+    // Create entity reference field with data format as a target.
+    $date_format_field_name = $this->createEntityReferenceField('date_format');
+

s/data/date

Fixed #39.

Other than that, the current patch keeps the 'field_types' entry in the annotations as a 1st eligibility criteria, and adds a new, more granular step as a static method on top of that. Is that really what we want ? Shouldn't we go with only a method then ? Other than history, is there a reason to go with a 2-level eligibility system ?

This bothers me as well a little bit, but it probably shouldn't hold up this patch.

Committed b5c736d and pushed to 8.0.x. Thanks!

diff --git a/core/modules/field_ui/src/DisplayOverviewBase.php b/core/modules/field_ui/src/DisplayOverviewBase.php
index 73f353f..1bad243 100644
--- a/core/modules/field_ui/src/DisplayOverviewBase.php
+++ b/core/modules/field_ui/src/DisplayOverviewBase.php
@@ -13,7 +13,6 @@
 use Drupal\Core\Config\ConfigFactoryInterface;
 use Drupal\Core\Entity\Display\EntityDisplayInterface;
 use Drupal\Core\Entity\EntityManagerInterface;
-use Drupal\Core\Field\FieldDefinition;
 use Drupal\Core\Field\FieldDefinitionInterface;
 use Drupal\Core\Field\FieldTypePluginManagerInterface;
 use Drupal\Core\Field\PluginSettingsInterface;

Removed the usued use on commit - this class had changed it's name to BaseFieldDefinition but fortunately neither that not FieldDefinition is actually being used in DisplayOverviewBase.

(Widget|Formatter)PluginManager::getInstance() needs to fallback to the 'default' widget/formatter if the currently configured formatter is not "applicable" anymore.

Here are some tests for that. They will fail until the actual fallback is implemented.

I corrected the tests, and also wrote a quick fix that seems to pass. It probably needs improvement.

Passing ManagerDisplayTest now. Also updated some comments. Still not sure if the added if-conditions in *PluginManager are reasonable.

Oops, regenerating patch. The interdiff was correct, though.

Looks good.

Committed c2a74e6 and pushed to 8.0.x. Thanks!

Thanks @Arla, the committed followup looks good to me too.

Fix media tag.