EntityAutocomplete form element has no docs on how to use it

The patch looks great! Just a small problem that needs to be fixed:

+++ b/core/lib/Drupal/Core/Entity/Element/EntityAutocomplete.php
@@ -14,8 +14,61 @@
+ * $form['my_element'] = array(
...
+ * $form['my_element'] = array(
...
+ *    'target_bundles' => array('article', 'page'),
...
+ * $form['my_element'] = array(
...
+ * $form['my_element'] = array(
...
+ *  '#autocreate' => array(

We should use the short array syntax.

Please find attached the corrected patch file

Much better, thanks!

1. +++ b/core/lib/Drupal/Core/Entity/Element/EntityAutocomplete.php
   @@ -14,8 +14,61 @@
   + * If you want to restrict the matches to a single or a set of bundles, you can
   ...
   + * If we want to allow an input of multiple entity labels into the element
   ...
   + * If we want to allow an input of an entity label that does not exist yet but
   

   "you" or "we"? We should be consistent, I think.

2. +++ b/core/lib/Drupal/Core/Entity/Element/EntityAutocomplete.php
   @@ -14,8 +14,61 @@
   + * $form['my_element'] = [
   + *  '#type' => 'entity_autocomplete',
   + *  '#target_type' => 'taxonomy_term',
   + *  '#autocreate' => [
   + *   'bundle' => 'tags',
   + *   'uid' => <a valid user ID>,
   + *  ],
   + * ];
   

   Indentation in all these sample code blocks needs another look.

Examples has an... example. :-) http://cgit.drupalcode.org/examples/tree/ajax_example/src/Form/EntityAut...

And you can learn how to test it: http://cgit.drupalcode.org/examples/tree/ajax_example/tests/src/Function...

Cleaned up some formatting and added some properties

Remember to branch / start over from 8.7.x before working on the next patch :-)

Doh! Should be good now

i triggered #13 retesting and it went green
so adjusting status :)

Both my comments from #7 still apply to the latest patch as well, the indentation across all code samples needs another look.

Please find updated interdiff patch file.

Updated interdiff

Update interdiff and patch

@virajrajankar, it looks like you've uploaded the interdiff as the patch as well. You should create a patch of all the differences from 8.7.x as well as an interdiff which should be the changes between comment #13 and your new patch.

Thanks for ur update @mradcliffe, Here i have fixed the issue from comment #7 and #16. Please find the patch to test.

Thank you, @dhirendra.mishra. You should also change the issue status to Needs review when uploading a patch.

It is also very helpful to create an interdiff between the patch you posted in #25 and patch #13.

+ * Properties:
+ * - #target_type: The entity type of the entity/entities being selected.
+ * - #validate_reference: Indicates whether the referenced entity should be 
+ * validated.
+ *   Defaults to TRUE. This should only be set to FALSE if proper validation by 
+ * the
+ *   selection handler is performed at another level on the extracted form 
+ * values.
+ * - #process_default_value: Indicates the field might need some processing in 
+ * order to
+ *   be a valid runtime value for the field type. Defaults to TRUE. This should 
+ * only
+ *   be set to FALSE if the #default_value property is processed at another 
+ * level
+ *   (e.g. by a Field API widget) and its value is properly checked for access.
  *

I reviewed the patch in #25 and I do not think this is what @joachim means with regard to "long lines need wrapping". If a new line needs to be added, then the line following it should be modified to make sense.

For instance, starting with the line added "validated". This should be indented so that it lines up with the # on the previous line. And then the following sentence "Defaults to TRUE" needs to be brought up onto the same line. This means that the entire section may need to be redone instead of only adding new lines in order to improve readability.

Here is the updated patch which addresses comment #7 and other issues.

#27 patch is looking good to me

@joachim I have added your point 2 but I dont understand the point 1. It would be repeat mostly . Please check it and guide me if you need any changes.

@joachim, @kalyansamanta
I corrected point no 1. as per @joachim added. Please have a look once. I tested and Applied patch core/lib/Drupal/Core/Entity/Element/EntityAutocomplete.php cleanly on the local machine with Drupal 8.7.x.

Hi @joachim,
I have updated the patch. Please have a look and let me know if there are any issues.

Thanks, @Ramya Balasubramanian

+++ b/core/lib/Drupal/Core/Entity/Element/EntityAutocomplete.php
@@ -14,9 +14,77 @@
+ * which can target all or specific bundles of an entity.
+ * ¶
+ * Properties:
...
+ * @endcode
+ * ¶
+ * If you want to restrict the matches to a single or a set of bundles,
...
+ * @endcode
+ * ¶
+ * If you want to allow an input of multiple entity labels into the element
...
+ * @endcode
+ * ¶
+ * If you want to allow an input of an entity label that does not exist yet
...
+ * @endcode
+ * ¶

5 unexpected whitespaces at the end of each line

Thanks @jungle. Will update the patch soon.

Hi @jungle,
I have removed whitespaces and updated the patch. Please have a look and let me know if there are any issues

Thanks, @Ramya Balasubramanian! Assigning to myself to do adjustment later

Hey @jungle,
Could you please tell me what kind of adjustments we need to make here ?

1. +++ b/core/lib/Drupal/Core/Entity/Element/EntityAutocomplete.php
   @@ -14,8 +14,76 @@
   + * Entity autocomplete element allow you to create an entity reference field
   

   allow you to allows you

2. +++ b/core/lib/Drupal/Core/Entity/Element/EntityAutocomplete.php
   @@ -14,8 +14,76 @@
   + * - #selection_handler: The plugin ID of the entity reference selection
   + * handler.
   + * - #selection_settings: An array of settings that will be passed to the
   + * selection handler.
   

   The indention in bullets should be adjusted.

3. +++ b/core/lib/Drupal/Core/Entity/Element/EntityAutocomplete.php
   @@ -14,8 +14,76 @@
   + *  '#default_value' => $entity,
   

   I'd change $entity to $node

4. +++ b/core/lib/Drupal/Core/Entity/Element/EntityAutocomplete.php
   @@ -14,8 +14,76 @@
   + * If you want to allow an input of multiple entity labels into the element
   + * you set the '#tags' property to TRUE, default Value is FALSE:
   

   Wrapped too early.

5. +++ b/core/lib/Drupal/Core/Entity/Element/EntityAutocomplete.php
   @@ -14,8 +14,76 @@
   + * // #autocreate should be an array where the 'bundle' key is required
   + * // should be the bundle name for the new entity.
   + * // uid is optional and defaults to current logged-in user.
   

   Comment here should be moved out @code

Well, I meant to finish it myself. Interrupted in the middle. A quick review and an incomplete patch, FYI, interdiff is ignored.

Since the above patch is failed, I am assigning this ticket to myself for the changes mentioned by jungle.

Hi @jungle,
I have added a patch, please have a look and let me know if there are any issues.

Looks better, thank you again, @Ramya Balasubramanian!

+++ b/core/lib/Drupal/Core/Entity/Element/EntityAutocomplete.php
@@ -14,8 +14,76 @@
+ * can be create it "on the fly" with the '#autocreate' property, it allows two

can be create to can create?

I am not a native English speaker, so I'd leave this to others to do further review.

(I guess you forgot to unassign yourself, doing for you)

Hi @jungle,
As per your above comment, I have fixed that and added a patch. Please have a look and let me know

Thanks, @Ramya Balasubramanian!

1. +++ b/core/lib/Drupal/Core/Entity/Element/EntityAutocomplete.php
   @@ -14,8 +14,76 @@
   + * - #tags: To allow an input of multiple entity labels into the element
   + *   you set the '#tags' property to TRUE.
   

   Wrapped too early, could be ... the element you

2. +++ b/core/lib/Drupal/Core/Entity/Element/EntityAutocomplete.php
   @@ -14,8 +14,76 @@
   + * - #autocreate: It should be an array where the 'bundle' key is required
   + *   and should be the bundle name for the new entity.
   

   the same. wrapped too early.

3. +++ b/core/lib/Drupal/Core/Entity/Element/EntityAutocomplete.php
   @@ -14,8 +14,76 @@
   + * - #process_default_value: Indicates the field might need some processing
   + *   in order to be a valid runtime value for the field type.
   

   And here, wrapped too early.

4. +++ b/core/lib/Drupal/Core/Entity/Element/EntityAutocomplete.php
   @@ -14,8 +14,76 @@
   + * If you want to restrict the matches to a single or a set of bundles,
   + * you can use the 'target_bundles' selection setting:
   

   And here too, wrapped too early.

Meanwhile, I will send this to the #documentation group on slack to see if anyone could do an extra review.

So would be better to wait for feedback from others before making the new patch to save your time.

Thanks for the patch! Here are some more review comments:

a) Grammar:

+ * Entity autocomplete element allows you to create an entity reference field
+ * which can target all or specific bundles of an entity.

- Needs to start with "An" or "The".
- Before "which" you need a comma. Or better yet, change "which" to "that".

b) But actually this sentence is not factually correct. This is an entity autocomplete. It is NOT an entity reference field. They are not the same thing at all. This is an element, not a field.

