Problem/Motivation

This is a prerequisite for #2786841: Entity reference fields should add selection handler config dependencies and probably for #2776571: EntityAutocomplete should pass the original URI to the selection handler.

  • DefaultSelection and ViewsSelection are duplicating a lot of code: protected properties, constructor, create() method.
  • Selection handlers should have setters and getters for their configuration.
  • Selection handlers should declare their handler default settings in a consistent way in order to get sane defaults. Right now DefaultSelection hardcodes this, see ::buildConfigurationForm(). Default configurations should be sane automatically.
  • Anticipating #2786841: Entity reference fields should add selection handler config dependencies, selection handlers should implement calculateDependencies() even with no effect right now.

Proposed resolution

  1. Introduce a new base class for selection handlers - SelectionPluginBase:
    abstract class SelectionPluginBase extends PluginBase implements SelectionInterface, ConfigurablePluginInterface {
      ...
    }
    
  2. Introduce a trait SelectionTrait to provide shared code between DefaultSelection and ViewsSelection:
    trait SelectionTrait {
      ...
    }
    
  3. Make DefaultSelection and ViewsSelection extend the new SelectionPluginBase class and use SelectionTrait.
  4. Make Broken selection handler extend SelectionPluginBase .
  5. Move the common code from DefaultSelection and ViewsSelection into SelectionTrait.
  6. In SelectionPluginBase provide base implementations for: defaultConfiguration(), setConfiguration(), getConfiguration(), calculateDependencies(), buildConfigurationForm(), validateConfigurationForm(), submitConfigurationForm() and entityQueryAlter().
  7. SelectionPluginBase::setConfiguration() will take care to assure BC by creating a handler_settings level, same as the field config has. Basically all handler settings will live in the root level but will be synced also under handler_settings. Add a @todo to remove the BC handler_settings key in Drupal 9.0.x.
  8. Implement ::defaultConfiguration() in all core selection handlers that need it.
  9. Cleanup code from DefaultSelection, UserSelection and ViewsSelection that assures sane-defaults because now the configuration is already sane.
  10. Remove empty instances of buildConfigurationForm(), validateConfigurationForm(), submitConfigurationForm() and entityQueryAlter() from implementations because we already provide them now in the base class.

Remaining tasks

Continue with #2786841: Entity reference fields should add selection handler config dependencies.

User interface changes

None.

API changes

This change will not introduce any BC break. All custom selectors based on DefaultSelection or ViewsSelection will continue to works as we provide base implementations for all new methods. Custom selections not extending DefaultSelection or ViewsSelection but implementing directly SelectionInterface will not be affected at all.

API changes:

  • New base class for selection handlers SelectionPluginBase:
    abstract class SelectionPluginBase extends PluginBase implements SelectionInterface, ConfigurablePluginInterface {
      ...
    }
    
  • New trait SelectionTrait:
    trait SelectionTrait {
      ...
    }
    

Data model changes

None.

