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.

#1959806: Provide a generic 'entity_autocomplete' Form API element

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
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)
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dpi’s picture

Issue summary: View changes

Has anyone taken a look at this recently?

I'd just like to add that this would be great for contrib modules such as Relation.

dpi’s picture

#1959806: Provide a generic 'entity_autocomplete' Form API element has been postponed pending progress of this issue.

andypost’s picture

andypost’s picture

andypost’s picture

Xano’s picture

Assigned: amateescu » Unassigned
Status: Active » Needs review
FileSize
5.02 KB

Status: Needs review » Needs work

The last submitted patch, 6: drupal_2107243_6.patch, failed testing.

mgifford’s picture

amateescu’s picture

Assigned: Unassigned » amateescu

Working on this, I should have something by tomorrow.

amateescu’s picture

Status: Needs work » Needs review
FileSize
46.93 KB

Actually, let's see what the bot thinks about the current state of the patch.

pwolanin’s picture

Overall this looks fine so far - some of the

On question is around a change of some of the plugin IDs like:

- *   id = "file_default",
+ *   id = "default:file"

I think a ":" in the ID generally indicates a plugin derivative in current core via the const DERIVATIVE_SEPARATOR = ':';

Was that intended?

amateescu’s picture

Assigned: amateescu » Unassigned
FileSize
57.52 KB
19.99 KB

I think a ":" in the ID generally indicates a plugin derivative in current core via the const DERIVATIVE_SEPARATOR = ':';

Was that intended?

Yes, 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.

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Thank you for the speedy fix. This is a huge improvement for DER. I think #11 is answered in #12 so it is RTBC.

jibran’s picture

Issue summary: View changes

Added BE to IS.

jibran’s picture

Issue summary: View changes

Right 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.

amateescu’s picture

Title: Decouple ER selection plugins from field definitions » Decouple entity reference selection plugins from field definitions
Priority: Normal » Major

Thanks @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.

jibran’s picture

Do you think it is a good idea to add SelectionPluginManagerInterface?

amateescu’s picture

I thought about it but couldn't decide if it's really needed or not :/

jibran’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Need tests
FileSize
25.98 KB

Sorry but missing auto create option for taxonomy ER field.

Let's add SelectionPluginManagerInterface as well.

amateescu’s picture

Status: Needs work » Needs review
Issue tags: -Need tests
FileSize
63.87 KB
6.46 KB

Alright, let's do that.

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Awesome it worked locally so RTBC.

Wim Leers’s picture

Scanned this patch for nitpicks; didn't find any. Looks great!

jibran’s picture

