Follow up from #1683644: Use Annotations for plugin discovery once it lands.

From #1683644-119: Use Annotations for plugin discovery:

why do we need the "id" key at all

From #1683644-123: Use Annotations for plugin discovery:

The id key is to prevent us from having to know the full PSR-0 name of any class we ever want to implement.

But perhaps there's a sensible way to default the id to something other than the full class name?

Comments

effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia commented

One option is to combine the module name (i.e., the 2nd part of the namespace: between "Drupal" and "Plugin") with the last part of the class name (either as-is or lowercased). So, for example, for
Drupal/aggregator/Plugin/aggregator/fetcher/DefaultFetcher, we could default the plugin id to one of:
- aggregator.DefaultFetcher
- aggregator_DefaultFetcher
- aggregator_default_fetcher (Symfony has functions for converting CamelCase to underscored lowercase).

David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein commented

I wonder a little bit if we could remove it entirely (rather than making it optional)... is there any use case for having complete freedom to choose the name of your implementing class but then needing additional freedom to choose a different name for the plugin ID?

Maybe since other discovery methods allow it to be specified, though, this should too. (And making it optional is a good start either way.)

Of the suggestions above, I'd vote for "aggregator.DefaultFetcher" just because that's the one that popped into my head before reading this issue :) I'd vote against "aggregator_default_fetcher" because I think that obscures things too much. As I've started to slowly make my way through the new plugin system code, I really like what I've seen but the one thing that's bothered me is that there are a lot of different things named in different places, and I have trouble figuring out how to go from reading the code that instantiates/uses the plugin to finding the code that actually implements it. Standardizing IDs so that looking at the ID helps you immediately see which class implements the plugin seems like a small step down the road of solving that.

EclipseGc’s picture

EclipseGc CreditAttribution: EclipseGc commented

Maybe since other discovery methods allow it to be specified, though, this should too.

Yes, this. Annotations are not some special flower. We've tried to tweak and tweak them over and over again, and I still am questioning most of that since 99% of these issues have absolutely nothing to do with Annotations. I'm not a fan of generating ids for this stuff because we can't make any assumptions really. If the DefaultFetcher class had been named just Default, then going with aggregator.Default would be insufficient, so you actually (bare minimum) need aggregator.fetcher.Default, then to complicate matters further, nothing prevents another module from implementing a class with exactly the same name (since their namespaces will be different) and suddenly we have id collisions. Obviously we could have id collisions now, and likely the solution for solving it in either case is going to be "dsm() the plugin definitions that are returned" but removing this responsibility from the developer is a bad idea in my opinion... especially when said developer might want to manually invoke this plugin for testing or other purposes, and now he doesn't even know the id of his own plugin.

We have to have an id (generated or no) I still don't understand why we'd want to generate this.

Eclipse

effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia commented

If the DefaultFetcher class had been named just Default, then going with aggregator.Default would be insufficient, so you actually (bare minimum) need aggregator.fetcher.Default

Why? plugin ids only have to be unique within their type, not globally.

nothing prevents another module from implementing a class with exactly the same name (since their namespaces will be different) and suddenly we have id collisions.

Right, that's why this issue suggests making the first part of the id the module name. So, if module 'foo' implements DefaultFetcher, its class is Drupal\foo\Plugin\aggregator\fetcher\DefaultFetcher, which would correspond to an id of foo.DefaultFetcher and not collide.

Of the suggestions above, I'd vote for "aggregator.DefaultFetcher"

#2 gives some good arguments for that, but it would introduce a different id pattern (dots and capital letters) than have been traditionally used as info hook array keys and CTool plugin ids. Not sure how much of a concern that is though.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Postponed » Closed (won't fix)

Considering that in most cases, all that is needed is the id key (and module until #1780396: Namespaces of disabled modules are registered during test runs is fixed), I think we should just won't fix this.

One of the benefits of these IDs is that a class name could be changed at any time, and nothing referencing the ID would even know.

Also in light of #1879496: Do we need to worry about plugin ID collisions?, I'm closing this.