CommentFileSizeAuthor
#140 add_a_base_class_for-2787873-140.patch54.6 KBjibran
#140 interdiff-a595c9.txt2.31 KBjibran
#132 add_a_base_class_for-2787873-132.patch54.6 KBjibran
#132 interdiff-efa229.txt915 bytesjibran
#130 add_a_base_class_for-2787873-130.patch54.33 KBjibran
#130 interdiff-c67a85.txt1.74 KBjibran
#130 interdiff-c67a85.txt1.18 KBjibran
#113 2787873-113.interdiff.txt1.1 KBclaudiu.cristea
#113 2787873-113.patch53.94 KBclaudiu.cristea
#110 2787873-110.patch53.94 KBclaudiu.cristea
#110 2787873-110.interdiff.txt3.47 KBclaudiu.cristea
#107 interdiff-105-to-107.txt3.13 KBclaudiu.cristea
#107 2787873-107.patch55.72 KBclaudiu.cristea
#105 interdiff-99-to-105.txt1.98 KBclaudiu.cristea
#105 2787873-105.patch55.78 KBclaudiu.cristea
#99 2787873-99.patch55.63 KBclaudiu.cristea
#96 2787873-96.patch56.24 KBclaudiu.cristea
#94 2787873-94.patch56.19 KBclaudiu.cristea
#94 interdiff-90-to-94.txt2.84 KBclaudiu.cristea
#90 2787873-90.patch56.15 KBclaudiu.cristea
#80 interdiff_60_to_76.txt11.44 KBclaudiu.cristea
#76 2787873-75.patch56.1 KBclaudiu.cristea
#76 interdiff.txt8.05 KBclaudiu.cristea
#71 interdiff.txt3.94 KBclaudiu.cristea
#71 2787873-71.patch55.67 KBclaudiu.cristea
#70 2787873-70.patch55.84 KBclaudiu.cristea
#70 interdiff.txt3.33 KBclaudiu.cristea
#69 interdiff.txt9.91 KBclaudiu.cristea
#69 2787873-69.patch54.8 KBclaudiu.cristea
#60 interdiff.txt759 bytesclaudiu.cristea
#60 2787873-60.patch51.8 KBclaudiu.cristea
#58 interdiff.txt963 bytesclaudiu.cristea
#58 2787873-58.patch51.81 KBclaudiu.cristea
#57 interdiff.txt6.78 KBclaudiu.cristea
#57 2787873-57.patch51.83 KBclaudiu.cristea
#54 interdiff.txt7.47 KBclaudiu.cristea
#54 2787873-54.patch51.42 KBclaudiu.cristea
#52 interdiff.txt949 bytesclaudiu.cristea
#52 2787873-52.patch49.09 KBclaudiu.cristea
#50 interdiff.txt984 bytesclaudiu.cristea
#50 2787873-50.patch49.11 KBclaudiu.cristea
#45 interdiff.txt4.26 KBclaudiu.cristea
#45 2787873-45.patch48.6 KBclaudiu.cristea
#44 interdiff.txt3.03 KBclaudiu.cristea
#44 2787873-44.patch45.9 KBclaudiu.cristea
#40 interdiff.txt26.06 KBclaudiu.cristea
#40 2787873-40.patch44.75 KBclaudiu.cristea
#36 2787873-36.patch26.28 KBclaudiu.cristea
#36 interdiff.txt3.93 KBclaudiu.cristea
#34 2787873-34.patch25.32 KBclaudiu.cristea
#34 interdiff.txt3.64 KBclaudiu.cristea
#27 2787873-27.patch25.35 KBclaudiu.cristea
#27 interdiff.txt700 bytesclaudiu.cristea
#25 interdiff.txt9.73 KBclaudiu.cristea
#25 2787873-25.patch25.24 KBclaudiu.cristea
#24 interdiff.txt9.41 KBclaudiu.cristea
#24 2787873-24.patch23.84 KBclaudiu.cristea
#19 interdiff.txt3.57 KBclaudiu.cristea
#19 2787873-19.patch17.27 KBclaudiu.cristea
#18 2787873-9.patch13.7 KBclaudiu.cristea
#18 interdiff.txt3.57 KBclaudiu.cristea
#9 2787873-9.patch13.7 KBclaudiu.cristea
#9 interdiff.txt4.03 KBclaudiu.cristea
#2 2787873-2.patch15.09 KBclaudiu.cristea
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

claudiu.cristea created an issue. See original summary.

claudiu.cristea’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
15.09 KB

.

claudiu.cristea’s picture

Issue summary: View changes

.

claudiu.cristea’s picture

Issue summary: View changes

.

Status: Needs review » Needs work

The last submitted patch, 2: 2787873-2.patch, failed testing.

claudiu.cristea’s picture

Both tests passed in #2. I cannot explain why this was moved to NW. Back to Needs Review.

claudiu.cristea’s picture

Status: Needs work » Needs review

.

amateescu’s picture

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?

claudiu.cristea’s picture

FileSize
4.03 KB
13.7 KB

