Problem/Motivation

Now that we have a generic 'entity_autocomplete' form element, UserAutocompleteController is just duplicated functionality.

Proposed resolution

Convert its usage to 'entity_autocomplete' and remove all the dead code.

Remaining tasks

None.

User interface changes

User entity reference fields will have new checkbox "Include the anonymous user." in field configuration form.

API changes

A new 'include_anonymous' option is added to the user reference selection plugin.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Prioritized changes Follow-up for a critical issue #1959806: Provide a generic 'entity_autocomplete' Form API element.
CommentFileSizeAuthor
#13 2434697-13.patch34.29 KBamateescu
#8 interdiff.txt423 bytesamateescu
#8 2434697-8.patch34.31 KBamateescu
#5 interdiff.txt4.08 KBamateescu
#5 2434697-5.patch34.72 KBamateescu
#3 2434697-3.patch36.96 KBpcambra
#3 interdiff-2434697-1-3.txt2.48 KBpcambra
#1 2434697.patch35.7 KBamateescu

Comments

amateescu’s picture

amateescu commented
Status: Active » Needs review
StatusFileSize
new 2434697.patch35.7 KB

17 files changed, 115 insertions(+), 380 deletions(-)

Status: Needs review » Needs work

The last submitted patch, 1: 2434697.patch, failed testing.

pcambra’s picture

pcambra
he/him
Primary language Spanish
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-2434697-1-3.txt2.48 KB
new 2434697-3.patch36.96 KB

I think the form was making wrong assumptions about the current user being the one checked.

Status: Needs review » Needs work

The last submitted patch, 3: 2434697-3.patch, failed testing.

amateescu’s picture

amateescu commented
Status: Needs work » Needs review
StatusFileSize
new 2434697-5.patch34.72 KB
new interdiff.txt4.08 KB

Thanks @pcambra, your patch finally made me realize that the comment behavior is so custom that there's no point in trying to use EntityAutocomplete::validateEntityAutocomplete() at all, so I removed that weird new #process_autocomplete_value stuff in favor of just overriding #element_validate in CommentForm.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Another lovely diffstat! :)

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

This is perfect. Just two nits. Also updated user interface changes in IS.

  1. +++ b/core/modules/comment/src/Tests/CommentInterfaceTest.php
@@ -196,4 +196,5 @@ function testCommentInterface() {
+

    Out of the scope. :) 

  2. +++ b/core/modules/system/src/Tests/Entity/EntityReferenceSelection/EntityReferenceSelectionAccessTest.php
@@ -203,6 +203,7 @@ public function testUserHandler() {
+        'include_anonymous' => TRUE,

    I believe we don't have to add that.

amateescu’s picture

amateescu commented
StatusFileSize
new 2434697-8.patch34.31 KB
new interdiff.txt423 bytes

Thanks for reviewing :)

Fixed #7.1 and #7.2 is correct, we don't have to but I added it so it's a bit more clear what we're testing.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented

Thank you @amateescu for the patch this is a huge improvement and thank you for clarifying #7.2.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work 
  1. +++ b/core/modules/content_translation/src/ContentTranslationHandler.php
@@ -390,19 +390,21 @@ public function entityFormAlter(array &$form, FormStateInterface $form_state, En
-      $form['content_translation']['name'] = array(
-        '#type' => 'textfield',
+      $form['content_translation']['uid'] = array(

+++ b/core/modules/content_translation/src/Tests/ContentTranslationUITest.php
@@ -219,7 +219,7 @@ protected function doTestAuthoringInfo() {
-        'content_translation[name]' => $user->getUsername(),

@@ -237,7 +237,7 @@ protected function doTestAuthoringInfo() {
-      'content_translation[name]' => $this->randomMachineName(12),
+      'content_translation[uid]' => $this->randomMachineName(12),

    How come we're changing the the form element from name to uid? Whereas we're not in CommentForm. I'm not sure that changing it is correct. 

  2. +++ b/core/modules/entity_reference/config/schema/entity_reference.schema.yml
@@ -36,3 +36,6 @@ entity_reference.default.handler_settings:
+    include_anonymous:
+      type: boolean
+      label: 'Include the anonymous user in the matched entities.'

    But this is meaning less for anything other than selecting users. Feels like this is blocked on #2436835: Unable to create config schema for entity type specific entity reference selection plugin.

amateescu’s picture

amateescu commented
Status: Needs work » Reviewed & tested by the community

Re #10:

1. The change is correct because the entity autocomplete form element, by default through EntityAutocomplete::validateEntityAutocomplete(), returns entity IDs. CommentForm does not use the default behavior of the element, that's why it is not changed.
2. The user selection handler already has the 'filter' custom setting that's only used for user entities, which means that #2436835: Unable to create config schema for entity type specific entity reference selection plugin. will have to take the new one into account in a reroll.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 8: 2434697-8.patch, failed testing.

amateescu’s picture

amateescu commented
Status: Needs work » Reviewed & tested by the community
StatusFileSize
new 2434697-13.patch34.29 KB

Rerolled.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Yep let's handle #10.2 in the other issue.

Committed 3e2af23 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

  • alexpott committed 3e2af23 on 8.0.x 
    Issue #2434697 by amateescu, pcambra: Remove UserAutocompleteController
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.