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
SelectionPluginManager uses the 'non-standard' ReflectionFactory instead of ContainerFactory because selection plugins need to receive a field definition, and this prevents us from doing the regular dependency injection dance.
Proposed resolution
Remove the coupling of selection plugins to a Field API field definition and let them accept a $configuration array instead.
Remaining tasks
Write a patch.
User interface changes
None.
API changes
The signature of selection plugins will change.
Related Issues
#1959806: Provide a generic 'entity_autocomplete' Form API element
Beta phase evaluation
Issue category | Task simply moving code around |
---|---|
Issue priority | Not critical but it'll improve security |
Prioritized changes | This improve security see #2341357-12: Views entity area config is not deployable and missing dependencies. |
Disruption | Disruptive for DER but can fix all the dirty workarounds (in other words PLEASE PLEASE PLEASE disrupt us) |
Comment | File | Size | Author |
---|---|---|---|
#41 | interdiff.txt | 915 bytes | amateescu |
#41 | 2107243-41.patch | 86.39 KB | amateescu |
#40 | Screenshot 2015-01-23 07.44.27.png | 15.27 KB | larowlan |
#40 | Screenshot 2015-01-23 07.43.12.png | 30.3 KB | larowlan |
Comments
Comment #1
dpiHas anyone taken a look at this recently?
I'd just like to add that this would be great for contrib modules such as Relation.
Comment #2
dpi#1959806: Provide a generic 'entity_autocomplete' Form API element has been postponed pending progress of this issue.
Comment #3
andypostSuppose settings should be unified first #2221559: Unify target_bundle vs target_bundles settings for entity reference fields
Comment #4
andypostComment #5
andypostComment #6
XanoComment #8
mgiffordComment #9
amateescu CreditAttribution: amateescu commentedWorking on this, I should have something by tomorrow.
Comment #10
amateescu CreditAttribution: amateescu commentedActually, let's see what the bot thinks about the current state of the patch.
Comment #11
pwolanin CreditAttribution: pwolanin commentedOverall this looks fine so far - some of the
On question is around a change of some of the plugin IDs like:
I think a ":" in the ID generally indicates a plugin derivative in current core via the const DERIVATIVE_SEPARATOR = ':';
Was that intended?
Comment #12
amateescu CreditAttribution: amateescu commentedYes, it was. When the original ER patch got in we didn't have #2184951: Allow plugins to declare themselves a derivative so I had to invent my own way of doing derivative overrides, see the code removed from the old
Drupal\entity_reference\Plugin\Derivative\SelectionBase::getDerivativeDefinitions()
.And here is the full patch.
Comment #13
jibranThank you for the speedy fix. This is a huge improvement for DER. I think #11 is answered in #12 so it is RTBC.
Comment #14
jibranAdded BE to IS.
Comment #15
jibranRight 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. So updated BE in IS accordingly.
Comment #16
amateescu CreditAttribution: amateescu commentedThanks @jibran, that's also what I had in mind for the issue summary.
Given that this is an important step for having generic Form API entity autocomplete elements and the security improvements gained, I think this is major.
Comment #17
jibranDo you think it is a good idea to add
SelectionPluginManagerInterface
?Comment #18
amateescu CreditAttribution: amateescu commentedI thought about it but couldn't decide if it's really needed or not :/
Comment #19
jibranSorry but missing auto create option for taxonomy ER field.
Let's add
SelectionPluginManagerInterface
as well.Comment #20
amateescu CreditAttribution: amateescu commentedAlright, let's do that.
Comment #21
jibranAwesome it worked locally so RTBC.
Comment #22
Wim LeersScanned this patch for nitpicks; didn't find any. Looks great!
Comment #23
jibranWe can use
SelectionPluginManagerInterface
here now instead ofSelectionPluginManager
.Comment #25
jibranFixed the test fails and #23 also removed
$field_definition
formEntityReferenceAutocomplete::getMatches()
.Comment #27
jibranFixed the fails.
Comment #28
jibranSome minor clean up.
Comment #29
larowlanNitpick:entity type ID?
https://www.drupal.org/node/2389335 aims to remove queryFactory service in favour of $storageHandler->getQuery() - should we ease that by cleaning these bits up here? We already have the entity manager, so can use $this->entityManager->getStorage($entity_type)->getQuery()
So this overrides the derived version of default from the deriver? If so can we add a comment that it works this way?
Same here and for term/node?
I was expecting to see a test that utilised the new decoupled entity-reference selection handler outside of an ER field - or is that not possible yet (i.e. this is an intermediate step).
Manual test next
Comment #30
larowlanManual testing
cannot add a user or node ER field from UI - looks like a 500 on the submit
Comment #31
jibranThank you for the review.
Let's do this in #1959806: Provide a generic 'entity_autocomplete' Form API element. It seems more related.
I can add the fields normally locally. Perhaps it is related to simpletest.me. #20 has user tests.
Comment #33
amateescu CreditAttribution: amateescu commented@jibran, I didn't include any autocomplete changes in this patch on purpose, that will be handled in #1959806: Provide a generic 'entity_autocomplete' Form API element. I'm sorry but I have to start again from #20.
This patch fixes #23 and #29.1 and .2.
For #29.3 and .4, this is the standard way of doing derivative overrides since #2184951: Allow plugins to declare themselves a derivative, which didn't include any special documentation for those overrides...
Comment #34
amateescu CreditAttribution: amateescu commentedI previously missed a few places where we could use DI and the injected entity manager instead of procedural functions.
Comment #35
amateescu CreditAttribution: amateescu commentedThere's also a "new" PluginFormInterface that we can use now.
Comment #37
amateescu CreditAttribution: amateescu commentedThat's a very good point. This is perfectly possible now so I updated two tests that were strictly related to the selection plugins.
Comment #38
amateescu CreditAttribution: amateescu commentedPhew, this was such an old codebase.. I think I'm done now :/
Comment #39
amateescu CreditAttribution: amateescu commentedFixed a >80 chars docblock and created a smaller patch, thanks to @larowlan :)
Comment #40
larowlanUnless bot disagrees, this is rtbc
Manual testing shots:
Comment #41
amateescu CreditAttribution: amateescu commentedFixed a stale method reference.
Comment #42
alexpottThis change is not unfrozen see https://www.drupal.org/core/beta-changes#unfrozen
However the decoupling to make things easier for DER and improvements to security make this good to go. Committed 5d20c57 and pushed to 8.0.x. Thanks!
Comment #44
jibranThanks @alexpott for committing it, @amateescu for fixing it and @larowlan reviewing it. Let's fix #1959806: Provide a generic 'entity_autocomplete' Form API element now.
Comment #45
jibranWe forgot to add
SelectionBase::buildEntityQuery()
toSelectionInterface
. Should I create a follow up for that? Perhaps we can make it protected.Comment #46
amateescu CreditAttribution: amateescu commentedInterfaces can not have protected methods. And why would we want
buildEntityQuery()
on the interface in the first place? How would the Views selection implement it?Comment #47
jibranI mean if it is only an internal method for all the
EntityReferenceSelection
plugins extended fromSelectionBase
then we can make it protected onSelectionBase
class not theSelectionInterface
.ViewsSelection
has implemented emptyentityQueryAlter()
so we can also implement emptybuildEntityQuery()
So there are two ways to fix this.
Either make
entityQueryAlter()
andbuildEntityQuery()
protected forSelectionBase
and removeentityQueryAlter()
fromSelectionInterface
or addbuildEntityQuery()
toSelectionInterface
and implement and empty methodbuildEntityQuery()
forViewsSelection
.Comment #48
amateescu CreditAttribution: amateescu commentedOk, let's open a separate issue to make it protected :)
Edit: opened #2421849: Make SelectionBase::buildEntityQuery() protected.