@amateescu, thank you for review.

#8.1.

I don't think the generic plugin system should know or care about our (Drupal's) config dependency mechanism

Well, we are already doing this in other places of core. For example EntityDisplayBase (which is a config entity) collects dependencies from components (formatters, widgets). Then, in onDependencyRemoval(), asks each component for its own onDependencyRemoval(). In this way plugins are able to react themselves on a dependency removal for the dependencies that they were introduced.

I agree that this seems out of scope but I wanted to prepare #2786841: Entity reference fields should add selection handler config dependencies. I'm dropping now that interface and I will add it later, in #2786841: Entity reference fields should add selection handler config dependencies.

#8.2. Done.

#8.3. Hm. My understanding is that an implementation overrides the parent's settings. I agree that we don't have in this patch a case where a setting overlaps but I still believe that this is correct. I changed also the other implementations.

claudiu.cristea’s picture

Issue summary: View changes

Updating the IS to reflect #8 and #9.

claudiu.cristea’s picture

Issue tags: +code cleanup, +API clean-up
amateescu’s picture

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

claudiu.cristea’s picture

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

Hm... it's not a feature request. I don't know the policy.

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.

OK, let's discuss this later, in the other issue.

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

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

amateescu’s picture

Status: Reviewed & tested by the community » Needs review

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

claudiu.cristea’s picture

@amateescu, Oh, I missed that. Well, this is fixing that too but, yes, I forgot to verify the schema. I'll do this right now. Good point!

claudiu.cristea’s picture

claudiu.cristea’s picture

FileSize
3.57 KB
13.7 KB

OK. I fixed all core schemas. Now they are following the classes inheritance. I also adjusted their labels and docs (where case). We should be covered now but let's see the tests.

claudiu.cristea’s picture

FileSize
17.27 KB
3.57 KB

Ouch! The patch was wrong.

jibran’s picture

Status: Needs review » Needs work

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

claudiu.cristea’s picture

Status: Needs work » Needs review

@jibran, no, sorry, this patch is not changing the schema at all. It only reorganises the schema definitions to follow the class hierarchy but, at the end, all existing field handler settings are kept exactly as they are. Can you point me to the diff where you feel that we changed the schema?

EDIT: Also I'm keeping the same schema selectors, so if a contrib extended the existing schema, it will get exactly the same schema in those selectors.

dawehner’s picture

  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?

claudiu.cristea’s picture

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

They are used in both main (1st tier) implementations: DefaultSelection & ViewsSelection, so it's about code sharing.

claudiu.cristea’s picture

FileSize
23.84 KB
9.41 KB

