We need to be able to inject services from the container into the WidgetFactory. See #2017851: Move entity_reference_get_selection_handler() to a method on SelectionPluginManager for an example of where.

Files: 
CommentFileSizeAuthor
#23 2052751_WidgetFactory-23.patch1.47 KByched
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,651 pass(es).
[ View ]
#11 widget-2052751-11.patch7.07 KBtim.plunkett
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch widget-2052751-11.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#11 interdiff.txt5.98 KBtim.plunkett
#8 widget-2052751-8.patch2.92 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,081 pass(es).
[ View ]
#6 widget-2052751-6.patch2.74 KBoriol_e9g
PASSED: [[SimpleTest]]: [MySQL] 57,996 pass(es).
[ View ]
#4 widget-2052751-4.patch2.7 KBoriol_e9g
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch widget-2052751-4.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#1 widget-2052751-1.patch2.88 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 57,300 pass(es).
[ View ]

Comments

tim.plunkett’s picture

Status:Active» Needs review
StatusFileSize
new2.88 KB
PASSED: [[SimpleTest]]: [MySQL] 57,300 pass(es).
[ View ]

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.

yched’s picture

Status:Needs review» Reviewed & tested by the community

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.

alexpott’s picture

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

Needs a reroll

git ac https://drupal.org/files/widget-2052751-1.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  2952  100  2952    0     0    787      0  0:00:03  0:00:03 --:--:--   847
error: patch failed: core/modules/field/lib/Drupal/field/Plugin/Type/Widget/WidgetPluginManager.php:50
error: core/modules/field/lib/Drupal/field/Plugin/Type/Widget/WidgetPluginManager.php: patch does not apply
oriol_e9g’s picture

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new2.7 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch widget-2052751-4.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Status:Needs review» Needs work

The last submitted patch, widget-2052751-4.patch, failed testing.

oriol_e9g’s picture

Status:Needs work» Needs review
StatusFileSize
new2.74 KB
PASSED: [[SimpleTest]]: [MySQL] 57,996 pass(es).
[ View ]

Core is moving fast!

pcambra’s picture

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

tim.plunkett’s picture

StatusFileSize
new2.92 KB
PASSED: [[SimpleTest]]: [MySQL] 58,081 pass(es).
[ View ]

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

dawehner’s picture

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

tim.plunkett’s picture

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

tim.plunkett’s picture

StatusFileSize
new5.98 KB
new7.07 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch widget-2052751-11.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

dawehner’s picture

Status:Needs review» Reviewed & tested by the community

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

tim.plunkett’s picture

Issue tags:+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.

amateescu’s picture

Assigned:Unassigned» yched
Status:Reviewed & tested by the community» Needs review

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

Status:Needs review» Needs work

The last submitted patch, widget-2052751-11.patch, failed testing.

tim.plunkett’s picture

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.

amateescu’s picture

Status:Needs work» Needs review

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

Setting back to NR for Yves.

tim.plunkett’s picture

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

amateescu’s picture

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

yched’s picture

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.

Berdir queued 11: widget-2052751-11.patch for re-testing.

Status:Needs review» Needs work

The last submitted patch, 11: widget-2052751-11.patch, failed testing.

yched’s picture

Assigned:yched» Unassigned
Issue summary:View changes
Status:Needs work» Needs review
StatusFileSize
new1.47 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,651 pass(es).
[ View ]

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.

yched’s picture

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

yched’s picture

RTBC anyone ?

amateescu’s picture

Title:WidgetFactory should extend or be replaced by ContainerFactory» WidgetFactory is not used anywhere so it should be removed
Status:Needs review» Reviewed & tested by the community

Sure thing!

alexpott’s picture

Category:Task» Bug report
Status:Reviewed & tested by the community» Fixed

Dead code is bug.

Committed 87cb9a6 and pushed to 8.0.x. Thanks!

alexpott’s picture

Issue tags:-API change

And this turned out to not be an API change

  • alexpott committed 87cb9a6 on 8.0.x
    Issue #2052751 by tim.plunkett, oriol_e9g, yched | alexpott: Fixed...

Status:Fixed» Closed (fixed)

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