+++ b/core/modules/entity_reference/src/EntityReferenceAutocomplete.php
@@ -39,7 +39,7 @@ class EntityReferenceAutocomplete {
   public function __construct(EntityManagerInterface $entity_manager, SelectionPluginManager $selection_manager) {

We can use SelectionPluginManagerInterface here now instead of SelectionPluginManager.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 20: 2107243-20.patch, failed testing.

jibran’s picture

Status: Needs work » Needs review
FileSize
5.87 KB
67.81 KB

Fixed the test fails and #23 also removed $field_definition form EntityReferenceAutocomplete::getMatches().

Status: Needs review » Needs work

The last submitted patch, 25: 2107243-25.patch, failed testing.

jibran’s picture

Status: Needs work » Needs review
FileSize
69.32 KB
1.51 KB

Fixed the fails.

jibran’s picture

FileSize
69.48 KB
2.59 KB

Some minor clean up.

larowlan’s picture

  1. +++ b/core/lib/Drupal/Core/Entity/EntityReferenceSelection/SelectionPluginManagerInterface.php
    @@ -0,0 +1,44 @@
    +   *   A Drupal entity type.
    

    Nitpick:entity type ID?

  2. +++ b/core/lib/Drupal/Core/Entity/Plugin/EntityReferenceSelection/SelectionBase.php
    @@ -32,42 +37,89 @@
    +   * @var \Drupal\Core\Entity\Query\QueryFactory
    ...
    +  protected $queryFactory;
    
    @@ -280,11 +332,11 @@ public function validateAutocompleteInput($input, &$element, FormStateInterface
    +    $query = $this->queryFactory->get($target_type);
    

    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()

  3. +++ b/core/modules/comment/src/Plugin/EntityReferenceSelection/CommentSelection.php
    @@ -2,20 +2,20 @@
    + *   id = "default:comment",
    

    So this overrides the derived version of default from the deriver? If so can we add a comment that it works this way?

  4. +++ b/core/modules/file/src/Plugin/EntityReferenceSelection/FileSelection.php
    @@ -2,18 +2,18 @@
    + *   id = "default:file",
    

    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

larowlan’s picture

Status: Needs review » Needs work
FileSize
90.16 KB

Manual testing
cannot add a user or node ER field from UI - looks like a 500 on the submit

jibran’s picture

Status: Needs work » Needs review
FileSize
7.08 KB
70.07 KB

Thank you for the review.

  1. Fixed.
  2. Nice catch done.
  3. Added a comment and @see please let me know if you want me to improve it.

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).

Let's do this in #1959806: Provide a generic 'entity_autocomplete' Form API element. It seems more related.

cannot add a user or node ER field from UI - looks like a 500 on the submit

I can add the fields normally locally. Perhaps it is related to simpletest.me. #20 has user tests.

Status: Needs review » Needs work

The last submitted patch, 31: 2107243-31.patch, failed testing.

amateescu’s picture

Assigned: Unassigned » amateescu
Status: Needs work » Needs review
FileSize
64.33 KB
7.48 KB

@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...

amateescu’s picture

FileSize
75.26 KB
14.5 KB

I previously missed a few places where we could use DI and the injected entity manager instead of procedural functions.

amateescu’s picture

FileSize
76.87 KB
8.75 KB

There's also a "new" PluginFormInterface that we can use now.

The last submitted patch, 34: 2107243-34.patch, failed testing.

amateescu’s picture

FileSize
88.68 KB
14.53 KB

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).

That's a very good point. This is perfectly possible now so I updated two tests that were strictly related to the selection plugins.

amateescu’s picture

Assigned: amateescu » Unassigned

Phew, this was such an old codebase.. I think I'm done now :/

amateescu’s picture

FileSize
86.38 KB
757 bytes

Fixed a >80 chars docblock and created a smaller patch, thanks to @larowlan :)

larowlan’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
FileSize
30.3 KB
15.27 KB

Unless bot disagrees, this is rtbc

Manual testing shots:

amateescu’s picture

FileSize
86.39 KB
915 bytes

Fixed a stale method reference.

alexpott’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Fixed

This 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!

  • alexpott committed 5d20c57 on 8.0.x
    Issue #2107243 by amateescu, jibran, larowlan, Xano: Decouple entity...
jibran’s picture

Thanks @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.

jibran’s picture

We forgot to add SelectionBase::buildEntityQuery() to SelectionInterface. Should I create a follow up for that? Perhaps we can make it protected.

amateescu’s picture

We forgot to add SelectionBase::buildEntityQuery() to SelectionInterface. Should I create a follow up for that? Perhaps we can make it protected.

Interfaces 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?

jibran’s picture

Perhaps we can make it protected.

I mean if it is only an internal method for all the EntityReferenceSelection plugins extended from SelectionBase then we can make it protected on SelectionBase class not the SelectionInterface.

How would the Views selection implement it?

ViewsSelection has implemented empty entityQueryAlter() so we can also implement empty buildEntityQuery()
So there are two ways to fix this.
Either make entityQueryAlter() and buildEntityQuery() protected for SelectionBase and remove entityQueryAlter() from SelectionInterface or add buildEntityQuery() to SelectionInterface and implement and empty method buildEntityQuery() for ViewsSelection.

amateescu’s picture

Ok, let's open a separate issue to make it protected :)

Edit: opened #2421849: Make SelectionBase::buildEntityQuery() protected.

Status: Fixed » Closed (fixed)

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