Similar to all kind of different plugins managers widgets and formatters should also use defaultpluginmanager.

Possible followups:

CommentFileSizeAuthor
#9 drupal-2034563-9.patch6.12 KBdawehner
#9 interdiff.txt742 bytesdawehner
#5 drupal-2034563-5.patch6.07 KBdawehner
#5 interdiff.txt927 bytesdawehner
#1 drupal-2034563-1.patch6.07 KBdawehner

Comments

dawehner’s picture

dawehner
Primary language German
commented
Status: Active » Needs review
Issue tags: +Plugin system
StatusFileSize
new drupal-2034563-1.patch6.07 KB

.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Priority: Normal » Major

Release blocker imo

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Title: Switch field plugin managers to defaultpluginmanager » Switch field plugin managers to DefaultPluginManager
berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work 
+++ b/core/modules/field/lib/Drupal/field/Plugin/Type/Widget/WidgetPluginManager.phpundefined
@@ -40,13 +44,21 @@ class WidgetPluginManager extends PluginManagerBase {
     $this->discovery = new ProcessDecorator($this->discovery, array($this, 'processDefinition'));

You don't need this anymore, $this->processDefinition() is called automatically.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt927 bytes
new drupal-2034563-5.patch6.07 KB

Good catch!

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Reviewed & tested by the community

Nice, looks good now!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, drupal-2034563-5.patch, failed testing.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented 
+++ b/core/modules/field/lib/Drupal/field/Plugin/Type/Widget/WidgetPluginManager.phpundefined
@@ -40,13 +44,19 @@ class WidgetPluginManager extends PluginManagerBase {
     $this->factory = new WidgetFactory($this->discovery);

This line is the problem, this needs to pass $this to the factory now.

Wondering if we want to change the widget factory to extend from ContainerFactory/implement the same logic there while we're changing this?

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt742 bytes
new drupal-2034563-9.patch6.12 KB

If we would remove the custom constructor for widgets by for example moving the field definition instance into the settings or definition, we would not need a custom factory at all anymore.

It feels like the simplest thing for now would be to go with that and open new follow ups. (I am not sure whether constructors are a public api).

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Reviewed & tested by the community

Agreed, let's get this in first. Can you open an issue to discuss WidgetFactory? I know that @EclipseGc is also recommending to inline the factory part in your manager when you have specific logic.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 8c19f19 and pushed to 8.x. Thanks!

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

Anonymous’s picture

(not verified) commented
Issue summary: View changes

added follow ups