WidgetFactory is not used anywhere so it should be removed

As of #2035315: Add a dedicated @FieldWidget annotation WidgetFactory is dead code.

Adding a dummy create(), a la EntityFormController, as a guide for anyone that decides to implement ContainerFactoryPluginInterface.

Right, WidgetFactory is not actually used anymore (WidgetPluginManager sets $this->factory = new WidgetFactory, but since it also provides its own createInstance() method, $this->factory is actually never used), and we should have removed the code in #2035315: Add a dedicated @FieldWidget annotation.

Core is moving fast!

+++ b/core/modules/field/lib/Drupal/field/Plugin/Type/Widget/WidgetBase.php
@@ -53,6 +54,15 @@ public function __construct($plugin_id, array $plugin_definition, FieldDefinitio
+  public static function create(ContainerInterface $container, array $configuration, $plugin_id, array $plugin_definition) {

Shouldn't this have a comment block? Apparently it had inheritdoc but probably doesn't make sense anymore

Got lost since my patch. And yes, its still correct.

@@ -56,6 +57,18 @@ public function __construct($plugin_id, array $plugin_definition, FieldDefinitio
+  public static function create(ContainerInterface $container, array $configuration, $plugin_id, array $plugin_definition) {
+    return new static(
+      $plugin_id,
+      $plugin_definition,
+      $configuration['field_definition'],
+      $configuration['settings']
+    );
+  }
+

I don't get that change. Why do we need the create() method on the WidgetBase which does not implement the ContainerFactoryPluginInterface at all? If someone needs it in their own custom widget, all they need is that interface. Formatters are pretty much doing the same.

I'm doing that here because the dividing up of $configuration to satisfy the constructor is confusing, and it provides an example more than anything. It's to help illustrate what you need to do, taking the place of

+++ /dev/null
@@ -1,25 +0,0 @@
-    return new $plugin_class($plugin_id, $plugin_definition, $configuration['field_definition'], $configuration['settings']);

I don't care that much though. We could force the interface here, or just leave this out and make every injection-needing widget figure that weirdness out...

Discussed this further with @dawehner in IRC, this is much cleaner.

Nice! This improves the DX a bit . Do we have to tag that as API change?

Yes, though it is not widespread, and returns the signature to match that of PluginBase, and almost every other plugin. We need to fix FormatterBase next.

Wait, what? What's going on here? Why would every plugin need to match the signature or PluginBase?

At least, let's please give a bit more than three minutes to a field api maintainer to see these changes..

It doesn't have to match. But it is nice to.

Otherwise, every single widget that needs services injected will need to know the magical keys of $configuration['field_definition'] and $configuration['settings'] when implementing ControllerInterface::create(). That's a problem.

Hm.. I thought the plugin system was all about flexibility. Silly me :)

Setting back to NR for Yves.

It was all fine until we needed injection. Passing the burdens of flexibility onto the plugin implementations themselves is not cool.

I would tell you what I'd do with that injection, but this is a public issue queue :)

I'm not sure I like this change either.

Widgets are objects that need a $field_definition and an array of $settings. That's what their current constructor signature expresses.
They happen to be mostly instantiated though the plugin system, but that should not force the shape of their constructor.
Having a "fixed" signature imposed, with all "interesting" arguments (those specific to the plugin type aka the business logic at hand) packed into a $configuration array that the constructor then needs to unpack seems really wrong.

$configuration is Plugin-lingo, it's the format used by FactoryInterface::createInstance() just so that there *can* be a method with a fixed signature in an interface.

I'd much rather have that "unpacking of $configuration" done
- either in the Plugin-related system that calls create() (standalone factory or createInstance() method on the manager), as in HEAD,
- or in the create() method as in the initial iterations of this patch. create() is the interface between the Plugin system and the object, so having some Plugin-related "translation" is fine here.
But it should not bleed further down in the plugin implementation class IMO.

WidgetFactory is actually not used at all:

- WidgetPluginManager::__construct() does specify $this->factory = new WidgetFactory($this);
- but it also overrides DefaultPluginManager::createinstance() with custom code that doesn't use $this->factory (just like FormatterPluginManager does, it's code similar to Containerfactory but tailored for the specific constructors of Widgets). So there is no active code that actually ever does anything with $this->factory.

Patch removes WidgetPluginManager's $this->factory and the WidgetFactory class altogether.

To be more explicit : #23 brings WidgetPluginManager in line with FormatterPluginManager. If we want to change something in this area and have them use ContainerFactory instead of a custom implementation of createInstance(), it should happen on both sides - but as explained in #20, I'm not in favor of doing that

RTBC anyone ?

Sure thing!