Problem/Motivation

Currently it is impossible to provide schema config for the entity reference selection plugins. The core uses the default handler schema but can't provide schemas for the additional settings defined in the entity type specific entity reference selection plugins.

But, In the UserSelection plugin (users: "default:user") there is an additional setting to filter by role. and the config schema for this can be found in entity_reference.schema.yml instead of inside user.schema.yml.

Field type plugins have storageSettingsToConfigData() and storageSettingsFromConfigData() but we don't have user available in $settings there.

Proposed resolution

The config must contain default:user in this case.

Remaining tasks

Agree if selection plugins use ConfigurablePluginInterface is required to fix this issue or not.

User interface changes

None.

API changes

Entity reference handler settings config schema changes from entity_reference.[%parent.handler].handler_settings to entity_reference_selection.[plugin_id].

SelectionPluginManagerInterface gets a new public method getPluginId($target_type, $base_plugin_id).

FieldItemBase::fieldSettingsToConfigData(array $settings) is changed to fieldSettingsToConfigData(array $settings, FieldConfigInterface $field_config).

Files: 
CommentFileSizeAuthor
#67 interdiff.txt825 bytesamateescu
#67 2436835-67.patch18.77 KBamateescu
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,025 pass(es). View
#65 interdiff.txt471 bytesamateescu
#65 2436835-65.patch18.76 KBamateescu
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,796 pass(es). View
#64 interdiff.txt3.38 KBamateescu
#64 2436835-64.patch18.74 KBamateescu
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,785 pass(es). View
#62 interdiff-to-31.txt9.96 KBamateescu
#62 2436835-62.patch17.63 KBamateescu
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,692 pass(es), 99 fail(s), and 8 exception(s). View
#59 interdiff.txt1.57 KBamateescu
#59 2436835-59.patch35.32 KBamateescu
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,135 pass(es). View
#57 interdiff-to-33.txt5.65 KBamateescu
#57 2436835-57.patch36.05 KBamateescu
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,131 pass(es), 4 fail(s), and 0 exception(s). View
#45 interdiff.txt1.07 KBamateescu
#45 2436835-45.patch16.53 KBamateescu
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,590 pass(es). View
#43 interdiff.txt464 bytesamateescu
#43 2436835-43.patch15.46 KBamateescu
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,760 pass(es), 63 fail(s), and 12 exception(s). View
#35 28-35-interdiff.txt1.61 KBamateescu
#35 2436835-35.patch15.44 KBamateescu
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,177 pass(es). View
#33 2436835.33.patch34.33 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,186 pass(es). View
#33 29-33-interdiff.txt20.56 KBalexpott
#33 2436835.33.patch34.33 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,193 pass(es). View
#31 interdiff-to-30.1.txt4.84 KBamateescu
#31 2436835-31.patch15.33 KBamateescu
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,177 pass(es). View
#30 interdiff.txt2.2 KBamateescu
#30 2436835-29-with-test.patch18.79 KBamateescu
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,187 pass(es), 3 fail(s), and 1 exception(s). View
#30 2436835-28-with-test.patch17.45 KBamateescu
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,184 pass(es). View
#29 2436835.29.patch16.59 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,182 pass(es). View
#28 24-28-interdiff.txt2.52 KBamateescu
#28 2436835-28.patch15.25 KBamateescu
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,293 pass(es). View
#26 2436835.26.patch16.23 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,768 pass(es), 256 fail(s), and 128 exception(s). View
#26 21-26-interdiff.txt1.6 KBalexpott
#24 interdiff-to-9.txt9.22 KBamateescu
#24 2436835-24.patch13.42 KBamateescu
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,301 pass(es), 4 fail(s), and 0 exception(s). View
#21 2436835.21.patch14.4 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#21 19-21-interdiff.txt4.02 KBalexpott
#19 interdiff.txt1.22 KBamateescu
#19 2436835-19.patch10.28 KBamateescu
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,292 pass(es), 1 fail(s), and 2 exception(s). View
#18 2436835.18.patch11.5 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,301 pass(es). View
#18 14-18-interdiff.txt1.71 KBalexpott
#14 2436835.14.patch10.15 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,282 pass(es), 28 fail(s), and 55 exception(s). View
#14 1-14-interdiff.txt7.65 KBalexpott
#9 interdiff.txt3.86 KBamateescu
#9 2436835-6.patch6.15 KBamateescu
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,766 pass(es), 133 fail(s), and 108 exception(s). View
#4 2436835.4.patch3.5 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,288 pass(es), 1 fail(s), and 2 exception(s). View

Comments

Berdir’s picture

Issue summary: View changes
Berdir’s picture

Version: 8.1.x-dev » 8.0.x-dev
Gábor Hojtsy’s picture

alexpott’s picture

Status: Active » Needs review
FileSize
3.5 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,288 pass(es), 1 fail(s), and 2 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, 4: 2436835.4.patch, failed testing.