Yes, it make sense to add also the configuration form methods in SelectionPluginBase. In this way a custom selector that needs no configuration form (it's likely that such cases exists) will not be forced to implement ::buildConfigurationForm(). Not to add that this removes a lot of code duplication.

Also ::entityQueryAlter() is not so much used so added that too to the base class.

claudiu.cristea’s picture

FileSize
25.24 KB
9.73 KB

Addressing #22. Indeed the injected services are not used in all child classes - for example Broken or a custom selector doesn't need them. Discussed with @dawehner on IRC to provide a patch where we have an intermediary base class that extends SelectionPluginBase and is dedicated only to DefaultSelector & ViewsSelector, supplying them with the injected services. Course, a custom selector that needs all those services, can extend that (BasicSelectionPluginBase). Others can extend directly from SelectionPluginBase.

dawehner’s picture

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

claudiu.cristea’s picture

FileSize
700 bytes
25.35 KB

Thank you, @dawehner.

Addressing #26.

claudiu.cristea’s picture

Issue summary: View changes

Updated IS to reflect #25.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Cool, thank you

amateescu’s picture

  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.

dawehner’s picture

Status: Reviewed & tested by the community » Needs work

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!

claudiu.cristea’s picture

@amateescu

We're losing this @todo.

Hm. That's not actually a @todo, it contains no "call to action". To do what? Not a big deal to keep it but I felt it has no sense. What we expect from TermSelection::entityQueryAlter() and cannot be done so that we need to add a @todo?

amateescu’s picture

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

claudiu.cristea’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
3.64 KB
25.32 KB

OK, switched to trait.

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

@amateescu,

Unless I'm not reading correctly the code, when looking at D7 EntityReference_SelectionHandler_Generic_taxonomy_term::entityFieldQueryAlter(), I see that, in D8, the access tag is already added in DefaultSelection::buildEntityQuery(), in the next piece of code:

    // Add entity-access tag.
    $query->addTag($target_type . '_access');

This is the reason why NodeSelection is missing too the entityQueryAlter() implementation.

Correct me if I'm wrong, I'll add the @todo back.

amateescu’s picture

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

claudiu.cristea’s picture

FileSize
3.93 KB
26.28 KB

Thank you @amateescu,

#35 1, 2, 4-7: Fixed.

#35.3: That is implementing DependentPluginInterface (via ConfigurablePluginInterface).

amateescu’s picture

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

claudiu.cristea’s picture

@amateescu, I understand now but I (partialy) disagree. The plugin should know for what target entity type he was instantiated. That piece of information is missed. #2786841: Entity reference fields should add selection handler config dependencies (following this issue) is an example why we need that information. So, in my understanding target_type is part of plugin configuration.

Alternatively, we can pass only handler_settings to plugin configuration but in that case we need to store the target_type as protected property and assure a getter & a setter (interface change!). We need that target_type.

claudiu.cristea’s picture

Quoting from #2636322-9: Entity reference selection plugins cannot be instantiated without configuration:

And in order to fix this in a backwards compatible way, we should also duplicate the passed-in configuration in a 'handler_settings' sub-key, so that contrib or custom selection plugins don't break after this change.

And this doesn't look so good. The config structure will be confusing. What if a custom handler asks for a top-level target_type after this issue is committed? This means we need to copy also target_type in top-level as you are suggesting for handler_settings, in order to not break the BC.

Having the top-level keys target_type & handler_settings is the reason why I introduced ::defaultHandlerSettings(). Plugin Configuration and Handler Settings are two different structures even the second is contained in the first.

claudiu.cristea’s picture

Version: 8.2.x-dev » 8.3.x-dev
FileSize
44.75 KB
26.06 KB

OK, implemented the new plugin config structure.

claudiu.cristea’s picture

Issue summary: View changes
claudiu.cristea’s picture

Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, 40: 2787873-40.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
45.9 KB
3.03 KB

So, when handler_settings are copied over root level, the operations should NOT be array merge deep because we have to totally override the root values, not merge them.

I left a test case, where an option is passed in BC mode, not converted just to prove that the value takes precedence.

claudiu.cristea’s picture

FileSize
48.6 KB
4.26 KB

Add a unit test to check how plugin config is resolving.

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

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

catch’s picture

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

claudiu.cristea’s picture

Status: Needs review » Reviewed & tested by the community

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

We cannot provide an update path because we are talking here about plugin config (runtime) and that is not stored. We don't change the field settings -- that would have been something stored. We provide this BC layer for the case some existing code is instantiating the plugin with the old structured configuration.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 45: 2787873-45.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
49.11 KB
984 bytes
amateescu’s picture

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

claudiu.cristea’s picture

Status: Needs review » Needs work

The last submitted patch, 52: 2787873-52.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
51.42 KB
7.47 KB

We were lucky with #2374301: Cannot predict the order of results in taxonomy autocompletion because this spotted a bug in our patch.

Changes:

  • It's not correct to NestedArray::mergeDeep() in ::setConfiguration() because if someone is setting an array value with ::setConfiguration() it expects to retrieve the same value, not a merged one.
  • A config under BC levelhandler_settings should not override the same config stored on top level because if someone stored a config in top level, we assume that he/she is aware about the new API.
  • We should enforce developers not to use the BC layer handler_settings in ::defaultConfiguration() by throwing an exception in ::setConfiguration().
  • Enrich EntityReferenceSelectionUnitTest to account all these changes.
amateescu’s picture

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

Status: Needs review » Needs work

The last submitted patch, 54: 2787873-54.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
51.83 KB
6.78 KB

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

@amateescu++

Looks so nice, now :)

