Provide a generic 'entity_autocomplete' Form API element

Here's an initial review.

+++ b/core/modules/entity_reference/entity_reference.module
@@ -64,6 +64,45 @@ function entity_reference_entity_field_info_alter(&$info, $entity_type) {
+ * hook_element_info_alter().

Implements ...

+++ b/core/modules/entity_reference/entity_reference.module
@@ -64,6 +64,45 @@ function entity_reference_entity_field_info_alter(&$info, $entity_type) {
+ * Adds a new process callback to handle the #entity_reference hash key.
+ * The items bellow described the list of accepted parameters for this hash key:
+ *   - type: (optionnal, default: tags) [single|tags]
+ *   - handler: (optionnal, default: default) the entity_reference handler.
+ *   - target_type: the target entity type.
+ *   - handler_settings: (optionnal) An array of handler settings such as
+ *     target_bundles, sort and auto_create.

This comment should be moved to the doxygen block of entity_reference_process_element (and translated to english? :P).

+++ b/core/modules/entity_reference/entity_reference.module
@@ -64,6 +64,45 @@ function entity_reference_entity_field_info_alter(&$info, $entity_type) {
+  array_unshift($type['textfield']['#process'], 'entity_reference_process_element');

The process callback should be better named 'entity_reference_textfield_element_process'.

+++ b/core/modules/entity_reference/entity_reference.module
@@ -64,6 +64,45 @@ function entity_reference_entity_field_info_alter(&$info, $entity_type) {
+ * Adds entity_rerefence functionality to elements with a valid
+ * #autocomplete_path. Also attach the entity_reference validation callback on
+ * the element.
+ *
+ * @param array $element
+ *   The form element to process.
+ * @param array $form_state
+ *   The form state array.
+ *
+ * @return array
+ *   The form element.
+ */
+function entity_reference_process_element($element, &$form_state) {
+  if (empty($element['#autocomplete_path']) && !empty($element['#entity_reference'])) {
+    $element['#autocomplete_path'] = url(
+      'entity_reference/form_element/autocomplete/' . base64_encode(serialize($element['#entity_reference'])),
+      array("absolute" => TRUE)
+    );
+    $element['#element_validate'] = array(array(drupal_container()->get('entity_reference.autocomplete'), 'elementValidate'));
+  }
+  return $element;

You don't care if #autocomplete_path is valid or not because you generate one in this process callback. Also the doxygen block should follow the coding standards.. see for example: http://drupalcode.org/project/drupal.git/blob/refs/heads/8.x:/core/modul...

The autocomplete path doesn't need to be passed through url().

drupal_container()->get() should be replaced with Drupal::service().

+++ b/core/modules/entity_reference/entity_reference.routing.yml
@@ -5,3 +5,10 @@ entity_reference.autocomplete:
\ No newline at end of file

As the comment says, an empty line is needed at the end of the file :)

+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/EntityReferenceAutocomplete.php
@@ -99,4 +99,82 @@ public function getMatches($field, $instance, $entity_type, $entity_id = '', $pr
+  public function elementValidate($element, &$form_state, $form) {

This basically duplicates both widget validation methods provided by ER.. can't we unify them instead?

+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/EntityReferenceController.php
@@ -97,4 +97,59 @@ public function handleAutocomplete(Request $request, $type, $field_name, $entity
+  public function handleElementAutocomplete(Request $request, $settings) {

This one duplicates a part of the existing handleAutocomplete() method. We should get the common parts and move them to a protected method instead.

Sorry for slacking with the review here.. I'll give it another look this weekend :)

+++ b/core/modules/entity_reference/entity_reference.moduleundefined
@@ -76,6 +76,44 @@ function entity_reference_field_widget_info_alter(&$info) {
+function entity_reference_element_info_alter(&$type) {
+  array_unshift($type['textfield']['#process'], 'entity_reference_textfield_element_process');
+}
...
+function entity_reference_textfield_element_process($element, &$form_state) {
+  if (!empty($element['#entity_reference'])) {
+    $element['#autocomplete_path'] = 'entity_reference/form_element/autocomplete/' . base64_encode(serialize($element['#entity_reference']));
+    $element['#element_validate'] = array(array(Drupal::service('entity_reference.autocomplete'), 'elementValidate'));
+  }
+  return $element;

I'm wondering whether instead there could be a element which for itself

+++ b/core/modules/entity_reference/entity_reference.routing.ymlundefined
--- a/core/modules/entity_reference/lib/Drupal/entity_reference/EntityReferenceAutocomplete.php
+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/EntityReferenceAutocomplete.phpundefined

+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/EntityReferenceAutocomplete.phpundefined
@@ -97,4 +97,82 @@ public function getMatches($field, $instance, $entity_type, $entity_id = '', $pr
+  public function elementValidate($element, &$form_state, $form) {

It feels wrong to couple the nice service of entity reference autocompletion with formapi related code.

spelling.

Working on #2211553: Allow usage of entity_reference field widget and formatter for base fields I found that handle() should accept entity_type and fieldDefinition but base fields does not have bundle property #2213597: FieldDefinition for base fields should know it's bundle

This patch will be simplified a lot if we do #2107243: Decouple entity reference selection plugins from field definitions first, so postponing on that.

#2107243: Decouple entity reference selection plugins from field definitions landed, unpostponed!

On it :)

Does this allow us to move ER widgets out of e_r.module and into Core, like we did for formatters a bit earlier ?

Form #2107243-15: Decouple entity reference selection plugins from field definitions

Right now in core there is no secure way to use entity autocomplete see the discussion in #2341357-12: Views entity area config is not deployable and missing dependencies. This fix will allow us to use selection plugin for entity autocompletes and selection plugin have proper access checks for all entities.

Because of this reason this is task.
Add BE and updated IS. Please improve.

Does this allow us to move ER widgets out of e_r.module and into Core, like we did for formatters a bit earlier ?

Exactly! Let's see how big the initial patch will be and decide then if we want to do it in the same issue.

I've been working on this in the past few days, trying to come up with a nice API for the generic form element, and I think I mostly succeeded based on how clean is the code for ER's field widgets now.

There are still a few open questions marked with @todo in the patch, so opinions are welcome! And I'll expand the test coverage a bit after the architectural discussions are over.

Looks awesome :-)

Not an in-depth review, just a couple remarks and thoughts on @todos after a first cursory look

1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/AutocompleteWidget.php
   @@ -75,28 +95,18 @@ public function settingsSummary() {
   +      '#autocreate_bundle' => $this->getAutocreateBundle(),
   

   The function can currently return NULL (if several bundles eligible), what happens then ?

2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/AutocompleteWidget.php
   @@ -110,9 +120,19 @@ public function errorElement(array $element, ConstraintViolationInterface $error
   -  public function elementValidate($element, FormStateInterface $form_state, $form) { }
   +  public function massageFormValues(array $values, array $form, FormStateInterface $form_state) {
   

   Sweet :-)

3. +++ b/core/lib/Drupal/Core/Render/Element/EntityAutocomplete.php
   @@ -0,0 +1,169 @@
   +    // @todo Use an array for the 'autocreate API' below?
   +    $info['#autocreate'] = FALSE;
   +    $info['#autocreate_bundle'] = NULL;
   +    $info['#autocreate_uid'] = \Drupal::currentUser()->id();
   

   Was wondering that too, since there's no need to figure out a value for the last two if the first one is FALSE.
   So maybe just

   $element['#autocreate'] = array(
     'bundle' => ...,
     'uid' => ...
   );
   would be clearer ? If you don't want autocreate, just don't put an '#autocreate' entry ?
   
   Then, not sure if the same reasoning applies to the various #autocomplete_* entries...
   </li>
   
   <li>
   <code>
   +++ b/core/lib/Drupal/Core/Render/Element/EntityAutocomplete.php
   @@ -0,0 +1,169 @@
   +    $info['#element_validate'] = array(array($class, 'validateEntityAutocomplete'));
   +    array_unshift($info['#process'], array($class, 'processEntityAutocomplete'));
   

   thanks for the static methods ;-)

4. +++ b/core/modules/system/src/Controller/EntityReferenceController.php
   @@ -0,0 +1,83 @@
   + * @todo Should we rename this to EntityAutocompleteController?
   + */
   +class EntityReferenceController extends ControllerBase {
   

   definitely +1

5. +++ b/core/modules/system/src/Controller/EntityReferenceController.php
   @@ -0,0 +1,83 @@
   +    if ($input = $request->query->get('q')) {
   +    // Get the typed string, if exists from the URL.
   

   Indent

6. +++ b/core/modules/system/src/EntityReferenceAutocomplete.php
   @@ -2,17 +2,14 @@
   - * Contains \Drupal\entity_reference/EntityReferenceAutocomplete.
   + * Contains \Drupal\system\EntityReferenceAutocomplete.
   

   Class is just being moved around, but a name nitpick is something I have a hard time resisting...

   The service and class name do not really help understanding what the object is and does - it's not an "autocomplete" :-) AutocompleteMatcher maybe ?

Awesome work!

Just fixing the patch for now, those fails are depressing.

Hm, that's not the correct fix, more cleanup coming tomorrow :)

Let's see if I got them all properly this time.

Re #17, thanks for starting the reviews :)

The function (AutocompleteWidget::getAutocreateBundle()) can currently return NULL (if several bundles eligible), what happens then ?

No "autocreation" is happening in that case, that's why I put a @todo there that links to #2412569: Allow setting the auto-create bundle on entity reference fields with multiple target bundles.

So maybe just

$element['#autocreate'] = array(
  'bundle' => ...,
  'uid' => ...
);

would be clearer ? If you don't want autocreate, just don't put an '#autocreate' entry ?

Yep, that's exactly what I had in mind. The only problem is that I don't know where to document this stuff. If all the #autocreate_* keys are receiving default values in the form element class, at least people can go there and see them. Maybe there is a standard way of doing this and I have to find out which is it :)

thanks for the static methods ;-)

Wouldn't have done it any other way :D

@todo Should we rename this to EntityAutocompleteController?

definitely +1

Done.

The service and class name do not really help understanding what the object is and does - it's not an "autocomplete" :-) AutocompleteMatcher maybe ?

Yup, I like that name a lot. And renames like this are definitely needed for this patch.

It's a bit sad that we lose all those access denied exceptions in the controller, but I can't think of any good replacement for them. Maybe we could impose that the field widgets pass something like:

'#handler_settings' => array(
  ... regular selection plugin settings ...
  'entity_form' => array(
    'entity_type_id' => ...
    'bundle' => ...
    'field_name' => ...
    'entity_id' => ... (only if we're on a edit form)
  )
)

and then the controller could see if 'entity_form' is set and do all the "field exists" checks.

config schema++

Really amazing work on the patch. This is almost perfect.

It's a bit sad that we lose all those access denied exceptions in the controller, but I can't think of any good replacement for them.

Now we are using selection plugin name and settings in url so perhaps we should add an access method to EntityReferenceSelection plugins. I know we have access checks in buildEntityQuery() methods but this is just a suggestion.
I think we need a new test for fapi element.
Pardon my ignorance, can you please explain what is the difference between entity_reference_autocomplete_tags and entity_reference_autocomplete?
We have to update API change section in IS and BE also need update.
Here is a minor review. I couldn't find anything alarming.

1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/AutocompleteWidget.php
   @@ -144,43 +165,28 @@ protected function getLabels(EntityReferenceFieldItemListInterface $items, $delt
   +      if (($target_bundles = $this->getSelectionHandlerSetting('target_bundles')) && count($target_bundles) == 1) {
   ...
   +      elseif (($bundles = entity_get_bundles($this->getFieldSetting('target_type'))) && count($bundles) == 1) {
   

   Why not reset($bundles) as third option for now until @todo is resolved?

2. +++ b/core/lib/Drupal/Core/Render/Element/EntityAutocomplete.php
   @@ -0,0 +1,170 @@
   + * @FormElement("entity_autocomplete")
   

   I think we need a followup doc issue for fapi doc update.

3. +++ b/core/lib/Drupal/Core/Render/Element/EntityAutocomplete.php
   @@ -0,0 +1,170 @@
   +    // @todo Use an array for the 'autocreate API' below?
   

   I think it would be great if we can combine them in an array so +1 on that.

Have you tried introducing a core.routing.yml file? I would not bet, but I could guess that it works.

1. +++ b/core/config/schema/core.entity.schema.yml
   index c4c7593..e4343f1 100644
   --- a/core/core.services.yml
   
   +++ b/core/core.services.yml
   @@ -349,6 +349,9 @@ services:
   +  entity.autocomplete_matcher:
   +    class: Drupal\system\EntityAutocompleteMatcher
   

   We reference a system code in core.services.yml?

2. +++ b/core/lib/Drupal/Core/Render/Element/EntityAutocomplete.php
   @@ -0,0 +1,170 @@
   +/**
   + * Provides an entity autocomplete form element.
   + *
   + * @FormElement("entity_autocomplete")
   + */
   +class EntityAutocomplete extends Textfield {
   

   @tim and @dawe talked about that and we agreed it should be in Drupal\Core\Entity\Element instead

Cursory review.

1. +++ b/core/config/schema/core.entity.schema.yml
   @@ -222,6 +222,34 @@ field.widget.settings.checkbox:
   +  label: 'Entity reference autocomplete (Tags style) display format settings'
   

   "Tags style" is a bit strange to mention, no? That's highly specific to one well-known example.

   Perhaps something like "autocreate non-existing items" makes more sense?

2. +++ b/core/lib/Drupal/Core/Render/Element/EntityAutocomplete.php
   @@ -0,0 +1,170 @@
   +    // Nothing to do if there is no target entity type.
   +    if (empty($element['#target_type'])) {
   +      return $element;
   +    }
   

   Would throwing an exception be more appropriate?

3. +++ b/core/lib/Drupal/Core/Render/Element/EntityAutocomplete.php
   @@ -0,0 +1,170 @@
   +  protected static function createNewEntity($entity_type_id, $bundle ,$label, $uid) {
   

   Nit: s/ ,$label/, $label/

4. +++ b/core/modules/book/config/install/core.entity_form_display.node.book.default.yml
   @@ -25,7 +25,6 @@ content:
   -      autocomplete_type: tags
   

   This can be removed everywhere because core only supports one autocomplete type?

5. +++ b/core/modules/system/src/Controller/EntityAutocompleteController.php
   @@ -0,0 +1,80 @@
   +   * The autocomplete helper for entity references.
   ...
   +   *   The autocompletion helper for entity references.
   

   s/helper/matcher/

6. +++ b/core/modules/system/src/Controller/EntityAutocompleteController.php
   @@ -0,0 +1,80 @@
   +   *   The matched entity labels as JSON.
   

   as a JSON response

7. +++ b/core/modules/system/src/Controller/EntityAutocompleteController.php
   @@ -0,0 +1,80 @@
   +      // Selection settings are passed in as an encoded serialized array.
   

   Why?

8. +++ b/core/modules/system/src/EntityAutocompleteMatcher.php
   @@ -0,0 +1,91 @@
   + * Helper class to get autocompletion results for entity reference.
   

   Matcher

9. +++ b/core/modules/system/src/EntityAutocompleteMatcher.php
   @@ -0,0 +1,91 @@
   +   * The Entity reference selection handler plugin manager.
   ...
   +   *   The Entity reference selection handler plugin manager.
   

   s/Entity/entity/

10. +++ b/core/modules/system/src/EntityAutocompleteMatcher.php
    @@ -0,0 +1,91 @@
    +   * This function can be used by other modules that wish to pass a mocked
    +   * definition of the field on instance.
    

    "on instance"? I don't know what that means.

11. +++ b/core/modules/system/src/EntityAutocompleteMatcher.php
    @@ -0,0 +1,91 @@
    +   *   A list of matched entity labels.
    

    Could be more descriptive about what it returns. ("An array containing pairs of …".)

Thanks everyone for the interest in this issue :)

Re #26:

Now we are using selection plugin name and settings in url so perhaps we should add an access method to EntityReferenceSelection plugins. I know we have access checks in buildEntityQuery() methods but this is just a suggestion.

Those access checks have to brought back in the controller, not in the selection plugins.

I think we need a new test for fapi element.

Yes, I already said above I'm waiting until architectural reviews are done.

Pardon my ignorance, can you please explain what is the difference between entity_reference_autocomplete_tags and entity_reference_autocomplete?

entity_reference_autocomplete provides one for element for each field value, while entity_reference_autocomplete_tags provides a single element for all values.

Why not reset($bundles) as third option for now until @todo is resolved?

Done.

I think we need a followup doc issue for fapi doc update.

Yup, I just need to figure out how the fapi documentation on d.o works. Feel free to open the issue though :)

I think it would be great if we can combine them in an array so +1 on that.

Done, #autocreate is now NULL by default and it's expected to be an array. Still not sure where to document this...

Re #27:

Have you tried introducing a core.routing.yml file? I would not bet, but I could guess that it works.

I just tried now, it doesn't work.

We reference a system code in core.services.yml?

Fixed.

@tim and @dawe talked about that and we agreed it should be in Drupal\Core\Entity\Element instead

I'm a bit surprised that it works, but it's done :)

Re #28:

1. "Tags style" is a bit strange to mention, no? That's highly specific to one well-known example.

   Perhaps something like "autocreate non-existing items" makes more sense?

   That's just the name of the widget. Also, tagging does not imply "autocreate non-existing items" :)

2. Would throwing an exception be more appropriate?

   Why not, done.

3. Fixed.

4. This can be removed everywhere because core only supports one autocomplete type?

   Nope, that was part of the autocomplete path before, but not anymore.

5. Fixed.
6. Fixed.

7. // Selection settings are passed in as an encoded serialized array.
   Why?

   Because I don't know another way of passing a PHP array as part of the autocomplete path / url :)

8. Fixed.
9. Fixed.

10. "on instance"? I don't know what that means.

    Have you forgotten D7 already? :P Removed that part, it wasn't needed anymore since #2107243: Decouple entity reference selection plugins from field definitions.

11. Could be more descriptive about what it returns. ("An array containing pairs of …".)

    Sure, improved that doc a bit.

Not positive, but I believe this blocks #2416987: Fix UI regression in the menu link form, making this critical as well.

Also this could be backported to https://www.drupal.org/project/references_dialog

Added full test coverage for the form element... I think we should be pretty much done here :)

Interdiff requested by @dawehner on IRC.

Silly me.

Just throwing this out there in case someone is interested: it would also be nice if the element could support a #bundles property, or even a select list before the autocomplete allowing me to narrow down the available options to autocomplete by individual bundles. Also, does this work with UUIDs?

@Dave Reid
Yet another thing to discuss about in #2353611: Make it possible to link to an entity by UUID :)

Note: I did some manual testing and it worked exactly as expected out of the box.

1. +++ b/core/modules/system/src/Controller/EntityAutocompleteController.php
   @@ -0,0 +1,80 @@
   +   * @param Request $request
   

   Nitpick, afaik we use absolute class (so with namespace)

2. +++ b/core/modules/system/system.routing.yml
   @@ -460,3 +460,11 @@ system.admin_content:
   +system.entity_autocomplete:
   +  path: '/entity_reference_autocomplete/{target_type}/{selection_handler}/{selection_settings}'
   +  defaults:
   +    _controller: '\Drupal\system\Controller\EntityAutocompleteController::handleAutocomplete'
   +    selection_settings: ''
   +  requirements:
   +    _access: 'TRUE'
   

   It would be so great to have a core.routing.yml file, just saying, but totally out of scope of this issue.

Fixed the Request namespace and added an issue to the last @todo remaining in the patch: #2418249: Consider improving the DX of #default_value for the entity_autocomplete form element.

@Dave Reid:

Just throwing this out there in case someone is interested: it would also be nice if the element could support a #bundles property, or even a select list before the autocomplete allowing me to narrow down the available options to autocomplete by individual bundles.

That's already supported. You can provide a 'target_bundles' option to the 'default' selection handler:

$form['test-element'] = array(
  '#type' => 'entity_autocomplete',
  '#target_type' => 'node',
  '#selection_handler' => 'default', // Not needed, 'default' is already the default :)
  '#selection_settings' => array(
    'target_bundles' => array(
      'article',
    ),
  ),
);

@dawehner:

It would be so great to have a core.routing.yml file, just saying, but totally out of scope of this issue.

Yep, I tried to do that but it doesn't work. Let's open an issue for it though.

I looked at the interdiff. should still be rtbc.

I know it is blocking menu work but I'd like to wait for @yched review. Can we please hold RTBC till then?

Yeah, I think that's fine. It's only holding up #2418017: Implement autocomplete UI for the link widget and we can review the parts of that patch that are not this one in the meantime.

Notes from my (non-exhaustive) review:

1) We still need a change record. It needs to note that this new field is now available, and a before/after example on how to use it. From http://privatepaste.com/a6b5659a7f:

$form['test-element'] = array(
  '#type' => 'entity_autocomplete',
  '#target_type' => 'node',
  '#selection_handler' => 'default', // Not needed, 'default' is already the default :)
  '#selection_settings' => array(
    'target_bundles' => array(
      'article',
    ),
  ),
);

2) It seems like core/lib/Drupal/Core/Entity/Element/EntityAutocomplete.php should have this type of documentation and currently does not. I guess #1617948: [policy for now] New standard for documenting form/render elements and properties is still outstanding so we haven't yet agreed on the proper way to do this. Maybe ask jhodgdon/timplunkett their thoughts?
3) This patch itself doesn't contain any conversions, which is a little odd because normally we would do at least one. However, given #2418017: Implement autocomplete UI for the link widget does and hopefully will land within a few days, I think that's fine and we can extrapolate from there:

-      $element['uri']['#type'] = 'textfield';
+      $element['uri']['#type'] = 'entity_autocomplete';
+      $element['uri']['#target_type'] = 'node';

So the default DX here looks pretty simple. We had discussed on IRC that in a follow-up we should talk about allowing #target_type to optionally accept an array of valid types.

Making that explicit, though that shouldn't stop anyone else from reviewing too. :)

1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/AutocompleteWidget.php
   @@ -75,30 +95,27 @@ public function settingsSummary() {
   +      // Entiy reference field items are handling validation themselves via
   

   Entity is missing a t

2. +++ b/core/modules/system/src/Tests/Entity/Element/EntityAutocompleteElementFormTest.php
   @@ -0,0 +1,292 @@
   +        'single' => 'single - inexistent label',
   ...
   +    $this->assertEqual($form_state->getErrors()['single'], t('There are no entities matching "%value".', array('%value' => 'single - inexistent label')));
   ...
   +        'single' => 'single - inexistent label (42)',
   ...
   +        'single_no_validate' => 'single - inexistent label',
   +        'single_autocreate_no_validate' => 'single - autocreate inexistent label'
   ...
   +        'single_no_validate' => 'single - inexistent label (42)',
   +        'single_autocreate_no_validate' => 'single - autocreate inexistent label (43)'
   

   inexistent is nonexistent

3. Do we have the requisite test coverage to prove the security improvements?
4. Needs a change record.

Will try to review tonight :-)

I can only find one nit that hasn't already been found. Looking forward to yched's review :)

+++ b/core/lib/Drupal/Core/Entity/Element/EntityAutocomplete.php
@@ -0,0 +1,218 @@
+  public static function processEntityAutocomplete(&$element, FormStateInterface $form_state, &$complete_form) {
...
+  public static function validateEntityAutocomplete(&$element, FormStateInterface $form_state, &$complete_form) {

We could typehint &$element and &$complete_form to array here.

Re #43:

1. Fixed.
2. Fixed.
3. The security improvement is brought by the fact that we're using ER selection handlers for matching entity labels, and they are tested in Drupal\system\Tests\Entity\EntityReferenceSelection\EntityReferenceSelectionAccessTest
4. Added a change record draft: https://www.drupal.org/node/2418529

Fixed #45 too :)

@webchick:

This patch itself doesn't contain any conversions, which is a little odd because normally we would do at least one.

Note that there is actually a conversion in the patch, the ER autocomplete widgets are converted to use this new element otherwise they wouldn't work anymore. See this hunk:

--- a/core/modules/entity_reference/src/Plugin/Field/FieldWidget/AutocompleteWidgetBase.php
+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/AutocompleteWidget.php

Ok I have done some manual testing. This testing was done after applying the patch and a fresh Drupal install.
AutocompleteWidget is working fine.

Then I change the term ER field to AutocompleteTagsWidget and unlimited cardinality
I created a new node with two tags works fine.
I went to edit the field. It was populated correctly

I removed both tags and added new tag

I previewed the node.

I pressed back to content editing page. The field was still populated by new tag.
I clicked view tab on node

I clicked edit tab and saw new tag instead of original tags.

Looking at the last screenshot in #47 just above:
On edit of a field with pre-existing values, shouldn't we populate #default_value with the disambiguating "(entity_id)" at the end ?
That's what ER autocomplete widget currently does in HEAD.

@yched, I think that's a new tag, so an unsaved entity.

@jibran, can you please try the same scenario in HEAD without the patch applied?

Patch is awesome, beautifully documented and commented, @amateescu+++++++++

A couple questions / nitpicks below, none of them are blocking, except maybe the first one.

1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/AutocompleteTagsWidget.php
   @@ -0,0 +1,46 @@
   +class AutocompleteTagsWidget extends AutocompleteWidget {
   
   +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/AutocompleteWidget.php
   @@ -16,9 +16,29 @@
   +class AutocompleteWidget extends WidgetBase {
   

   Yay for the widgets in Core !

   If those move from e_r.module into the large-ish "all field types" bucket in Drupal/Core/Field/Plugin/Field/FieldWidget, we should probably rename the classes to *Entity*Autocomplete, they are not "generic autocomplete" widgets.

   Also - does this mean a couple modules can stop depending on e_r.module ?
   (actually, wondering what's left in e_r.module now :-p)

2. +++ b/core/lib/Drupal/Core/Entity/Element/EntityAutocomplete.php
   @@ -0,0 +1,218 @@
   +    $info['#selection_handler'] = 'default';
   +    $info['#selection_settings'] = array();
   

   #autocreate is now an array of data, would it make sense for '#selection_*' as well ?

3. +++ b/core/lib/Drupal/Core/Entity/Element/EntityAutocomplete.php
   @@ -0,0 +1,218 @@
   +    $info['#validate_reference'] = TRUE;
   

   Wondered about that, and the ER widgets provide a nice explanation, but that is potentially dangerous.

   is there a place we can document the do's and don'ts about that property on the FAPI element ? Like "only set to FALSE if proper validation by the selection handler is performed at another level on the extracted form values" or something.
   Maybe at least here in the Element's getInfo(), if nowhere else ?

   Similarly, maybe reversing the bool and naming it '#skip_reference_validation' would make it sound a bit scarier ?

4. +++ b/core/lib/Drupal/Core/Entity/Element/EntityAutocomplete.php
   @@ -0,0 +1,218 @@
   +      foreach (Tags::explode($element['#value']) as $input) {
   

   Isn't it weird that we Tags::explode() regardless of $element['#tags'] ? What if a non-tag-style autocomplete contains a node title that includes a comma ?

5. +++ b/core/lib/Drupal/Core/Entity/Element/EntityAutocomplete.php
   @@ -0,0 +1,218 @@
   +          // Auto-create item. See
   +          // \Drupal\Core\Field\Plugin\Field\FieldType\EntityReferenceItem::presave().
   

   Is that reference still relevant ? Just wondering - ER fields are now just one use case now, other direct consumers of the new FAPI element will need to handle that 'entity' themselves, right ?

6. +++ b/core/lib/Drupal/Core/Entity/Element/EntityAutocomplete.php
   @@ -0,0 +1,218 @@
   +            'entity' => static::createNewEntity($element['#target_type'], $element['#autocreate']['bundle'], $input, $element['#autocreate']['uid'])
   

   Means you're forced to provide an #autocreate['uid'] even for an autocomplete on an entity type that is not an EntityOwnerInterface ?
   Maybe we can just default to anon if no #autocreate['uid'] is provided ?

7. +++ b/core/lib/Drupal/Core/Entity/Element/EntityAutocomplete.php
   @@ -0,0 +1,218 @@
   +    };
   

   JS leak ? ;-)

8. +++ b/core/modules/book/config/install/core.entity_form_display.node.book.default.yml
   @@ -25,7 +25,6 @@ content:
   -      autocomplete_type: tags
   

   And a lot others : yay, those made little sense on the non-tag widget :-)

9. +++ b/core/modules/system/src/Controller/EntityAutocompleteController.php
   @@ -0,0 +1,80 @@
   +  public function __construct(EntityAutocompleteMatcher $entity_autocomplete_matcher) {
   +    $this->entityAutocompleteMatcher = $entity_autocomplete_matcher;
   

   Uber-nitpick, feel free to ignore :-)
   In the context of the "EntityAutoCompleteController", we could be less verbose on the var and property names, "matcher" would work fine IMO.

10. +++ b/core/modules/system/src/Tests/Entity/EntityAutocompleteTest.php
    @@ -2,25 +2,23 @@
      * @group entity_reference
      */
    -class EntityReferenceAutocompleteTest extends EntityUnitTestBase {
    +class EntityAutocompleteTest extends EntityUnitTestBase {
    

    @group needs updating ?

    Also, not sure what's the split between that test being moved around from entity_ref.module, and the newly added EntityAutocompleteElementFormTest, don't they both test the generic 'entity_autocomplete' element now ?

11. +++ b/core/lib/Drupal/Core/Entity/EntityAutocompleteMatcher.php
    @@ -0,0 +1,89 @@
    +   * @var \Drupal\Core\Entity\EntityReferenceSelection\SelectionPluginManagerInterface
    ...
    +  protected $selectionHandlerManager;
    

    The Return of Uber-nitpick: property name mismatch ? If the interface is SelectionPluginManager, the property could be selectionManager ? Not sure where we stand on naming around selection plugins :-)

@yched, I knew we could count on you to point out every possible nitpick :D

Sorry if this comment looks a bit rushed, the previous version was much longer but d.o decided to not post it..

1. Yup, without 'entity_reference' in the namespace the new class names are not very helpful. I prefixed them with EntityReference since they're still very much about the entity reference field and also because we kept this prefix in their plugin ids.
2. I think I prefer those better as top-level properties, they're more important than the #autocreate stuff.
3. I just followed what Drupal\Core\Render\Element\PathElement is doing with its #validate_path property. But until we can document it properly on api.d.o, I included your comment there.
4. It's what the previous code was doing, and it's not weird because we're doing Tags::encode() on every label in EntityAutocompleteMatcher :)
5. You're right that it's not so relevant anymore, but I thought it would be a nice example for anyone reading the code on how to handle the 'entity' property, so I expanded the comment a bit.
6. You're not forced to provide a uid, we default to the current user in EntityAutocomplete::processEntityAutocomplete().
7. I've no idea how that got there.. wasn't me :D
8. Yeah, I think those were not even used anymore since we switched from #autocomplete_path to #autocomplete_route_name :/
9. Fixed.
10. Updated the @group. The difference is that EntityAutocompleteElementFormTest tests that the properties of the form element are behaving correctly (e.g. #tags, #autocreate) while the one that is moved (EntityAutocompleteTest) tests the autocomplete controller and the matcher service.
11. Fixed.

Whaaaaat, d.o ate my comment :((

Edit: oh well, I wrote it again.

Thanks for the replies @amateescu.

Works for me, pending the possible issue in #48 / #49 (sorry, can't test myself, my local install seems broken atm)

can you please try the same scenario in HEAD without the patch applied?

This is the bug in HEAD :(. I'll try to create a test-only patch for this bug in a follow up bug report.
On edit of a field with pre-existing values, shouldn't we populate #default_value with the disambiguating "(entity_id)" at the end ?
I think that's a new tag, so an unsaved entity.That is correct.

I think we need a followup doc issue for fapi doc update.
Yup, I just need to figure out how the fapi documentation on d.o works. Feel free to open the issue though :)

We still need a follow up for this .
Thank you @webchick for holding the RTBC. Thank you @yched for the review. And thank you @amateescu for awesome work on the patch. I have created an issue #2411981: Fixes after generic 'entity_autocomplete' Form API element for DER to cope with these core changes please help us find the correct solution there. Thank you all for reviews.
RTBC +1.

On edit of a field with pre-existing values, shouldn't we populate #default_value with the disambiguating "(entity_id)" at the end ?
I think that's a new tag, so an unsaved entity. That is correct

@jibran :
Not sure we're clear here.
The referenced entity was "new" when you typed its name in the ER widget in the containing node and submitted the form. Then it was created and saved during the node save.
Next time you edit the containing node, the referenced entity is not "new" anymore, it's a regular case of "existing node references existing entity", and it should appear as "Title (id)" in the widget #default_value.

That's what happens in HEAD currently (tested it yesterday night), but the last screenshot in your #47 seems to indicate it's not the case with the patch ?
(again, sorry, can't test it myself right now, I will tonight)

Yeah that is bug.
Steps to reproduce:

1. Create a node with new tags. (ER auto tag widget)
2. Edit the node. (This shows correct default values)
3. Replace the default values of ER field with new tag.
4. Preview the node.
5. Come back to edit form
6. Now default value is new tag with no id( which is correct)
7. View the node. See I didn't save the node.
8. I can see the tags added in 1.
9. Edit the node.
10. I should see 2. Instead I see 6.(This is the bug.)

Thanks, it's much easier to follow the steps presented like this without the distracting images. I can confirm the bug in HEAD, so it's not related to this patch.

It also happens with the taxonomy reference field in HEAD, so this is actually a general problem with the node preview functionality. I'm guessing that we're not clearing whatever is in temp store when we come back from preview to the edit form.

I found one more problem: this autocomplete unfortunately autocompletes even nodes that are not accessible by the current user. This will violate the information disclosure policy we currently have for the link widget in HEAD, where we carefully made sure to not ever expose whether a path exists or not, and definitely never tell the user that a certain entity exists, let alone disclose its title.

Perhaps that's a pre-existing problem in ER autocomplete in HEAD, but I think it's worth un-RTBC'ing this for — I find that very sad too btw :( — because we should at least have a discussion about this.

Meant to un-RTBC.

And looked at the patch:

1. +++ /dev/null
   @@ -1,112 +0,0 @@
   -    if ($entity_id !== 'NULL') {
   -      $entity = $this->entityManager->getStorage($entity_type)->load($entity_id);
   -      if (!$entity || !$entity->access('view')) {
   -        throw new AccessDeniedHttpException();
   -      }
   -    }
   

   This is removed, but not added. So it's a newly introduced bug. And it means we have no test coverage for it.

2. +++ b/core/modules/system/src/Controller/EntityAutocompleteController.php
   @@ -0,0 +1,80 @@
   +  public function handleAutocomplete(Request $request, $target_type, $selection_handler, $selection_settings = '') {
   +    $matches = array();
   +    // Get the typed string from the URL, if it exists.
   +    if ($input = $request->query->get('q')) {
   +      $typed_string = Tags::explode($input);
   +      $typed_string = Unicode::strtolower(array_pop($typed_string));
   +
   +      // Selection settings are passed in as an encoded serialized array.
   +      $selection_settings = $selection_settings ? unserialize(base64_decode($selection_settings)) : array();
   +
   +      $matches = $this->matcher->getMatches($target_type, $selection_handler, $selection_settings, $typed_string);
   +    }
   +
   +    return new JsonResponse($matches);
   +  }
   

   No such access checking in here.

I signaled the same thing in #23 and I agree it needs to be solved somehow.

But are you sure about autocompleting nodes that are not accessible to the current user? Selection handlers should already take care of that.

I can take a look in a few hours :)

But are you sure about autocompleting nodes that are not accessible to the current user? Selection handlers should already take care of that.

Absolutely certain, I'm afraid.

I created article node 1. Then created user 2 who couldn't access nodes (removed the access content permission from the "authenticated user" role). Verified by doing var_dump(Node::load(1)->access('view')), which printed TRUE for user 1, FALSE for user 2. Yet user 2 did get it in the autocomplete. Also after a drush cr.

I can take a look in a few hours :)

Yay! :)

I looked into it and the problem found in #58 has nothing to do with #59/#23, so I opened a separate issue: #2419923: Port SA-CONTRIB-2013-096 to D8

Alright, that addresses my concern. Thank you! Back to RTBC! :)

Actually, more than that: this patch (#51) is included in the patch at #2418017: Implement autocomplete UI for the link widget (because that issue is blocked on this one), and it's been working splendidly. Zero complaints.

Rerolled.

I already looked at this the other day and it looks like it only got more awesome since then. :) Change record looks good.

Committed and pushed to 8.0.x. Thanks!

Congrats @amateescu !

We still need a followup for #56 / #57 ?

See #2421263: Potential data loss: concurrent (i.e. by different users) node edits leak through preview for #56.

@jibran, I don't see how that issue is related. The only similarity is that both bugs are related to the node preview functionality.

I opened #2421581: Node preview causes stale values to be kept indefinitely for exisiting nodes to fix #56.

Thanks @amateescu.

Started working on converting stuff to this new form element, the first issue is here: #2428881: Remove TermAutocompleteController::autocompletePerVid()

Next one: #2428941: Update the Node views wizard to use 'entity_autocomplete' for the "tagged_with" field

And the last: #2434697: Remove UserAutocompleteController

Edit: This one also fixes #50.4. The fact that core was doing it that way doesn't mean that we shouldn't improve it :)