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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pwolanin’s picture

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.

dawehner’s picture

Status: Active » Needs review
FileSize
6.54 KB

There we go.

dawehner’s picture

Issue tags: +PHPUnit

.

pwolanin’s picture

Status: Needs review » Needs work

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

dawehner’s picture

Status: Needs work » Needs review
FileSize
7.5 KB
5.03 KB

Let's use an instance variable instead.

pwolanin’s picture

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?

pwolanin’s picture

Also, I think would be simpler:

  if (!is_subclass_of($plugin_class, $this->interface)) {
pwolanin’s picture

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

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

pwolanin’s picture

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

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

  • Added a method which is used in every extending factory.
  • Renamed the testfile.
  • Throw an exception if an invalid interface is passed.
dawehner’s picture

Renamed some of the test methods.

dawehner’s picture

Added starting "\".

pwolanin’s picture

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)
dawehner’s picture

Oh that is better.

pwolanin’s picture

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')) {
dawehner’s picture

Status: Needs work » Needs review
FileSize
2.04 KB
11.06 KB

Oh damn.

pwolanin’s picture

extra line break before the closing parens:

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

dawehner’s picture

Ups.

pwolanin’s picture

Status: Needs review » Reviewed & tested by the community

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

pwolanin’s picture

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

update title

alexpott’s picture

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.

dawehner’s picture

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.

EclipseGc’s picture

Issue tags: -plugin +Plugin system

fixing tags.

pwolanin’s picture

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

EclipseGc’s picture

if that passes, this looks pretty good to me.

Eclipse

fubhy’s picture

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

dawehner’s picture

FileSize
658 bytes
11.05 KB

Good catch!

pwolanin’s picture

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.

tim.plunkett’s picture

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

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

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Still green.

pwolanin’s picture

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

dawehner’s picture

Adding tag.

pwolanin’s picture

Status: Reviewed & tested by the community » Needs work

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

dawehner’s picture

Status: Needs work » Needs review

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

jhedstrom’s picture

Issue summary: View changes
Status: Needs review » Needs work
Issue tags: +Needs reroll
tim.plunkett’s picture

Status: Needs work » Closed (duplicate)
Issue tags: -Needs reroll