Thinking more, I had to revert back to merge deep when applying defaults. This is because of buildConfigurationForm(), where due to Ajax requests, it receives only a part of configuration. For example the form posts the sort > field but not sort > direction. Then, when saving the configuration, he should be able to add the missing part from defaults.

claudiu.cristea’s picture

FileSize
51.81 KB
963 bytes

Coding standards.

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

claudiu.cristea’s picture

FileSize
51.8 KB
759 bytes

Small docs fix.

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

Looks great now!

jibran’s picture

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.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 60: 2787873-60.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Reviewed & tested by the community

CI error. Back to RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 60: 2787873-60.patch, failed testing.

claudiu.cristea’s picture

Again CI error. Back to RTBC as per #61.

The last submitted patch, 60: 2787873-60.patch, failed testing.

catch’s picture

+++ 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’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
54.8 KB
9.91 KB

@catch, I got your point and I like the idea. Unfortunately I cannot externalize in a single protected method, so probably will not look 100% clean. Is this what you wanted? I'm setting back to RTBC to get @catch feedback on his requested change.

claudiu.cristea’s picture

FileSize
3.33 KB
55.84 KB

Testing also that the deprecation message is not triggered.

claudiu.cristea’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
55.67 KB
3.94 KB

More polishing.

@amateescu, I think the changes since #68 (see @catch request) need a review. Setting back to NR.

catch’s picture

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

claudiu.cristea’s picture

@catch, yes, I was thinking that we probably need a trait that can be reused.

amateescu’s picture

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

claudiu.cristea’s picture

@catch, I can't find an issue for error assertions. I added #2834021: Provide error test assertions via a trait.

claudiu.cristea’s picture

@amateescu, thank you.

Used 'ensure' and renamed. This looks much more better.

jibran’s picture

Status: Needs review » Reviewed & tested by the community

I think this is ready.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 76: 2787873-75.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Reviewed & tested by the community
claudiu.cristea’s picture

FileSize
11.44 KB

I'm adding an interdiff to previous RTBC version for a better review of changes requested by @catch in #68.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 76: 2787873-75.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 76: 2787873-75.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 76: 2787873-75.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 76: 2787873-75.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 76: 2787873-75.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
56.15 KB

Reroll.

@catch, in #80 there's a relevant diff for the changes you required in #68.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 90: 2787873-90.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Reviewed & tested by the community

Unrelated failures.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

claudiu.cristea’s picture

Only removed the reference to the current Drupal version.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 94: 2787873-94.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
56.24 KB

Reroll.

amateescu’s picture

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

Improved the title a bit :)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 96: 2787873-96.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
55.63 KB

Reroll because array() > []

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 99: 2787873-99.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 99: 2787873-99.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Reviewed & tested by the community
alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record

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.

claudiu.cristea’s picture

Status: Needs work » Needs review
Issue tags: -Needs change record
FileSize
55.78 KB
1.98 KB

Added CR https://www.drupal.org/node/2870971 and linked in code docs.

Status: Needs review » Needs work

The last submitted patch, 105: 2787873-105.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
55.72 KB
3.13 KB

Fixed the unit test left after deprecation message has been changed.

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

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

claudiu.cristea’s picture

Status: Reviewed & tested by the community » Needs work
claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
3.47 KB
53.94 KB

The deprecation error is triggered in several places, so I added the @group legacy tag to the class.

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Thanks @claudiu.cristea

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 110: 2787873-110.patch, failed testing.

claudiu.cristea’s picture

It doesn't like quotes?

jibran’s picture

Status: Needs review » Reviewed & tested by the community

It is green again so back to RTBC.

catch’s picture

Status: Reviewed & tested by the community » Fixed

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

  • catch committed b0900c7 on 8.4.x
    Issue #2787873 by claudiu.cristea, amateescu, jibran, dawehner, catch:...
