Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 UTC on 18 March 2024, to get $100 off your ticket.
Problem/Motivation
Follow up to #2301393: Deprecate all of mail.inc, move drupal_mail to method on Mail plugin manager.
There is custom code to check if a plugin is an instance of the required interface.
+++ b/core/lib/Drupal/Core/Mail/MailManager.php
@@ -136,9 +154,100 @@ public function getInstance(array $options) {
+ throw new InvalidPluginDefinitionException($plugin_id, String::format('Class %class does not implement interface %interface', array(
+ '%class' => get_class($plugin),
+ '%interface' => 'Drupal\Core\Mail\MailInterface',
+ )));
}
Proposed resolution
Provide a generic way for handling this.
Remaining tasks
User interface changes
API changes
Comment | File | Size | Author |
---|---|---|---|
#50 | plugin-2318281-50.patch | 95.37 KB | tim.plunkett |
#37 | plugin-2318281-37.patch | 91.7 KB | dawehner |
#36 | 2318281-plugin_interface-36.patch | 85.97 KB | dawehner |
#30 | interdiff.txt | 923 bytes | tim.plunkett |
#30 | plugin-2318281-30.patch | 86.71 KB | tim.plunkett |
Comments
Comment #1
dawehnerHere is a start.
Comment #2
tim.plunkettI think this is a great idea.
I expanded this fully, which required adding 2 interfaces. Also, I pulled the code into another method on DefaultPluginManager, so that classes that don't use interfaces can opt out.
Comment #3
tim.plunkettWell let's actually address the issue summary.
Comment #5
tim.plunkettWhile trying to debug the fails, I realized the exception wasn't enough information. Expanded that, and fixed views join.
Comment #7
tim.plunkettAhh, very interesting. This code now triggers autoloading, which reveals some interesting assumptions/bugs in taxonomy.
Comment #8
dawehnerWell, we should make it optional to check the interface.
Comment #11
dawehnerThis time done properly.
Comment #13
tim.plunkettOkay, I agree it should be opt-in.
Comment #14
tim.plunkettComment #15
dawehnerOh wow!
Comment #16
EclipseGc CreditAttribution: EclipseGc commentedI'm pretty sure Kale is a vegetable...
Is this related to this patch? or did it sneak in from elsewhere?
In general, this looks pretty good. My one big question here is actually around adding this functionality to the DefaultPluginManager in Drupal\Core. Two points:
I guess the last couple things are just the aggregator's plugin manager looks completely weird compared to all the other here. I dunno why fetchers/parsers/processors are all dealt with in a single manager when they're doing such different things. I kind of thought we'd end up passing an interface through a constructor variable like we do directory and annotation class. Is there a reason we chose to hard code it on each manager instead of injecting it?
Eclipse
Comment #17
dawehnerFixed.
See #2318281-7: Provide a standard way to check a plugin is an instance of the required interface
Comment #19
neclimdulWe can probably follow up on this because we don't really do this anywhere else but I feel like we should have an explicit exception. Exceptions in the plugin system have always been a weak point.
Maybe this is exposing a problem with the implementing systems but this smells. An array for a singular variable? Multiple "valid" interfaces?
re #16
Code comments:
1) I _think_ that's the point. It doesn't implement the fruit interface.
2) In #7 Tim mentions some "interesting assumptions/bugs in taxonomy" and added this but I don't understand what this is fixing either.
Other comments:
1) We can follow up and discuss.
2) I'm actually a little uncomfortable with this. We explicitly go to lengths to not load code during definition discovery. While this doesn't load code outside of the specific plugin system, its a regression. On the other hand, allowing a user to select a plugin that is actually invalid is bad. On the other hand, this magic makes for a bit of a dev hurdle in "why doesn't my plugin show up?"
We originally didn't include this because of this autoload requirement and I'm curious what the use case is that discovery is going to find plugins that aren't actually plugins? We came to the conclusion originally that this wouldn't be something we would see.
Comment #20
tim.plunkettHa! Yes, kale is a vegetable. Hence it doesn't implement FruitInterface!
I'm worried about the autoloading too, but not sure if we should continue here.
Comment #21
dawehnerOh right! Just ignore my previous patch then.
Well, we could at least move this functionality onto the factory, which still should be okay, don't you think so?
Comment #22
EclipseGc CreditAttribution: EclipseGc commentedYes, I do think that's ok. It'll throw a nice big fat error and the dev will go "Why doesn't this implement that interface?... better fix it."
I know it's not perfect from a UI perspective because technically people COULD implement a plugin that doesn't implement the interface and it could be selected to be run and and and, but re: discovery, I've always used createInstance() as my example for how to enforce this. I'd be a lot more comfortable with that, and I also still think we should pass the interface through the constructor so that we have a 1 to 1 relationship of service ids to interface expectations. Unless we have a real legitimate use for a single service dealing with multiple interfaces. (Note: I say service to denote the difference between a manager that services multiple types under different service ids vs one that COULD service multiple from a single id... which I hope we don't do)
Eclipse
Comment #23
tim.plunkettI'll look into both of the last suggestions.
Comment #24
effulgentsia CreditAttribution: effulgentsia commentedI agree with both suggestions in #22. I also agree with #16.2.1: why not move all of the DefaultPluginManager additions to PluginManagerBase instead? #17 says to punt that to a followup, but I don't see why.
Comment #25
effulgentsia CreditAttribution: effulgentsia commentedxpost
Comment #26
tim.plunkettSomething like this?
Comment #28
tim.plunkettAhem.
Comment #30
tim.plunkettStupid mistake.
Comment #33
tim.plunkettDidn't retesting used to mark back to needs review? Oh well, passes now.
Comment #34
dawehnerHere is a question: why is that not part of the factory? Just curious. Technically, both obviously work.
I am fine if you want to kill me: did you considered to not introduce the views handler interface but use the base class instead? Technically
is_subclass_of
covers bothand we could actually have a proper discussion about the interface in contrast to just sneaking it in.
I am not a native speaker, but at least the why I understand "should" is optional. So maybe replace it with "has to".
Maybe implement with " or "?
it is a bit sad that we have two "ways" to specify it. Maybe we could just always use the property on the class? That way feels a bit more readable tbh.
oh git.
Comment #36
dawehnerWorking on moving to the default factory tomorrow. This is just a reroll.
Comment #37
dawehnerThis could pass.
Comment #39
dawehnerAlso expanded the test coverage a bit.
Comment #40
tim.plunkettWe can ditch the is_array part, that stopped being valid after the last rewrite. It will always be a string here.
Comment #42
tim.plunkettIt turns out you cannot use @covers for static methods. Running the coverage reports failed for me.
The fail was just a missing bit in ArchiverManager, it happens to be the only manager I can see that both calls parent::__construct AND overrides createInstance().
Comment #43
dawehnerI really think this is ready now. Anyone wants to step forward?
@tim.plunkett
Did you tried creating an issue against phpunit? Maybe in the future this could be fixed.
Comment #44
EclipseGc CreditAttribution: EclipseGc commentedWrong Contains text. Needs to be migrate_drupal.
Other than this, I am 100% on board with this issue. Needs work for that comment, RTBC subsequent to that.
Eclipse
Comment #45
tim.plunkettFixed that and removed use statements made obsolete by this patch. RTBC per #44.
Comment #47
tim.plunkettYay, it works! Finding plugins that aren't extending their interface FTW.
Comment #48
EclipseGc CreditAttribution: EclipseGc commentedJust reaffirming the RTBC on this.
Eclipse
Comment #50
tim.plunkettContext conflict with #2096703: Image toolkits should use PluginFormInterface and ContainerFactoryPluginInterface
Comment #51
alexpottNice patch - Committed 840cd4c and pushed to 8.0.x. Thanks!
Comment #53
BerdirThis changed the order of arguments in DefaultPluginManager::construct(). I'm not sure why it wasn't just added last, but this means that all existing (contrib) plugin managers are now dead and need to be updated. So it should have a change record and we also need to update a bunch of existing change records I think.
Comment #54
ArlaCool. I'm also wondering about the argument order. I wrote draft change record at https://www.drupal.org/node/2338561.
Comment #55
ArlaAlso updated some older records mentioning DefaultPluginManager:
https://www.drupal.org/node/2087153/revisions/view/6979427/7634271
https://www.drupal.org/node/2034535/revisions/view/7443805/7634275
Cookies to anyone who can find another record that needs updating because of this change!
Comment #56
alberto56 CreditAttribution: alberto56 commented@Arla thanks for the draft change record. Here is an example of an issue that is impacted by this: #2336309: Devel Generate cannot be used due to Plugin discovery problems.
Comment #57
tim.plunkettI think this has been addressed, if not please open a new issue.