Add a base class for entity reference selection handlers and fix the structure of their configuration

In principle, this is a good thing to do. I also tried to introduce a base class for selection plugins but it was too late in the dev cycle to get it in before 8.0.0, so I'm obviously +1 :)

1. +++ b/core/lib/Drupal/Component/Plugin/DependencyRemovalPluginInterface.php
   @@ -0,0 +1,30 @@
   +interface DependencyRemovalPluginInterface {
   

   I don't think the generic plugin system should know or care about our (Drupal's) config dependency mechanism. And it's certainly not something to be introduced in a small ER issue like this one :)

2. +++ b/core/lib/Drupal/Core/Entity/EntityReferenceSelection/SelectionPluginBase.php
   @@ -0,0 +1,133 @@
   + * Provides a base class for configurable selection handlers
   

   Missing a . at the end of the sentence.

3. +++ b/core/modules/views/src/Plugin/EntityReferenceSelection/ViewsSelection.php
   @@ -24,80 +18,33 @@
   +    ] + parent::defaultHandlerSettings();
   

   Shouldn't we put the parent's method call first like in the other handlers?

+++ b/core/lib/Drupal/Core/Entity/EntityReferenceSelection/SelectionPluginBase.php
@@ -113,13 +112,6 @@ public function calculateDependencies() {
-  public function onDependencyRemoval(array $dependencies) {
-    return FALSE;
-  }

We can save this discussion for the other issue, but I don't think adding that interface to the plugin system is going to fly, so we might as well just add this method to SelectionInterface.

Also, since this is a task, I think it will have to get in 8.3.x, no?

Ok, I'll let the committers decide if this is 8.2.x material or not :)

I just remembered that there is also an existing issue for making DefaultSelection implement ConfigurablePluginInterface: #2636322: Entity reference selection plugins cannot be instantiated without configuration

And there is a problem with the config schema, see comments #6, #8, #9 and #10 over there. Not sure yet what' the best way to handle this.. :/

If we are changing the schema then we need a post update hook and upgrade path test.

1. +++ b/core/config/schema/core.data_types.schema.yml
   @@ -769,11 +769,20 @@ text_format:
   +# Schema for all entity reference selection handlers that are not providing a
   +# specific schema.
   +entity_reference_selection.*:
   +  type: entity_reference_selection
   

   Its always nice to have these fallbacks!

2. +++ b/core/lib/Drupal/Core/Entity/EntityReferenceSelection/SelectionPluginBase.php
   @@ -0,0 +1,125 @@
   + */
   +abstract class SelectionPluginBase extends PluginBase implements SelectionInterface, ContainerFactoryPluginInterface, ConfigurablePluginInterface {
   +
   +  /**
   +   * The entity manager service.
   +   *
   +   * @var \Drupal\Core\Entity\EntityManagerInterface
   +   */
   +  protected $entityManager;
   +
   +  /**
   +   * The module handler service.
   +   *
   +   * @var \Drupal\Core\Extension\ModuleHandlerInterface
   +   */
   +  protected $moduleHandler;
   +
   +  /**
   +   * The current user.
   +   *
   +   * @var \Drupal\Core\Session\AccountInterface
   +   */
   +  protected $currentUser;
   

   Here is a question, why do we move all those dependencies into the base class, even nothing in this class actually uses it?

This looks great, just a minor remark.

+++ b/core/lib/Drupal/Core/Entity/EntityReferenceSelection/BasicSelectionPluginBase.php
@@ -0,0 +1,75 @@
+/**
+ * Provides a base class for core selection handlers.
+ */
+abstract class BasicSelectionPluginBase extends SelectionPluginBase implements ContainerFactoryPluginInterface {
+

We could add a comment like "Extends the base selection plugin with providing a list of helpful services by default".

Cool, thank you

1. +++ b/core/lib/Drupal/Core/Entity/EntityReferenceSelection/BasicSelectionPluginBase.php
   @@ -0,0 +1,78 @@
   +abstract class BasicSelectionPluginBase extends SelectionPluginBase implements ContainerFactoryPluginInterface {
   

   I don't really like this extra layer. IMO, traits should be used for code re-use, not class inheritance. But maybe that's just me...

2. +++ b/core/modules/taxonomy/src/Plugin/EntityReferenceSelection/TermSelection.php
   @@ -24,13 +23,6 @@ class TermSelection extends DefaultSelection {
   -    // @todo: How to set access, as vocabulary is now config?
   

   We're losing this @todo.

I don't really like this extra layer. IMO, traits should be used for code re-use, not class inheritance. But maybe that's just me...

Great point!

What we expect from TermSelection::entityQueryAlter() and cannot be done so that we need to add a @todo?

I would expect it to match what it was doing in ER 7.x :)

I also looked a bit at the D7 code and it's probably ok to remove that @todo. Here's a few more points:

1. +++ b/core/lib/Drupal/Core/Entity/EntityReferenceSelection/SelectionPluginBase.php
   @@ -0,0 +1,100 @@
   +   *   The current user.
   

   Leftover :)

2. +++ b/core/lib/Drupal/Core/Entity/EntityReferenceSelection/SelectionPluginBase.php
   @@ -0,0 +1,100 @@
   +  public function defaultConfiguration() {
   +    return [
   +      'target_type' => NULL,
   +      'handler' => NULL,
   +      'handler_settings' => $this->defaultHandlerSettings(),
   +    ];
   +  }
   

   Conceptually, the configuration of a selection plugin is actually what we currently store in 'selection_settings' plus the 'target_type', because, for example, the 'handler' setting doesn't make sense as we already have an instantiated plugin/handler by the time we are able to call defaultConfiguration() on it.

   This is also covered in #2636322-9: Entity reference selection plugins cannot be instantiated without configuration.

3. +++ b/core/lib/Drupal/Core/Entity/EntityReferenceSelection/SelectionPluginBase.php
   @@ -0,0 +1,100 @@
   +   * {@inheritdoc}
   +   */
   +  public function calculateDependencies() {
   

   Where does this inherit from?

4. +++ b/core/lib/Drupal/Core/Entity/EntityReferenceSelection/SelectionPluginBase.php
   @@ -0,0 +1,100 @@
   +   * @return array
   +   *
   +   * @todo Add this to SelectionInterface in Drupal 9.0.x.
   +   */
   +  public function defaultHandlerSettings() {
   

   We need to describe the return value a bit.

5. +++ b/core/lib/Drupal/Core/Entity/EntityReferenceSelection/SelectionTrait.php
   @@ -0,0 +1,74 @@
   + * Provides common methods and inject services for core selection handlers.
   

   inject -> injects

6. +++ b/core/lib/Drupal/Core/Entity/Plugin/EntityReferenceSelection/DefaultSelection.php
   @@ -37,89 +32,38 @@
   +      'sort' => [
   +        'field' => '_none',
   +      ],
   

   We also need to include the default sort direction and remove the default assignment from \Drupal\Core\Entity\Plugin\EntityReferenceSelection\DefaultSelection::buildConfigurationForm().

7. +++ b/core/modules/user/src/Plugin/EntityReferenceSelection/UserSelection.php
   @@ -82,16 +82,20 @@ public static function create(ContainerInterface $container, array $configuratio
   +      'filter' => [
            'type' => '_none',
   -      ),
   

   We also need to include a default for the 'role' filter and remove the default value assignment in \Drupal\user\Plugin\EntityReferenceSelection\UserSelection::buildConfigurationForm().

Hmm.. #35.2 is not fixed, the 'handler' config was just an example. Please read #2636322-9: Entity reference selection plugins cannot be instantiated without configuration :)

Discussed this at length during Drupalcon Dublin with @claudiu.cristea, and the resulting patch looks very good to me :)

+++ b/core/lib/Drupal/Core/Entity/EntityReferenceSelection/SelectionPluginBase.php
@@ -0,0 +1,123 @@
+
+    // In order to keep backward compatibility, we copy all settings, except
+    // 'target_type', 'handler' and 'entity' under 'handler_settings', following
+    // the structure from the field config. If the plugin was instantiated using

It's not clear to me what the implication of this is:

1. Is it possible to have field config that matches the new structure so the bc layer doesn't run?

2. If so why not convert core field types to the new structure via an upgrade path?

I realise setConfiguration() can theoretically come from somewhere that's not config, so we can't necessarily remove the bc layer from there, but we should be able to make it so it doesn't normally run (i.e. have an upgrade path site configuration and a preSave() to convert old default configuration to the new format on import).

If we did all that, we could @trigger_error() in the bc handling and have core tests pass even when deprecations are set to cause a fail - and that gives contrib/custom code a pointer how to update then.

+++ b/core/modules/taxonomy/src/Plugin/EntityReferenceSelection/TermSelection.php
@@ -41,7 +41,9 @@ public function buildConfigurationForm(array $form, FormStateInterface $form_sta
+      $configuration['handler_settings']['sort'] = ['field' => 'name', 'direction' => 'asc'];

Don't we want to skip the 'handler_settings' part of the array?

Looks great!

+++ b/core/modules/taxonomy/src/Plugin/EntityReferenceSelection/TermSelection.php
@@ -49,15 +41,16 @@ public function buildConfigurationForm(array $form, FormStateInterface $form_sta
-      $this->configuration['handler_settings']['sort'] = ['field' => 'name', 'direction' => 'asc'];
+      $configuration = $this->getConfiguration();
+      $configuration['sort'] = ['field' => 'name', 'direction' => 'asc'];
+      $this->setConfiguration($configuration);

Now that I look at this again, wouldn't it be better to set this in TermSelection::defaultConfiguration()?

Looks great now!

Surprisingly, DER 8.x-1.x branch is also green this patch applied to the core. Patch looks good. There is no BC break. +1 for RTBC.

+++ b/core/lib/Drupal/Core/Entity/EntityReferenceSelection/SelectionPluginBase.php
@@ -0,0 +1,133 @@
+    // In order to keep backward compatibility, we copy all settings, except
+    // 'target_type', 'handler' and 'entity' under 'handler_settings', following
+    // the structure from the field config. If the plugin was instantiated using
+    // the 'handler_settings' level, those values will be used. In case of
+    // conflict, the root level settings will take precedence. The backward
+    // compatibility aware configuration will have the next structure:

Can we move the bc handling out to a separate protected method, and also @trigger_error('...', E_USER_DEPRECATED) when it finds the old keys. That will help to isolate it as well as inform people when they're relying on it. Can also mark the actual protected method @deprecated and for 9.x removal then.

@claudiu.cristea that change looks great :) We don't have a standard way to test deprecation methods yet. I think it's fine to add a non-standard test but will ping alexpott since he's been thinking about that more.

+++ b/core/lib/Drupal/Core/Entity/EntityReferenceSelection/SelectionPluginBase.php
@@ -130,4 +93,64 @@ public function submitConfigurationForm(array &$form, FormStateInterface $form_s
+   * Assures a BC level configuration.
...
+  protected function assureBcConfiguration() {

We usually use "ensure" for stuff like this.

And the "Bc" part of the new method names looks a bit weird, but I also don't have any better suggestion right now :)

I think this is ready.

Improved the title a bit :)

We need a change record for this.

The BC layer looks good and already complies with the policy apart from the fact it is missing a link to a change record.

The change record looks very good and detailed, back to RTBC :)

Thanks @claudiu.cristea

It is green again so back to RTBC.

Committed/pushed to 8.4.x, thanks! Sorry this took a while to get back to.

Can it be considered for backporting to 8.3.x? It unblocks the bug #2786841: Entity reference fields should add selection handler config dependencies.

It's an API addition and new deprecation, so normally we wouldn't backport it even to fix a major bug, we do make occasional exceptions for very serious issues where the disruption is unlikely, but some of those cases have broken patch releases too.

Either way we should get that issue fixed in 8.4.x first, then figure out if/how to backport both afterwards.

Thank you @catch for clearing that up.

Hm, looks like this broke config schema definitions in custom/contrib, if it extended from entity_reference_selection. See #2881459: Build failing: Schema errors for field.field.node.paragraphed_content_demo.field_paragraphs_demo with the following errors: field.field.node.paragraphed_content_demo.field_paragraphs_demo:settings.handler_settings.target_bundles .

Should we do a follow-up to add the schema back with a @deprecated comment?

#2881874: Fatal DefaultSelection has colliding constructor definitions coming from traits on PHP 5.5.9

I think we need to revert this rather than a followup. We can do so once the PHP 5.5 tests in #121 continue running to confirm or rule out that that issue is related to PHP 5.5 (versus a contrib module).

I reproduced the fatal noticed in #2881874: Fatal DefaultSelection has colliding constructor definitions coming from traits on PHP 5.5.9.

#113 causes the following when using the node add form on PHP 5.5.9:

Fatal error: Drupal\Core\Entity\Plugin\EntityReferenceSelection\DefaultSelection has colliding constructor definitions coming from traits in /vagrant/www/core/lib/Drupal/Core/Entity/Plugin/EntityReferenceSelection/DefaultSelection.php on line 375

What's very interesting is that I triggered a PHP 5.5 test run on QA before we reverted this and it passed, so something weird is going on, but we still need to figure out exactly what was causing the fatal and fix it.

Thanks @vaplas. Apparently it also reduced the number of non-random fails. ;) I wonder what the best way to test 5.5.9 is? Maybe we should have a minimum supported version environment that does not run nightly but can be added to run a specific test.

Is it NW just for #125? or does it include #120 as well?

Re-rolled #113 with patch from #2881874-7: Fatal DefaultSelection has colliding constructor definitions coming from traits on PHP 5.5.9 and improved the function name hence two interdiffs. Please add @tetranz to credit list from #2881874: Fatal DefaultSelection has colliding constructor definitions coming from traits on PHP 5.5.9.

How about now?

1. +++ b/core/lib/Drupal/Core/Entity/EntityReferenceSelection/SelectionPluginBase.php
   @@ -0,0 +1,160 @@
   +      if (!in_array($key, ['handler', 'target_type', 'entity', 'handler_settings'])) {
   

   nit:in case there are numeric keys, we should use the third argument here for type safety sake and avoid weird side-effects.

2. +++ b/core/lib/Drupal/Core/Entity/EntityReferenceSelection/SelectionTrait.php
   @@ -0,0 +1,74 @@
   +  public function __construct(array $configuration, $plugin_id, $plugin_definition, EntityManagerInterface $entity_manager, ModuleHandlerInterface $module_handler, AccountInterface $current_user) {
   
   +++ b/core/lib/Drupal/Core/Entity/Plugin/EntityReferenceSelection/DefaultSelection.php
   @@ -38,88 +36,51 @@
   +    SelectionTrait::__construct as private selectionTraitConstruct;
   ...
   +    $this->selectionTraitConstruct($configuration, $plugin_id, $plugin_definition, $entity_manager, $module_handler, $current_user);
   

   Are we sure we want a trait with a constructor? This is going to lead to some bad DX in my opinion.

   Couldn't this be a named method like 'initialize()' or something that would make this less messy?

3. +++ b/core/lib/Drupal/Core/Entity/Plugin/EntityReferenceSelection/DefaultSelection.php
   @@ -198,12 +159,7 @@ public function buildConfigurationForm(array $form, FormStateInterface $form_sta
   +      if ($configuration['sort']['field'] != '_none') {
   

   off topic: we really need that issue that converts _none to a constant to land

note: we could call the initialize/setup method in a constructor on the base class so we don't have the confusion

For the record we have 119 traits in core.

None of them have constructors

Fixed #34.

Committed/pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!