jibran’s picture

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

catch’s picture

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.

jibran’s picture

Thank you @catch for clearing that up.

Berdir’s picture

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?

xjm’s picture

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

cilefen’s picture

  • cilefen committed d5d37e0 on 8.4.x
    Revert "Issue #2787873 by claudiu.cristea, amateescu, jibran, dawehner,...
cilefen’s picture

Status: Fixed » Needs work

#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
xjm’s picture

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.

vaplas’s picture

The problem is detected with 5.5.9, and Drupal 8 system requirements point to this version https://www.drupal.org/docs/8/system-requirements/web-server. But testing is performed with the version php-5.5.38 (https://dispatcher.drupalci.org/job/php5.5_mysql5.5/5091/parameters/). It seems this was done to reduce the number of random fails.

xjm’s picture

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.

jibran’s picture

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

jibran’s picture

claudiu.cristea’s picture

+++ b/core/lib/Drupal/Core/Entity/Plugin/EntityReferenceSelection/DefaultSelection.php
@@ -38,88 +36,47 @@
+  public function __construct(array $configuration, $plugin_id, $plugin_definition, EntityManagerInterface $entity_manager, ModuleHandlerInterface $module_handler, AccountInterface $current_user) {
+    $this->selectionTraitConstruct($configuration, $plugin_id, $plugin_definition, $entity_manager, $module_handler, $current_user);
   }

Let's add a comment to explain why we're doing this.

jibran’s picture

claudiu.cristea’s picture

Status: Needs review » Reviewed & tested by the community

Looks good.

The entire issue was reviewed and committed once, in #116, I'm reviewing only #130 and #132, so I'm able to RTBC (if gets green).

larowlan’s picture

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

larowlan’s picture

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

larowlan’s picture

For the record we have 119 traits in core.

None of them have constructors

claudiu.cristea’s picture

Are we gonna rework this issue? This was committed once after was in RTBC state from Dublin Con 2016 (september 2016) until late May 2017 (!!!) Such a complex work should not have been reverted, but the PHP 5.5.9 issue should have live in its own follow-up. Very disappointed :(

However:

#134.1: Numeric keys in the 1st level of the array is a malformed configuration. Probably that should be treated distinctively as check and throw an exception if somebody wants to set such setting. But it's not in the scope of this issue, shouldn't we create a followup to deny numeric keys in the 1st tier?

#134.2: Initially the code from trait was placed in a base class but we moved to trait, see #30, as is only code reusing. So, we are only reusing code here, isn't this the purpose of traits? I don't understand where's the DX loss? If an implementation wants another constructor, it will use the trait and extend/override the trait implementation.

vaplas’s picture

I could not get the fall for php 5.5.9.

Environment (OpenServer):

  • Windows 10-x64
  • Apache-2.4-x64
  • MySQL-5.7-x64

Steps:

  1. apply patch #113 (for last 8.4.x, and for two month ago version too)
  2. get this php version http://windows.php.net/downloads/releases/archives/php-5.5.9-Win32-VC11-...
  3. set PhpStorm CLI interpreter with this version and php.ini with the necessary extensions
  4. go to Drupal\Tests\node\Functional\NodeCreationTest and add code to setUp():
  5. // To make sure that php 5.5.9 is running
    $this->assertEquals('5.5.9', phpversion());
  6. run NodeCreationTest, all tests passed.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

jibran’s picture

claudiu.cristea’s picture

Status: Needs review » Reviewed & tested by the community

Thank you.

  • catch committed 7d1c57a on 8.5.x
    Issue #2787873 by claudiu.cristea, jibran, amateescu, catch, dawehner,...
catch’s picture

Version: 8.5.x-dev » 8.4.x-dev
Status: Reviewed & tested by the community » Fixed

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

  • catch committed 9573bf6 on 8.4.x
    Issue #2787873 by claudiu.cristea, jibran, amateescu, catch, dawehner,...