Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
The method validateAutocompleteInput()
is not implemented in ViewsSelection
class which prevents entity reference fields to be validated.
Proposed resolution
Extend SelectionBase
and use validateAutocompleteInput()
from it. Remove the methods that have the same code as parent class. Provide tests.
Remaining tasks
User interface changes
API changes
Comment | File | Size | Author |
---|---|---|---|
#19 | interdiff.txt | 777 bytes | amateescu |
#19 | 2482705-19.patch | 8.36 KB | amateescu |
#15 | views_selection_validate_autocomplete-2482705-15.patch | 7.94 KB | mbovan |
#15 | views_selection_validate_autocomplete-2482705-15-interdiff.txt | 1008 bytes | mbovan |
#15 | views_selection_validate_autocomplete-2482705-15-TEST-ONLY.patch | 3.33 KB | mbovan |
Comments
Comment #1
mbovan CreditAttribution: mbovan at MD Systems GmbH commentedHere is the patch with the same implementation for the
validateAutocompleteInput()
as inSelectionBase
class.Comment #2
BerdirNote 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.
Comment #3
mbovan CreditAttribution: mbovan at MD Systems GmbH commentedSince #2482625: Views entity reference selection with autocomplete widget broken is committed we can continue with this issue.
Added SelectionValidateTrait and now we call
validateAutocomplete()
in both SelectionBase and ViewsSelection classes.Providing test only patch too...
Comment #4
BerdirWondering 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..
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?
Comment #6
mbovan CreditAttribution: mbovan at MD Systems GmbH commentedImplemented suggestions from #4.
Comment #8
dawehnerJust some small points ...
Use
{@inheritdoc}
insteadShould we introduce some dedicated test coverage for the views selection plugin?
Comment #9
Berdir1. 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.
Comment #10
jhodgdonYeah, 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:
I think either way is fine, but @inheritdoc, not so much.
Comment #11
BerdirAssigning to @amateescu for his review :)
Comment #12
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedI 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
toDrupal/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).
Comment #13
BerdirAsked @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?
Comment #14
mbovan CreditAttribution: mbovan at MD Systems GmbH commentedSure, providing a patch based on #13.
Comment #15
mbovan CreditAttribution: mbovan at MD Systems GmbH commentedViewsSelection
extendsSelectionBase
now, so removing implementation ofSelectionInterface
andContainerFactoryPluginInterface
.Comment #16
alexpottre #13 Yeah renaming would need to be justified by something critical. Perhaps we can solve the naming issues with food documentation on both classes?
Comment #19
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedHow about this? I can't think of a better way of saying that the views selection plugin is not based on entity query :)
Comment #20
BerdirDiscussed with @amateescu, he's ok in general with extending like that. Patch looks good to me now.
Comment #21
alexpottThe issue summary could do with an update. Patch looks good to me.
Comment #22
jibranI'd like to see what @dawehner thinks about this patch.
Comment #24
mbovan CreditAttribution: mbovan at MD Systems GmbH commentedUpdated the issue summary.
Comment #25
BerdirI 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.
Comment #26
jibranRTBC +1
Comment #27
alexpottThis 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!