ViewsSelection::validateAutocompleteInput is not implemented

Note that the code is identical to SelectionBase, but ViewsSelection currently doesn't extend from that (which makes sense, because it's not actually a base class).

We could add a real base class or a trait for this as well... And we need tests.

+++ b/core/modules/entity_reference/src/Tests/EntityReferenceAdminTest.php
@@ -248,21 +249,39 @@ public function testFieldAdminHandler() {
+    $edit = array(
+      'title[0][value]' => 'Test',
+      'field_test_entity_ref_field[0][target_id]' => $node->getTitle()
+    );
+    $this->drupalPostForm('node/add/' . $this->type, $edit, t('Save'));
+    $this->assertLink($node->getTitle());

Wondering if we should also test the more advanced cases of that method, for example when there are two nodes with the same title (as then this can't work anymore).

It's a trait now, so we could say it's covered elsewhere but it probably doesn't hurt to have that covered here as well? The test shouldn't assume that we're using the same code..

+++ b/core/modules/views/src/Plugin/EntityReferenceSelection/ViewsSelection.php
@@ -263,7 +266,7 @@ public function validateReferenceableEntities(array $ids) {
    */
   public function validateAutocompleteInput($input, &$element, FormStateInterface $form_state, $form, $strict = TRUE) {
-    return NULL;
+    return $this->validateAutocomplete($input, $element, $form_state, $form, $strict);
   }

Can't we just directly implement the public method in the interface? Then you don't need duplicated documentation there.

You can't implement the interface there, but you can say something like Implements \Drupal\.... in the docblock.. I think that's fine for such cases?

Just some small points ...

1. +++ b/core/lib/Drupal/Core/Entity/Plugin/EntityReferenceSelection/SelectionValidateTrait.php
   @@ -0,0 +1,56 @@
   +  /**
   +   * Implements \Drupal\Core\Entity\EntityReferenceSelection\SelectionInterface::validateAutocompleteInput().
   +   */
   

   Use {@inheritdoc} instead

2. +++ b/core/modules/entity_reference/src/Tests/EntityReferenceAdminTest.php
   index 7b4d424..47a946c 100644
   --- a/core/modules/views/src/Plugin/EntityReferenceSelection/ViewsSelection.php
   
   --- a/core/modules/views/src/Plugin/EntityReferenceSelection/ViewsSelection.php
   +++ b/core/modules/views/src/Plugin/EntityReferenceSelection/ViewsSelection.php
   

   Should we introduce some dedicated test coverage for the views selection plugin?

1. is @inheritdoc really allowed in a trait that doesn't extend from the interface? That's impossible to resolve, thats why I suggested an explicit reference.

2. dedicated as in how exactly? The tests that we are adding are views selection specific, they extend the tests that @mbovan wrote for #2482625: Views entity reference selection with autocomplete widget broken, which are creating a entity reference view and using it for the field we are testing.

Yeah, we cannot use @inheritdoc because the trait doesn't technically implement the interface, so PhpStorm, other IDEs, and/or API module cannot figure that out.

So using the old "Implements..." as in this patch is fine.

Another possibility would be to do something like this:

/**
 * First line description from interface method goes here.
 *
 * See \full\name\of\interface::methodName() for documentation.
 */
function methodName() {
...

I think either way is fine, but @inheritdoc, not so much.

Assigning to @amateescu for his review :)

I would prefer adding a real base class rather than a trait with just one method :)

And we should rename Drupal/Core/Entity/Plugin/EntityReferenceSelection/SelectionBase to Drupal/Core/Entity/Plugin/EntityReferenceSelection/DefaultSelection, but if the api change is deemed not acceptable at this point, we'll have to try and find a good name for the new base class.

But if everyone else thinks the trait is fine, I'm ok with the current patch as well (i.e. feel free to rtbc :P).

Asked @alexpott but I don't think renaming that is still an option.

However, what if we just extend from SelectionBase? We can override anything we don't need and extending from another plugin doesn't prevent us from being exposed as a completely different plugin in anyway. And there are actually quite a few methods that are identical...

@mbovan, care to try that?

re #13 Yeah renaming would need to be justified by something critical. Perhaps we can solve the naming issues with food documentation on both classes?

How about this? I can't think of a better way of saying that the views selection plugin is not based on entity query :)

Discussed with @amateescu, he's ok in general with extending like that. Patch looks good to me now.

The issue summary could do with an update. Patch looks good to me.

I'd like to see what @dawehner thinks about this patch.

I think the issue summary covers what this is doing.

Also not sure there's anything that @dawehner needs to look at. If I find the time, great, but there's nothing complicated going on here.

RTBC +1

This issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes. Committed a1b24c2 and pushed to 8.0.x. Thanks!