Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Related Issues
Comment | File | Size | Author |
---|---|---|---|
#27 | factory-2078417-27.patch | 11.05 KB | dawehner |
#27 | interdiff.txt | 658 bytes | dawehner |
#18 | interdiff.txt | 749 bytes | dawehner |
#18 | default_factory-2078417-18.patch | 11.05 KB | dawehner |
#16 | default_factory-2078417-16.patch | 11.06 KB | dawehner |
Comments
Comment #1
pwolanin CreditAttribution: pwolanin commentedmsonnabaum 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.
Comment #2
dawehnerThere we go.
Comment #3
dawehner.
Comment #4
pwolanin CreditAttribution: pwolanin commentedShould use an instance variable not a static (class) variable
Comment #5
dawehnerLet's use an instance variable instead.
Comment #6
pwolanin CreditAttribution: pwolanin commentedSo 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?
Comment #7
pwolanin CreditAttribution: pwolanin commentedAlso, I think would be simpler:
Comment #8
pwolanin CreditAttribution: pwolanin commentedAlso, can we check interface_exists() in setInterface()?
though I guess the code will break pretty fast if you sent in something invalid.
Comment #9
pwolanin CreditAttribution: pwolanin commentedIn these calls so we want/need the leading \ on the class name?
Comment #10
dawehnerComment #11
dawehnerRenamed some of the test methods.
Comment #12
dawehnerAdded starting "\".
Comment #13
pwolanin CreditAttribution: pwolanin commentedI still think we want the leading backslash on class names?
Also, I'd tweak this error message:
to something like:
Comment #14
dawehnerOh that is better.
Comment #15
pwolanin CreditAttribution: pwolanin commentedthis change is wrong in ContainerFactory:
should be just adding the call above
Comment #16
dawehnerOh damn.
Comment #17
pwolanin CreditAttribution: pwolanin commentedextra line break before the closing parens:
throw new PluginException(sprintf('Invalid interface name "%s" passed to setInterface', $interface)
);
Comment #18
dawehnerUps.
Comment #19
pwolanin CreditAttribution: pwolanin commentedAdd tests and looks complete. sort of annoying that the WidgetFactory is a special case.
Comment #20
pwolanin CreditAttribution: pwolanin commentedupdate title
Comment #21
alexpottThere is no \Drupal\plugin_test\Plugin\plugin_test\fruit\Orange so here you are still just testing the non existing class.
Comment #22
dawehnerMh, for me the Orange exists. #1778942: Discovery::getDefinition() / getDefinitions() : inconsistent return values for "no result" added it.
Comment #23
EclipseGc CreditAttribution: EclipseGc commentedfixing tags.
Comment #24
pwolanin CreditAttribution: pwolanin commented#18: default_factory-2078417-18.patch queued for re-testing.
Comment #25
EclipseGc CreditAttribution: EclipseGc commentedif that passes, this looks pretty good to me.
Eclipse
Comment #26
fubhy CreditAttribution: fubhy commentedTwo periods. Other than that RTBC from me.
Comment #27
dawehnerGood catch!
Comment #28
pwolanin CreditAttribution: pwolanin commented#27: factory-2078417-27.patch queued for re-testing.
Comment #30
tim.plunkett#27: factory-2078417-27.patch queued for re-testing.
Comment #31
dawehnerStill green.
Comment #32
pwolanin CreditAttribution: pwolanin commented#27: factory-2078417-27.patch queued for re-testing.
Comment #33
dawehnerAdding tag.
Comment #34
pwolanin CreditAttribution: pwolanin commented#27: factory-2078417-27.patch queued for re-testing.
Comment #36
dawehner#27: factory-2078417-27.patch queued for re-testing.
Comment #37
jhedstromComment #38
tim.plunkettSomehow this was duplicated and finished in #2318281: Provide a standard way to check a plugin is an instance of the required interface