Updated: Comment 0

Problem/Motivation

This came out of an idea of chx after some plugin discussions.
Currently you can define a plugin using the wrong interface, without anyone stopping you beside of runtime throwing a horrible exception.
If each plugin type knows the interface needed for itself (optional for sure), it can throw a nice help message: this plugin should implement that interface ...

Proposed resolution

Remaining tasks

User interface changes

API changes

Files: 
CommentFileSizeAuthor
#27 factory-2078417-27.patch11.05 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 58,844 pass(es).
[ View ]
#27 interdiff.txt658 bytesdawehner
#18 interdiff.txt749 bytesdawehner
#18 default_factory-2078417-18.patch11.05 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 59,253 pass(es).
[ View ]
#16 default_factory-2078417-16.patch11.06 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 58,404 pass(es).
[ View ]
#16 interdiff.txt2.04 KBdawehner
#14 default_factory-2078417-14.patch11.79 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#14 interdiff.txt1.27 KBdawehner
#12 default_factory-2078417-12.patch11.76 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#12 interdiff.txt4.02 KBdawehner
#11 default_factory-2078417-11.patch11.76 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#11 interdiff.txt2.65 KBdawehner
#10 default_factory-2078417-8.patch11.69 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#10 interdiff.txt9.33 KBdawehner
#5 interdiff.txt5.03 KBdawehner
#5 default_factory-2078417-4.patch7.5 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 58,574 pass(es).
[ View ]
#2 default_factory-2078417-2.patch6.54 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 58,062 pass(es).
[ View ]

Comments

msonnabaum suggests doing this in custom factories for one or a few plugin types and then seeing if it makes sense to provide in a more generic way in the base.

Status:Active» Needs review
StatusFileSize
new6.54 KB
PASSED: [[SimpleTest]]: [MySQL] 58,062 pass(es).
[ View ]

There we go.

Issue tags:+phpunit

.

Status:Needs review» Needs work

Should use an instance variable not a static (class) variable

Status:Needs work» Needs review
StatusFileSize
new7.5 KB
PASSED: [[SimpleTest]]: [MySQL] 58,574 pass(es).
[ View ]
new5.03 KB

Let's use an instance variable instead.

So I think we should name the variables better and always have a default interface set? e.g. Drupal\Component\Plugin\PluginInspectionInterface ?

Though it doesn't seem that we enforce that for all plugins?

Also, I think would be simpler:

  if (!is_subclass_of($plugin_class, $this->interface)) {

Also, can we check interface_exists() in setInterface()?

though I guess the code will break pretty fast if you sent in something invalid.

In these calls so we want/need the leading \ on the class name?

$this->defaultFactory->setInterface('Drupal\plugin_test\Plugin\plugin_test\fruit\FruitInterface');

StatusFileSize
new9.33 KB
new11.69 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
  • Added a method which is used in every extending factory.
  • Renamed the testfile.
  • Throw an exception if an invalid interface is passed.

StatusFileSize
new2.65 KB
new11.76 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Renamed some of the test methods.

StatusFileSize
new4.02 KB
new11.76 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Added starting "\".

I still think we want the leading backslash on class names?

Also, I'd tweak this error message:

'Invalid/non existing interface passed to setInterface'

to something like:
sprintf('Invalid interface name "%s" passed to setInterface()', $interface)

StatusFileSize
new1.27 KB
new11.79 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Oh that is better.

Status:Needs review» Needs work

this change is wrong in ContainerFactory:

-
-    // If the plugin provides a factory method, pass the container to it.
-    if (is_subclass_of($plugin_class, 'Drupal\Core\Plugin\ContainerFactoryPluginInterface')) {
-      return $plugin_class::create(\Drupal::getContainer(), $configuration, $plugin_id, $plugin_definition);
-    }
+    $this->validateInterface($plugin_class, $plugin_id);

should be just adding the call above

+   $this->validateInterface($plugin_class, $plugin_id);
     // If the plugin provides a factory method, pass the container to it.
     if (is_subclass_of($plugin_class, 'Drupal\Core\Plugin\ContainerFactoryPluginInterface')) {

Status:Needs work» Needs review
StatusFileSize
new2.04 KB
new11.06 KB
PASSED: [[SimpleTest]]: [MySQL] 58,404 pass(es).
[ View ]

Oh damn.

extra line break before the closing parens:

throw new PluginException(sprintf('Invalid interface name "%s" passed to setInterface', $interface)
);

StatusFileSize
new11.05 KB
PASSED: [[SimpleTest]]: [MySQL] 59,253 pass(es).
[ View ]
new749 bytes

Ups.

Status:Needs review» Reviewed & tested by the community

Add tests and looks complete. sort of annoying that the WidgetFactory is a special case.

Title:Allow to define an interface for plugin types, so we can throw an helpful exceptionSupport checking in the factory for an expected interface for a plugin type, so we can throw a helpful exception

update title

Status:Reviewed & tested by the community» Needs work

+++ b/core/tests/Drupal/Tests/Component/Plugin/Factory/DefaultFactoryTest.php
@@ -0,0 +1,128 @@
+      array('orange', array('class' => '\Drupal\plugin_test\Plugin\plugin_test\fruit\Orange')),

There is no \Drupal\plugin_test\Plugin\plugin_test\fruit\Orange so here you are still just testing the non existing class.

Status:Needs work» Reviewed & tested by the community

There is no \Drupal\plugin_test\Plugin\plugin_test\fruit\Orange so here you are still just testing the non existing class.

Mh, for me the Orange exists. #1778942: Discovery::getDefinition() / getDefinitions() : inconsistent return values for "no result" added it.

Issue tags:-plugin+Plugin system

fixing tags.

#18: default_factory-2078417-18.patch queued for re-testing.

if that passes, this looks pretty good to me.

Eclipse

+++ b/core/tests/Drupal/Tests/Component/Plugin/Factory/DefaultFactoryTest.php
@@ -0,0 +1,128 @@
+   * Tests the setInterface method with an invalid interface..

Two periods. Other than that RTBC from me.

StatusFileSize
new658 bytes
new11.05 KB
PASSED: [[SimpleTest]]: [MySQL] 58,844 pass(es).
[ View ]

Good catch!

Issue tags:-phpunit, -Plugin system

#27: factory-2078417-27.patch queued for re-testing.

Status:Reviewed & tested by the community» Needs work

The last submitted patch, factory-2078417-27.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+phpunit, +Plugin system

#27: factory-2078417-27.patch queued for re-testing.

Status:Needs review» Reviewed & tested by the community

Still green.

#27: factory-2078417-27.patch queued for re-testing.

Adding tag.

Status:Reviewed & tested by the community» Needs work

The last submitted patch, factory-2078417-27.patch, failed testing.

Status:Needs work» Needs review

#27: factory-2078417-27.patch queued for re-testing.