Fatal Error on re-saving required EntityReference field setting

Here's a test proving that this is broken.

Upgrading to critical, because this is a Fatal you can trigger from the UI and adding a required Entity reference field is not really an edge case.

This implements the solution from the issue summary. Since WidgetBase already has an errorElement() method which just returns $element I removed the entire method.

We might want to commit this as is, it's certainly an improvement over the status quo and the fix is correct.

The problem mentioned in the issue summary remains, however.

Nope, there's nothing wrong with \Drupal\entity_reference\Plugin\Field\FieldWidget\AutocompleteWidgetBase::errorElement(), this bug is a bit more subtle than that.

The problem here is that \Drupal\Core\Field\FieldItemList::defaultValuesFormValidate() calls the validate() method on itself but it does not account for the fact that, when dealing with the default value form / widget, the field definition should *not* be required. See the code that does this for the widget form itself in \Drupal\Core\Field\FieldItemList::defaultValueWidget().

Here's the correct fix :)

Also fixed a comment.

I just ran into this issue myself as well. It also fixes the problem that you can't switch the reference method for a required field. (The ajax-request generates the error about the target_id as well)

After applying this patch (to d8 beta2), these problems were gone!

Hmm... #4 fixes a separate bug so is legit as well IMO.

@tstoeckler, I don't see what bug is being fixed by #4.

Field widgets that do not return the $element they receive in formElement() and wrap it in another array, have to implement errorElement() to let the system know about their wrapper. See PathWidget and NumberWidget for other examples and the docblock of WidgetInterface::errorElement().

+++ b/core/lib/Drupal/Core/Field/FieldItemList.php
@@ -336,6 +336,9 @@ public function defaultValuesFormValidate(array $element, array &$form, FormStat
+    // Force a non-required field definition.
+    // @see self::defaultValueWidget().
+    $this->definition->required = FALSE;

--- a/core/modules/entity_reference/src/Tests/EntityReferenceAdminTest.php
+++ b/core/modules/entity_reference/src/Tests/EntityReferenceAdminTest.php

This affects all fields - right? Seems strange to bury the test for this in EntityReferenceAdminTest - I would have expected a test in ManageFieldsTest.

Re #9: The thing is that AutocompleteWidgetBase does not define a 'target_id' sub-element. That's why the current implementation of errorElement() is wrong. It is proven by the "undefined index" error in the IS and in #1.

Ohboy...

  public function formElement(FieldItemListInterface $items, $delta, array $element, array &$form, FormStateInterface $form_state) {
    $entity = $items->getEntity();

    // Prepare the autocomplete route parameters.
    $autocomplete_route_parameters = array(
      'type' => $this->getSetting('autocomplete_type'),
      'field_name' => $this->fieldDefinition->getName(),
      'entity_type' => $entity->getEntityTypeId(),
      'bundle_name' => $entity->bundle(),
    );

    if ($entity_id = $entity->id()) {
      $autocomplete_route_parameters['entity_id'] = $entity_id;
    }

    $element += array(
      '#type' => 'textfield',
      '#maxlength' => 1024,
      '#default_value' => implode(', ', $this->getLabels($items, $delta)),
      '#autocomplete_route_name' => 'entity_reference.autocomplete',
      '#autocomplete_route_parameters' => $autocomplete_route_parameters,
      '#size' => $this->getSetting('size'),
      '#placeholder' => $this->getSetting('placeholder'),
      '#element_validate' => array(array($this, 'elementValidate')),
      '#autocreate_uid' => ($entity instanceof EntityOwnerInterface) ? $entity->getOwnerId() : \Drupal::currentUser()->id(),
    );

    return array('target_id' => $element);
  }

@tstoeckler, are we looking at the same code here? :)

http://cgit.drupalcode.org/drupal/tree/core/modules/entity_reference/src...

Hmm... you're right. Sorry about that. Still patch #1 proves that this is "valid" with some definition of valid. I'll have to debug that in detail. But in that case let's proceed here. Sorry for holding this up.

adding needs issue summary update, because I'm not sure if the items in the remaining tasks still need to be done, or if there needs to be a separate issue opened.
If this is going to be used for the Friday Critical Office Hours, then we need to be more clear here.

Heh, I was wondering earlier today what happened to this issue.

IMO, both the proposed resolution and the remaining tasks from the current issue summary are inaccurate because there's nothing wrong with \Drupal\entity_reference\Plugin\Field\FieldWidget\AutocompleteWidgetBase::errorElement(), as proven by the patch in #6.

The only thing needed here in addition to #6 is a test in ManageFieldsTest that checks the ->required property of the field definition to be always FALSE, regardless of the actual (stored) field definition.

I've quickly checked to start a test in managefields in testDefaultValue(). However, it wasn't possible to get this fail on test_field.
So I'm starting to doubt this is indeed the right fix and starting to slowly agree with @tstoeckler that the problem lies in AutocompleteWidgetBase()/AutocompleteWidget.

Here you go :) Apparently it only happens when the field has unlimited cardinality.

Oh wow, never thought about that .. ok looks fine to me.

Also tagging.

+++ b/core/modules/field_ui/src/Tests/ManageFieldsTest.php
@@ -359,6 +359,24 @@ function testDefaultValue() {
+    $this->assertText("Saved $field_name configuration", 'The form was successfully submitted.');

Actually, shouldn't we use format_string here ?

So we don't do it either in the same test. Could potentially be a novice follow up to change those, but shouldn't hold up this patch.

Good to go

This issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 01d9799 and pushed to 8.0.x. Thanks!