c)

+ * - #tags: To allow an input of multiple entity labels into the element
+ *   you set the '#tags' property to TRUE.

Please rewrite this in a style more like other documentation. Such as: TRUE if the element allows multiple entity selection; FALSE if it allows only one entity to be selected.

d)

+ * - #autocreate: It should be an array where the 'bundle' key is required
+ *   and should be the bundle name for the new entity.

Again, please rewrite. We don't want to see words like "should". Document what it is. This documentation doesn't tell me much -- I don't really understand it. Don't start with "It". Take a look at other documentation in Drupal core and follow that style.

e)

+ * - #validate_reference: Indicates whether the referenced entity should be
+ *   validated. Defaults to TRUE.
+ * - #process_default_value: Indicates the field might need some processing
+ *   in order to be a valid runtime value for the field type.

Don't use "indicates". These are values that the user is passing in, not return values. Better would be things like "TRUE if the referenced entity should be validated" etc.

f) There seems to be some confusion in the documentation between the "target" and "referenced" entity. Are these two different things? If so, explain what the terms mean. If not, don't use two different terms in the documentation.

g) It is not normal in API documentation like this to provide 4 usage examples. These would be better to put in a documentation page on drupal.org instead and link to it, or better yet combine into 1 usage example that illustrates the most common usage.