Anushka-mp’s picture

The change of dynamic keys lead to views requiring the plugin_id. so the same code for plugin_id replicated in the ViewsSelection.php.

Anushka-mp’s picture

Status: Needs work » Needs review
Berdir’s picture

Assigned: Unassigned » amateescu

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.

amateescu’s picture

FileSize
6.15 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,766 pass(es), 133 fail(s), and 108 exception(s). View
3.86 KB

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.

amateescu’s picture

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

amateescu’s picture

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

amateescu’s picture

Status: Needs review » Needs work

The last submitted patch, 9: 2436835-6.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
7.65 KB
10.15 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,282 pass(es), 28 fail(s), and 55 exception(s). View

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.

amateescu’s picture

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.

amateescu’s picture

+++ 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 :)

Status: Needs review » Needs work

The last submitted patch, 14: 2436835.14.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
1.71 KB
11.5 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,301 pass(es). View

Fixing test fails.

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

amateescu’s picture

FileSize
10.28 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,292 pass(es), 1 fail(s), and 2 exception(s). View
1.22 KB

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.

Status: Needs review » Needs work

The last submitted patch, 19: 2436835-19.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
4.02 KB
14.4 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

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.

alexpott’s picture

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

Status: Needs review » Needs work

The last submitted patch, 21: 2436835.21.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review
FileSize
13.42 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,301 pass(es), 4 fail(s), and 0 exception(s). View
9.22 KB

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.

Status: Needs review » Needs work

The last submitted patch, 24: 2436835-24.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
1.6 KB
16.23 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,768 pass(es), 256 fail(s), and 128 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, 26: 2436835.26.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review
FileSize
15.25 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,293 pass(es). View
2.52 KB

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

alexpott’s picture

FileSize
16.59 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,182 pass(es). View

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.

amateescu’s picture

FileSize
17.45 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,184 pass(es). View
18.79 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,187 pass(es), 3 fail(s), and 1 exception(s). View
2.2 KB

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

amateescu’s picture

FileSize
15.33 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,177 pass(es). View
4.84 KB

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.

The last submitted patch, 30: 2436835-29-with-test.patch, failed testing.

alexpott’s picture

FileSize
34.33 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,193 pass(es). View
20.56 KB
34.33 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,186 pass(es). View

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.

Berdir’s picture

  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?

amateescu’s picture

FileSize
15.44 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,177 pass(es). View
1.61 KB

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.

alexpott’s picture

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.

amateescu’s picture

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.

alexpott’s picture

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

amateescu’s picture

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

alexpott’s picture

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

right, config and magic don't like each other

amateescu’s picture

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.

Gábor Hojtsy’s picture

Status: Needs review » Needs work
  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?

amateescu’s picture

Status: Needs work » Needs review
FileSize
15.46 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,760 pass(es), 63 fail(s), and 12 exception(s). View
464 bytes

#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']

Status: Needs review » Needs work

The last submitted patch, 43: 2436835-43.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review
FileSize
16.53 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,590 pass(es). View
1.07 KB
dawehner’s picture

  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?

amateescu’s picture

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

dawehner’s picture

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

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

Berdir’s picture

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

amateescu’s picture

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.

Gábor Hojtsy’s picture

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.

amateescu’s picture

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

Gábor Hojtsy’s picture

Issue summary: View changes
Gábor Hojtsy’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update

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

Berdir’s picture

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.

alexpott’s picture

Status: Needs review » Needs work

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.

amateescu’s picture

Status: Needs work » Needs review
FileSize
36.05 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,131 pass(es), 4 fail(s), and 0 exception(s). View
5.65 KB

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.

Status: Needs review » Needs work

The last submitted patch, 57: 2436835-57.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review
FileSize
35.32 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,135 pass(es). View
1.57 KB

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?

Berdir’s picture

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)

yched’s picture

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

amateescu’s picture

FileSize
17.63 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,692 pass(es), 99 fail(s), and 8 exception(s). View
9.96 KB

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.

Status: Needs review » Needs work

The last submitted patch, 62: 2436835-62.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review
FileSize
18.74 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,785 pass(es). View
3.38 KB

Fixed all that.

amateescu’s picture

FileSize
18.76 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,796 pass(es). View
471 bytes

Re-fixing #42.

jibran’s picture

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?

amateescu’s picture

FileSize
18.77 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,025 pass(es). View
825 bytes

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.

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Ok. Thank you for the fixes.

amateescu’s picture

Assigned: amateescu » alexpott

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

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
  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?

alexpott’s picture

Status: Needs review » Reviewed & tested by the community

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.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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.

  • alexpott committed ae4848f on 8.0.x
    Issue #2436835 by amateescu, alexpott: Unable to create config schema...
amateescu’s picture

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

Status: Fixed » Closed (fixed)

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

yched’s picture