Problem/Motivation

After visiting @alexpott's session "Drupal 9 is coming: Getting your code ready
" (https://events.drupal.org/seattle2019/sessions/drupal-9-coming-getting-y...) in Seattle, I wanted to make the contrib select2 module more BC stable by preventing constructor overwrites.

I found out that I am not able to prevent constructor overwriting for the Select2EntityReferenceWidget (https://git.drupalcode.org/project/select2/blob/8.x-1.x/src/Plugin/Field...) because none of it's base classes is implementing the ContainerFactoryPluginInterface.
The base class chain of this class is Select2EntityReferenceWidget -> Select2Widget -> OptionsSelectWidget -> OptionsWidgetBase -> WidgetBase. That means if one of this base classes changes it's constructor, I will have a breakage in Select2EntityReferenceWidget.

Proposed resolution

Add ContainerFactoryPluginInterface to Drupal\Core\Field\WidgetBase and Drupal\Core\Field\FormatterBase and implement the ::create() method in them. The big advantage is that all child classes will be more BC stable.

Remaining tasks

Discuss
Implement
Test
Commit

User interface changes

n.a.

API changes

Add ContainerFactoryPluginInterface to Drupal\Core\Field\WidgetBase

Data model changes

n.a.

Release notes snippet

n.a.

CommentFileSizeAuthor
#7 interdiff-3051633-2-7.txt1.4 KBchr.fritsch
#7 3051633-7.patch2.82 KBchr.fritsch
#2 3051633-2.patch1.43 KBchr.fritsch
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chr.fritsch created an issue. See original summary.

chr.fritsch’s picture

chr.fritsch
he/him
Primary language German
Location 🇩🇪🇪🇺🌍
CreditAttribution: chr.fritsch at Thunder commented
Issue summary: View changes
Status: Active » Needs review
FileSize
3051633-2.patch1.43 KB

Here is a first patch.

We could also look at all the child classes and clean up their constructors by using the ::create() method correctly.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented

@chr.fritsch it is okay in core to override a constructor - after all we can change the lot when something changes. But I think that what this issue is doing - making the deepest base class implement ::create is a good idea because it allows contrib and custom to extend any of core's concrete implementations safe in the knowledge that a core change is not going to break them because constructors of concrete plugins are not covered by our BC promise whereas the ::create() method is.

daniel.bosen’s picture

daniel.bosen
he/him
CreditAttribution: daniel.bosen at Thunder commented

+1 for this, WidgetBase and its descendants are overwritten a lot in contrib and custom code.

@alexpott I get that it is - in theory - OK for core to override constructors. But core code should be an important source for best practices, and it would be very helpful, if core overrides would also do the things you talked about in your session. I think the main reason for custom code to be overriding the constructor is, that people looked at how core does it...

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented

Was surprised that this even works but we did add explicit support for this interface in the custom createInstance() implementation in \Drupal\Core\Field\WidgetPluginManager::createInstance().

One reason to make sure of that would be to inject the dependencies that the class is using her, like \Drupal::token() and module handler.

Also, reminder for #3030640: [policy, documentation] Clarify Constructor BC policy and document best practices for subclassing other classes, we don't really have an documentation or policy on how to use that DI approach that alex talked about in his presentation ;)

hchonov’s picture

hchonov
Primary language Bulgarian
Location 🇪🇺🇩🇪🇧🇬
CreditAttribution: hchonov as a volunteer and at bio.logis Genetic Information Management GmbH commented

+1, but if we do it for widgets it would be good to be consistent and do it for field formatters as well :)

chr.fritsch’s picture

chr.fritsch
he/him
Primary language German
Location 🇩🇪🇪🇺🌍
CreditAttribution: chr.fritsch at Thunder commented
Title: Add ContainerFactoryPluginInterface to Drupal\Core\Field\WidgetBase » Add ContainerFactoryPluginInterface to Drupal\Core\Field\WidgetBase and Drupal\Core\Field\FormatterBase
Issue summary: View changes
FileSize
3051633-7.patch2.82 KB
interdiff-3051633-2-7.txt1.4 KB

So here is a new patch that also implements the interface in the formatter base class.

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented

The patch looks good to me, thanks for updating FormatterBase as well :)

How about @Berdir's feedback from #5? Should we inject the module_handler and token services in this issue? We would need to do that in a BC-compatible way, otherwise all the widget classes that override the constructor would break. Alternatively, we could use setter injection for those two services in the base class...

chr.fritsch’s picture

chr.fritsch
he/him
Primary language German
Location 🇩🇪🇪🇺🌍
CreditAttribution: chr.fritsch at Thunder commented

Hm,

adding module_handler and token to the constructor of WidgetBase feels a bit awkward. I checked all child classes and none of them is using these two services.
So we could inject these services to WidgetBase by setter injection like:

  public static function create(ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition) {
    $widget = new static($plugin_id, $plugin_definition, $configuration['field_definition'], $configuration['settings'], $configuration['third_party_settings']);
    $widget->setModuleHandler($container->get('module_handler'));
    $widget->setToken($container->get('token'));
    return $widget;
  }

The downside is we'll break all child classes, which already have their own ::create() method.
In order to fix that, I would propose to introduce two private methods for getting services like:

  /**
   * @deprecated 
   */
  private function getModuleHandler() {
    if(isset($this->moduleHandler)) {
      return $this->moduleHandler;
    }
    trigger_error("Dont use it");
    return $this->moduleHandler = \Drupal::moduleHandler();
  }

We could use that function in WidgetBase and we adopt all child classes, with an implemented ::create() method, to call $widget = parent:static().

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented

I think we should tackle inject of the services the class is already using in a follow-up. Adding a create doesn't make it easier for core to add or change dependencies - it makes it easier for contrib and custom.

chr.fritsch’s picture

chr.fritsch
he/him
Primary language German
Location 🇩🇪🇪🇺🌍
CreditAttribution: chr.fritsch at Thunder commented
Related issues: +#3059023: [PP-1] Use proper DI for all used services in WidgetBase and FormatterBase

Here is the follow-up #3059023: [PP-1] Use proper DI for all used services in WidgetBase and FormatterBase

chr.fritsch’s picture

chr.fritsch
he/him
Primary language German
Location 🇩🇪🇪🇺🌍
CreditAttribution: chr.fritsch at Thunder commented

I also created a CR: https://www.drupal.org/node/3059039

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented
Status: Needs review » Reviewed & tested by the community

Ok, I don't mind tackling the existing service usages in a followup :)

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed fcb657b and pushed to 8.8.x. Thanks!

  • catch committed fcb657b on 8.8.x 
    Issue #3051633 by chr.fritsch, alexpott, amateescu, daniel.bosen, Berdir...

Status: Fixed » Closed (fixed)

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

quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented

Publish the change record