Hi @jhodgdon,
I have addressed all your comments except point no 'G'. I have checked in core and found that 'Usage Examples' were added in around 41 files like
Button.php, Checkboxes.php, Actions.php, Container.php(core/lib/Drupal/Core/Render/Element). Updated the patch and please let me know if there are any other issues. Thanks.,

Thanks! This is getting much better! Some further comments:

a)

 /**
- * Provides an entity autocomplete form element.
+ * This provides an entity autocomplete form element.

This change is wrong. The first line of docs should start with a verb, same as the other elements and other classes. We have a standards page actually:
https://www.drupal.org/docs/develop/standards/api-documentation-and-comm...

b)

+ * The entity autocomplete allows you to create an element of type
+ * entity_autocomplete that can target all or specific bundles of an entity.

Again, what is "target"? This does not seem to tell me much. Maybe a better wording would be:

The autocomplete form element allows the user to select an entity, which can come from all or specific bundles of an entity type.

c)

+ * - #default_value: The default value can be either an entity object or an
+ *   array of entity objects.

Maybe it would be useful to say when it can be an array? You could change the order of the listed properties, so that #tags comes before #default_value, and it would make more sense. Also since #process_default_value is related to this property, maybe put that near #default_value too?

d)

+ * - #tags: TRUE if the element allows multiple selection. False if it allows
+ *   only one entity to be selected.

FALSE should be all caps.

e)

+ * - #autocreate: This #autocreate property can be used when the input of the
+ *   entity label does not exist yet but can be created "on the fly" on form
+ *   submission.

This could use some rewording, -- it shouldn't start with "This #autocreate property". Also the documentation here doesn't tell me what type of value this is... is it TRUE/FALSE? A function name? I really have no idea how to use it from the documentation here.

f) No element in Core has more than 1 usage example. So please make 1 small usage example, and instead of needing 4, put more complete documentation on the properties that you are attempting to instead document through usage examples. For example:
- Each property should document its own default value.
- The selection_handler property should document what type of plugin it is (it says it is the plugin ID, but what type of plugin is it? Without that information I don't know how to find the ones available in Core or how to define my own). Also can it be left blank, or what is the default?
- The selection_settings property should document the settings available in the default selection handler, such as how to select bundles.
- The information you put in the last example should go with the autocreate property docs instead.

Thanks @jhodgdon, I will address your comments and will update the patch soon

Hi @jhodgdon,
I have combined all the 4 usage examples to Single usage example and above that, I have added some brief idea about all properties of entity autocomplete. I am not very clear about the #autocreate property comment but changed the sentence now. Please have a look.

Looks way better, thanks @Ramya Balasubramanian.

+ *  '#default_value' => $node,

Instead of $node, can you specify <entity object>?

Also, as per #64, would be good to have in parentheses the default value of each of those properties when they aren't specified. I believe most of these can easily be found in https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Entity%21...

Hi @jcnventura,
Updated the patch now. Please have a look.

Please make interdiff files when you upload a new patch. Thanks!

The patch is still getting better... but I feel like some of my previous review comments were not addressed... A few notes:

a)

+ * - #default_value: The default value can be either an entity object or an
+ *   array of entity objects.

Again, it would be good here to say explicitly when it is an object and when it is an array.

b)

+ * - #process_default_value: This property shall be set to FALSE if the
+ *   #default_value property is processed at another level.

I do not think we have a single other instance of "shall be" in the Drupal API documentation. Approach documentation as telling the developer trying to use this thing what they need to do. So maybe something like "Set to TRUE if the #default_value entity should be processed, and FALSE if it does not need to be processed." ... but it probably needs more explanation of what "processed" would mean? I have no idea what it means really.

c) In #66 it was asked to put default values for all properties in; I am not seeing that. Example would be something like:

- #property_name: Documentation for the property (defaults to FALSE).

d) In a previous review I asked:

+ * - #selection_handler: The plugin ID of the entity reference selection
+ *   handler.

please say what type of plugin this is (the name of the class or something so that someone could find it)

e) Also asked in a previous review for the selection handler options to be documented and this is not done.

f)

+ * - #autocreate: The entity label can be created on the form submission which
+ *   allows two keys only when the input of the entity label does not exist.

This is not clear, not good grammar, and I still have no idea how to use it.

g)

+ * Usage example:
+ * To provide a basic autocomplete element that matches node titles from all
+ * bundles:

If we really need the "To provide" thing, it should be on the same line as Usage example: and probably should not start with a capital letter. The docs are also not matching the code, which specifies two specific bundles, not "all bundles":

+ *  '#selection_settings' => [
+ *    'target_bundles' => ['article', 'page'],
+ *   ],

Also, I noticed you said "that matches node titles" -- how do we know that will happen? Is that a property? It is not documented anywhere that matching happens on the titles.

Hi @jhodgon,

Thanks for your comments. Updated the patch and interdiff also.
1) I don't have idea about point no: a.
2) Then regarding

 *  
  '#selection_settings' => [
+ *    'target_bundles' => ['article', 'page'],
+ *   ],

The autocomplete form element allows the user to select an entity which can come from all or specific bundles of an entity type.

Thanks for the new patch! But it is quite a mess. Can you please make another pass, and make sure about grammar, punctuation, spaces around punctuation, and things like that?

And as a note, I assume the default value is an array if multiple selection is allowed, and a single entity if only 1 selection is allowed. So I would be sure to put the #tags property before #default_value so this makes sense when you explain it.

Another note: Please put the default values at the end, as we do in other documentation in Drupal, and the sentence should say "Defaults to" not "Default to".

Hi @jhodgdon,
I have added a new patch again and nows #tags are above the #default value. And changed as 'Defaults to'. Please have a look.

I think at this point I should make a new patch rather than trying to point out all the grammar/wording issues I am seeing. I'll do that a little later today.

Here is a new patch -- making a new patch with suggested corrections was easier than trying again to explain the corrections I would suggest.

As a note, I read carefully through the code for this class and several other related classes (the plugin manager etc.) and made sure the information provided here was accurate... I think it is. Please review!

1. +++ b/core/lib/Drupal/Core/Entity/Element/EntityAutocomplete.php
   @@ -14,8 +14,55 @@
   + * The autocomplete form element allows users to select an entity,
   

   'an entity' -> 'one or multiple entities'

   Because depending on the value of #tags, multiple entities can be selected.

2. +++ b/core/lib/Drupal/Core/Entity/Element/EntityAutocomplete.php
   @@ -14,8 +14,55 @@
   + * - #default_value: (optional) The default entity or array of entities,
   

   'or array' -> 'or an array'?

3. +++ b/core/lib/Drupal/Core/Entity/Element/EntityAutocomplete.php
   @@ -14,8 +14,55 @@
   + * - #process_default_value: (optional) Set to FALSE if the #default_value
   

   #process_default_value is missing its default value (:D) at the end of its description.

4. +++ b/core/lib/Drupal/Core/Entity/Element/EntityAutocomplete.php
   @@ -14,8 +14,55 @@
   + * - #validate_reference: (optional) Set to FALSE if validation of the selected
   

   #validate_reference as well as #process_default_value should be placed at the bottom of the keys, below #autocreate, because they're not as important as the other ones, and they should be somehow marked as "advanced" options that should be used with a lot of care.

#76.2 IMHO, no, because it is a way of shortening "the default [...] array of entities". If we are to lengthen it it would be to "the default entity or an array of default entities".

Addressed comments from #76 and #77.

+++ b/core/lib/Drupal/Core/Entity/Element/EntityAutocomplete.php
@@ -14,8 +14,57 @@
+ * - #process_default_value: (optional) Set to FALSE if the #default_value.
+ *   Defaults to TRUE.
+ *   property is processed and access checked elsewhere (such as by a Field API
+ *   widget). Advanced option. USE WITH CARE

These are incomplete sentences. "Defaults to TRUE" needs moving to the end.

Not sure we need the "Advanced option. USE WITH CARE" parts either, as it doesn't explain why it is advanced nor what care should be taken. As far as I know we don't do anything like this elsewhere in core.

Apologies. added it in a hurry.

Removed 'Advanced option. USE WITH CARE'

Looks great to me, thanks for fixing this up.

+++ b/core/lib/Drupal/Core/Entity/Element/EntityAutocomplete.php
@@ -14,8 +14,55 @@
+ * @see \Drupal\Core\Entity\Element

The does not exist. I wonder what this is supposed to point to. I'm not sure. It was added in #73. I'm not sure linking to \Drupal\Core\Render\Element makes much sense. Should we remove it?

Sorry, missed that in my reviews. It seems it was added in the patch in #73 and I have no idea why or what it was supposed to link to.

I think it can be removed.

Addressed #83

Thanks!

Committed/pushed to 9.1.x and cherry-picked back to 8.9.x, thanks!