Unable to create config schema for entity type specific entity reference selection plugin.

Tagging.g

I think all we need to do is all the plugin ID and use that with a fallback.

The views changes show nicely that plugin_id really is a duplicate of handler, we just don't store the complete plugin_id there.

I guess that's OK, so that we can keep the changes here limited, changing how handler works would be a much bigger change.

I think it would be good to get feedback from @amateescu here.

Note: We already moved parts of entity_reference.schema.yml into core when it was needed, I think we should move the remaining parts as well, but this might not be the right issue.

This might be same problem I ran into in #2429191: Deprecate entity_reference.module and move its functionality to core.

+++ b/core/lib/Drupal/Core/Entity/Plugin/EntityReferenceSelection/SelectionBase.php
@@ -118,6 +118,12 @@ public function buildConfigurationForm(array $form, FormStateInterface $form_sta
+    // Add the plugin ID to enable static configuration schema traversal.
+    $form['plugin_id'] = array(
+      '#type' => 'value',
+      '#value' => $this->getPluginId(),
+    );

The problem with this is that it only works for fields created in the UI but not for those created programatically (e.g. in tests).

Another problem is that the 'handler' field setting is actually the "handler group", while the exact selection plugin ID is derived at runtime from the "target_type" storage setting and "handler" field setting.

I thought about using FieldItemInterface::fieldSettingsToConfigData() to populate the plugin_id automatically, but there we don't have access to the target_type because it's a storage setting.

In any case, since we're doing schema updates here, we should also rename entity_reference.handler_settings because selection handlers are provided by core now, not by the entity_reference module, so I extracted the relevant parts from #2429191: Deprecate entity_reference.module and move its functionality to core in this interdiff.

Oops, #9 is a cross-post with #6-#8. But it does answer #8 a bit :)

+++ b/core/modules/entity_reference/src/Tests/Views/SelectionTest.php
@@ -67,6 +67,7 @@ public function setUp() {
+          'plugin_id' => 'views',

+++ b/core/modules/user/src/Tests/UserEntityReferenceTest.php
@@ -68,6 +68,7 @@ function testUserSelectionByRole() {
+    $field_definition->settings['handler_settings']['plugin_id'] = 'default:user';

I don't think that doing this in every test is a good solution because FieldConfig::create() will still be broken for ER fields if you don't know that you have to pass a plugin_id in handler_settings.

Side note: also found #2437785: FieldConfigStorageBase::mapToStorageRecord() calls the wrong field type method while looking into this.

The patch attached is based on the patch #4 - let's get it green before doing more renaming. Anyway we have an interface we should be using! ConfigurablePluginInterface - Which also highlights the fact we're missing dependencies :)

The interdiff is back to #1.

The changes in #14 are good to have, but, as I said in #11, they're not what's needed to fix the root cause :)

Even if we implement ConfigurablePluginInterface, the base issue is still that we are creating schemas based on a 'plugin_id' property that's computed on runtime.

+++ b/core/config/schema/core.data_types.schema.yml
@@ -566,7 +566,7 @@ field.field_settings.entity_reference:
-      type: entity_reference.[%parent.handler].handler_settings
+      type: entity_reference.handler_settings.[plugin_id]

To express the problem in a different way, we would need something like:

type: entity_reference.handler_settings.[%parent.handler]:[%get.the.storage.target_type.setting.somehow]

Edit: nevermind that, it doesn't make any sense :)

Fixing test fails.

I think the FieldConfig::create() is fixed because we set the default values in the entity selection plugin construction.

I think the FieldConfig::create() is fixed because we set the default values in the entity selection plugin construction.

If that's true, this patch should have the same testbot result as #18.

Damn you field type plugins and all your static funkiness.

We need to pass the field definition into these funky statics to stand a chance of delegating the the entity reference selection plugin.

Note: we already pass the field definition into to FieldItemInterface::calculateDependencies for similar reasons - so if we all think this is a good approach then we should open up an issue to do this for FieldItemInterface::defaultStorageSettings() and FieldItemInterface::defaultFieldSettings()

Re #22: I hope I'm allowed to disagree with the approach you're after. I tried to explain in #9 and #15 what's the actual problem here but apparently my point didn't get across :(

Let me try again: the problem is not what we get from FieldItemInterface::defaultFieldSettings(), that's just a default value that can be overridden 1) in the UI by changing the selection handler and 2) in FieldConfig::create() by passing custom values for ['settings']['handler'] and ['settings']['handler_settings'].

What we actually want is the ability to store in the field settings configuration the plugin_id that is determined by analyzing the 'target_type' storage setting and the 'handler' field setting. AFAIS, the best place to do that is in FieldItemInterface::fieldSettingsToConfigData(). So a much smaller change is to just pass the field configuration entity from FieldConfigStorageBase::mapToStorageRecord() to FieldItemInterface::fieldSettingsToConfigData().

I implemented this approach in the patch attached, which is based on #9 and also includes the fix from #2437785: FieldConfigStorageBase::mapToStorageRecord() calls the wrong field type method. Note that the tests are not included because the patch over there is quite self-contained and could easily get committed before this one.

@amateescu of course you can disagree. I think conceptually using ConfigurablePluginInterace and providing the correct default values is correct though. The patch in #21 is using the target type to determine the correct plugin. Also the solution in #24 only copes with adding the default value for plugin id. Selection plugins have more field setting defaults than just that.

I think conceptually using ConfigurablePluginInterace and providing the correct default values is correct though.

I already said that I agree with this in #15, but changing selection plugins to use that interface belongs to a separate issue.

Also the solution in #24 only copes with adding the default value for plugin id.

Because that's the only thing needed to fix the bug :)

This is issue is about assigning default values from a configurable plugin for a config entity. Unfortunately fields are a bit special so we don't have some of the niceties that other config entities that leverage the plugin system enjoy. But I think it is in scope to explore whether or not it is possible to use ConfigurablePluginInterace especially considering we're going to have to fix the config dependencies aspect of this too.

Also, yes #28 fixes the bug but seeing a fieldSettingsToConfigData without a fieldSettingsFromConfigData feels icky.

Here is a patch that makes #26 green.

If this test doesn't convince you that this disagreement is not about default values, I don't know what will :/

Also, yes #28 fixes the bug but seeing a fieldSettingsToConfigData without a fieldSettingsFromConfigData feels icky.

I went with that approach because I wanted to keep this contained in the field item class, and it seems that fieldSettingsToConfigData() is the only way for it to act on "field config presave". If that feels icky, the fix can also be implemented like this.

So here is patch with no plugin ID being added. Instead handler is now the plugin ID of the handler for configurable entity references and the test in #30 is included.

1. +++ b/core/lib/Drupal/Core/Entity/Plugin/EntityReferenceSelection/SelectionBase.php
   @@ -388,4 +381,47 @@ protected function reAlterQuery(AlterableInterface $query, $tag, $base_table) {
   +    return [
   +      'handler' => $this->getPluginId(),
   +      'handler_settings' => [
   

   a bit uncommon that handler is actually in the configuration as well, usually only the stuff inside handler_settings is considered configuration?

2. +++ b/core/modules/views/src/Plugin/EntityReferenceSelection/ViewsSelection.php
   @@ -269,4 +271,40 @@ public function settingsFormValidate($element, FormStateInterface $form_state, $
   +  public function calculateDependencies() {
   +    return [];
   

   should also have a @todo to depend on the configured view I guess? Do you mean to implement those @todo's here or in a follow-up?

This is issue is about assigning default values from a configurable plugin for a config entity.

The issue title and summary say that this issue is about allowing users to write config schema for individual selection plugins.

+++ b/core/lib/Drupal/Core/Entity/EntityReferenceSelection/SelectionPluginManager.php
@@ -45,17 +45,20 @@ public function getInstance(array $options) {
-      'handler' => 'default',
+      'handler' => 'default:' . $options['target_type'],
...
+    // Extract the selection group from the handler.
+    list($selection_group) = explode(':', $options['handler'], 2);

@@ -83,10 +86,11 @@ public function getSelectionGroups($entity_type_id) {
+      'handler' => $field_definition->getSetting('handler') ?: 'default:' . $target_type,

The 'handler' setting was never meant to be the selection plugin ID, as I explained in #9.

The plugin ID can only be computed on runtime based on a selection group and a target entity type, that's the design of the selection plugin (manager) so the patch in #33 introduces a regression in that regard.

I also fell into the same trap with my recent patches, this one should be better and less "icky".

Can we please focus on a patch that fixes the problem at hand and open a different issue to discuss using ConfigurablePluginInterace? Like I said before, I totally agree with that except for the part that it should be used for the plugin ID.

But

Another problem is that the 'handler' field setting is actually the "handler group", while the exact selection plugin ID is derived at runtime from the "target_type" storage setting and "handler" field setting.

Is exactly the problem. Configuration for the plugin is being saved to a static file. It being used to configure a possibly different plugin is wrong.

That's up to the plugin (author) to decide. If the "override plugin" can enhance the functionality of the original one just by "being there" (i.e. in the same group, with a higher weight), that's great. If it needs to add extra configuration options then it will also need to use a different group, like OG and Entityqueue are doing in D7 contrib.

@amateescu but this kind of magic replacement of a plugin is unlike anything else we have.

I'm well aware of that but does it imply that it's a bad thing?

Kinda because the the configuration is for the plugin you've chosen. To quote @Berdir:

right, config and magic don't like each other

Yes, but like I said, it's up to the plugin to decide if it can use only the configuration of its base plugin or not. And even if it's in the same group but it defines extra configuration options, it can provide it's own schema which will be used when the field is saved. Afaik, we don't do anything with config schema at runtime so this use case/scenario should work fine.

1. +++ b/core/config/schema/core.data_types.schema.yml
   @@ -754,3 +754,35 @@ text_format:
   +entity_reference_selection:
   +  type: mapping
   +  label: 'View handler settings'
   

   Label is incorrect.

2. +++ b/core/modules/views/config/schema/views.entity_reference.schema.yml
   @@ -1,9 +1,12 @@
   -entity_reference.views.handler_settings:
   +entity_reference_selection.views:
      type: mapping
      label: 'View handler settings'
   

   Why would this not extend from the base type?

#42.1: Fixed.
#42.2: Because the 'views' selection plugin has a completely different set of configuration options than the 'default' one.

Default has: target_bundles, sort, auto_create
Views has: view['vew_name'], view['display_name'], view['arguments']

Needed an update after #2437785: FieldConfigStorageBase::mapToStorageRecord() calls the wrong field type method.

1. +++ b/core/modules/entity_reference/src/ConfigurableEntityReferenceItem.php
   @@ -216,4 +208,28 @@ public static function fieldSettingsFormValidate(array $form, FormStateInterface
   +    // The 'plugin_id' handler setting is saved to config only to satisfy
   +    // config schema and it is not used on runtime.
   +    unset($settings['handler_settings']['plugin_id']);
   +
   +    return $settings;
   

   Just be clear, we do the exact same in views. Its though tricky if you think about being able to alter out the classes behind plugins. I know this is a problem we probably haven't considered yet, but I would argue that the code doing is is responsible for updating the schema as well.

2. +++ b/core/modules/user/config/schema/user.schema.yml
   @@ -174,3 +174,20 @@ condition.plugin.user_role:
            - type: string
   +
   +entity_reference_selection.default:user:
   +  type: entity_reference_selection
   +  mapping:
   +    filter:
   +      type: mapping
   +      label: 'Filter settings'
   +      mapping:
   +        type:
   +          type: string
   +          label: 'Filter by'
   +        role:
   +          type: sequence
   +          label: 'Restrict to the selected roles'
   +          sequence:
   +            - type: string
   +              label: 'Role'
   

   Just curious, do we have test coverage, so an actual instance of that entity reference selection plugin as part of a test?

#46.1 I'm not sure I can extract an actionable item from what you said :)
#46.2 Yep, there is \Drupal\user\Tests\UserEntityReferenceTest.

#46.2 Yep, there is \Drupal\user\Tests\UserEntityReferenceTest.

I'm a bit confused, did it had strict schema checking disabled?

No, core was just cheating and had the user specific schema hardcoded in entity_reference.module.

I'm a bit confused, did it had strict schema checking disabled?

Nope, its "extra" configuration setting was included in the main/default one, entity_reference.default.handler_settings.

Honestly its not clear to me what is the state of this issue? What is not agreed on yet? No remaining tasks listed in the summary.

We need to agree on the fact that making selection plugins use ConfigurablePluginInterface is not required to fix this issue.

@dawehner already said in #46.1 that Views is doing the same thing as ER, so I guess it's just down to convincing @alexpott.. :)

Ok, updated the issue summary with that info and the list of API changes I identified hopefully right.

I didn't look at the recent patches, but I don't have a strong opinion on whether we need ConfigurablePluginInterface here or not.

What is worrying me a bit is how dynamic the plugin selection is. I used to think that it's just a $entity_type suffix that we add to the plugin id, but I found out that it is a lot more dynamic than that, and we're relying on selection groups and basically pick the first one in it. So enabling a module that provides a new one means that all your existing fields automatically use that.

Yes, it works with config schema at first, but there are two problematic things about it IMHO:

1. As you said, a selection plugin might or might not use the same configuration as the one that it replaced. But after enabling such a module, it will get *existing configuration* based on the previous plugin.. whether it asked for it or not.

2. While config schema works at first, what happens if you enable such a module and then do a config-export again. The plugin_id will automatically be updated, but the rest of the settings won't, so if the new plugin provides different configuration, you have a mismatch?

AFAIK, it doesn't matter if we use ConfigurablePluginInterface or not, those issues remain the same?

Also, this is different from views in two ways:

a) Views stores the plugin_id because before, it had nothing. The plugin was used based on the views data definition, so it had to add *something* to be able to validate the schema. However, with ER, we already have the handler, and we're just making it more specific.
b) The plugin id for views isn't selected *that* dynamically. To use a different plugin_id, either the original code that generates the views data has to explicitly change or someone that provides an alternative plugin has to explicitly alter it in. Which i think makes you a bit more aware of the implications when writing/changing code.

What I would expect to happen in a way is that we negotiate what plugin to us when a ER field is saved/updated and then we stick with that choice. We could still re-negotiate if you edit it again or something. Then we could rely on handler containing the actually used plugin_id and could rely on that for the config schema.

I totally agree with @berdir - dynamic selecting a plugin after saving the configuration and then possibly using another plugin if the code base has changed is an unnecessary opening for bugs and unexpected behaviour when moving configuration between sites.

Oh well, another useful feature from ER dropped. I'm starting to get used to this :)

Here's @alexpott's patch from #33, rerolled and with the schema changes from my approach.

There are only two things that "bother" me a bit in this patch:

1. +++ b/core/lib/Drupal/Core/Field/FieldTypePluginManager.php
   @@ -61,11 +61,11 @@ public function getDefaultStorageSettings($type) {
   -  public function getDefaultFieldSettings($type) {
   +  public function getDefaultFieldSettings($type, FieldDefinitionInterface $field = NULL) {
   
   +++ b/core/modules/entity_reference/src/ConfigurableEntityReferenceItem.php
   @@ -45,10 +47,15 @@ public static function defaultStorageSettings() {
   -  public static function defaultFieldSettings() {
   -    return array(
   -      'handler_settings' => array(),
   -    ) + parent::defaultFieldSettings();
   +  public static function defaultFieldSettings(FieldDefinitionInterface $field = NULL) {
   +    $defaults = ['handler' => 'default', 'handler_settings' => []];
   +    // Get the default settings from the entity reference selection plugin if
   +    // possible.
   +    if ($field->getFieldStorageDefinition()->getSetting('target_type')) {
   +      $handler = \Drupal::service('plugin.manager.entity_reference_selection')->getSelectionHandler($field);
   +      $defaults = NestedArray::mergeDeep($defaults, $handler->defaultConfiguration());
   +    }
   +    return $defaults;
      }
   

   It feels wrong for a field type to provide default field settings by inspecting the settings of a FieldDefinition object, because they are conceptually on the same level.

   I would be a lot more comfortable with it if we'd pass in a FieldStorageDefinition object instead, because it feels more natural (in terms of hierarchy) that a field's default settings can depend on its "parent" storage settings.

2. I'm not a fan of the (small) code duplication between SelectionBase and ViewsSelection), so maybe we should introduce a base plugin class from which both can extend. Also, the class name SelectionBase is very unfortunate atm, it should be DefaultSelectionBase. However, even if this change doesn't really affect anyone, it would make the patch unnecessarily large so I'd prefer to handle it in a followup.

The newly introduced test coverage from #2430813: Selecting the 'views' selection handler on an entity_reference field causes a fatal error shows that the patch in #33 introduced a small problem in the ER field settings UI, but easily fixable.

@alexpott, @Berdir, what do you think about #57.1?

I agree with #57.1, default values should not depend on the field itself, that's kind of recursive. As the use case we have shows, we only need the field storage definition, +1 to that.

Also discussed the class name a bit with @amateescu. I was confused with the name before, I think it should be DefaultSelection (no base as it is not an abstract class). What I'm not sure about is if this issue or a follow-up can find enough reasons to rename it to be accepted/make up for the small API change.

I know of one module that subclasses it (the one we created this issue for), I don't have a problem to update that, personally ( I also don't have to do it myself, I have my minions... erm, students for that)

very much +1 to #57.1 and #52, FSDI rather than FDI :-)

Also, much less impressed with @Berdir's constant awesomeness since I learned he has minions...

I tried to do the switch from passing a FSDI instead of FDI to defaultFieldSettings() in @alexpott's patch/approach but it led to some code which doesn't make any sense :( Here's an example from \Drupal\Core\Field\BaseFieldDefinition::create():

    $field_type_manager = \Drupal::service('plugin.manager.field.field_type');
    $default_settings = $field_type_manager->getDefaultStorageSettings($type) + $field_type_manager->getDefaultFieldSettings($type, $field_definition->getFieldStorageDefinition());
    $field_definition->itemDefinition->setSettings($default_settings);

In this case, the call to getDefaultFieldSettings() will pass a field storage definition with no default settings because we just got them before in the same line of code.

So I had to go back and reroll my patch from #31 and make it store an actual plugin ID in the 'handler' setting.

Fixed all that.

Re-fixing #42.

I think it is ready just few minor things.

1. +++ b/core/config/schema/core.data_types.schema.yml
   @@ -552,7 +552,7 @@ field.value.changed:
   +  label: 'Entity reference storage settings'
   

   field storage.

2. +++ b/core/config/schema/core.data_types.schema.yml
   @@ -560,14 +560,14 @@ field.storage_settings.entity_reference:
   +      label: 'Entity reference selection settings'
   

   ER selection plugin settings.

3. +++ b/core/lib/Drupal/Core/Entity/EntityReferenceSelection/SelectionPluginManager.php
   @@ -45,19 +45,35 @@ public function getInstance(array $options) {
   +      'handler' => $this->getPluginId($options['target_type'], 'default'),
   ...
   +  public function getPluginId($target_type, $base_plugin_id) {
   

   Why not make it optional and set it to $base_plugin_id=default?

Fixed #66.1 and 2.

For point 3, specifying a default base plugin id should be handled by the caller, not in the helper method, so I'd rather leave it as is.

Ok. Thank you for the fixes.

I'd like to see if @alexpott is ok with switching back to my approach after trying to use his in #57/#59.

1. +++ b/core/lib/Drupal/Core/Entity/EntityReferenceSelection/SelectionPluginManager.php
   @@ -45,19 +45,35 @@ public function getInstance(array $options) {
        // Initialize default options.
        $options += array(
   -      'handler' => 'default',
   +      'handler' => $this->getPluginId($options['target_type'], 'default'),
          'handler_settings' => array(),
        );
    
   +    // A specific selection plugin ID was already specified.
   +    if (strpos($options['handler'], ':') !== FALSE) {
   +      $plugin_id = $options['handler'];
   +    }
   +    // Only a selection group name was specified.
   +    else {
   +      $plugin_id = $this->getPluginId($options['target_type'], $options['handler']);
   +    }
   

   How come we are still supporting a none fully-qualified handler eg. default - is this a bc layer?

2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
   @@ -59,7 +59,8 @@ public static function defaultStorageSettings() {
   +      'handler' => 'default:' . (\Drupal::moduleHandler()->moduleExists('node') ? 'node' : 'user'),
   +      'handler_settings' => array(),
   

   This seems super odd since it won't also be node or user - and at this point don't we know the target entity type?

re #70.1 @Berdir in IRC said "I think the fallback is for example when #type entity_autocomplete is used?" - I see. Perhaps a comment here would be useful to explain which situations will follow which code path.

re #70.2 @Berdir pointed out that this is just mirroring EntityReferenceItem::defaultStorageSettings() - so makes sense.

Okay I'm going to stop moaning :) and get this done. Configuration has the full plugin ID stored in it which is what I primarily cared about. Since my patches only really influenced the direction of @amateescu's patches I think it is okay for me to commit this. Committed ae4848f and pushed to 8.0.x. Thanks!

diff --git a/core/modules/entity_reference/entity_reference.module b/core/modules/entity_reference/entity_reference.module
index 8560762..889eaf3 100644
--- a/core/modules/entity_reference/entity_reference.module
+++ b/core/modules/entity_reference/entity_reference.module
@@ -95,7 +95,7 @@ function entity_reference_field_storage_config_update(FieldStorageConfigInterfac
 /**
  * Implements hook_ENTITY_TYPE_presave() for 'field_config'.
  *
- * Determine the selection hander plugin ID for an entity reference field.
+ * Determine the selection handler plugin ID for an entity reference field.
  */
 function entity_reference_field_config_presave(FieldConfigInterface $field) {
   if ($field->getType() != 'entity_reference') {
diff --git a/core/modules/entity_reference/src/ConfigurableEntityReferenceItem.php b/core/modules/entity_reference/src/ConfigurableEntityReferenceItem.php
index 2299351..c464ebf 100644
--- a/core/modules/entity_reference/src/ConfigurableEntityReferenceItem.php
+++ b/core/modules/entity_reference/src/ConfigurableEntityReferenceItem.php
@@ -8,7 +8,6 @@
 namespace Drupal\entity_reference;
 
 use Drupal\Component\Utility\String;
-use Drupal\Core\Field\FieldConfigInterface;
 use Drupal\Core\Field\FieldStorageDefinitionInterface;
 use Drupal\Core\Field\Plugin\Field\FieldType\EntityReferenceItem;
 use Drupal\Core\Form\FormStateInterface;

Minor cleanups on commit.

@Berdir was right for both questions :) Thanks for committing this!

Followup : #2578249: Some e_r fields get the wrong Selection handler