For issues like #1825896: Add module owner to plugin data on handlers it would be good if we could actually use the module owner for a plugin and store it, so that when the plugin is not there, we can provide better details about the dependencies.

When we were using the @Plugin definition for everything, we added the module owner to the actual annotation, but now we are using individual annotation classes (like PluginID) these have been removed.

I say bring this back, but automate this on AnnotatedClassDiscovery and get the module from the namespace.

CommentFileSizeAuthor
#24 2022087-24.patch4.44 KBdamiankloip
#24 interdiff-2022087-24.txt657 bytesdamiankloip
#21 2022087-21.patch4.44 KBdamiankloip
#21 interdiff-2022087-21.txt4.37 KBdamiankloip
#16 2022087-16.patch4.39 KBdamiankloip
#9 2022087-9.patch4.01 KBdamiankloip
#9 interdiff-2022087-9.txt948 bytesdamiankloip
#8 2022087-8.patch3.94 KBdamiankloip
#8 interdiff-2022087-8.txt456 bytesdamiankloip
#6 2022087-6.patch3.95 KBdamiankloip
#6 interdiff-2022087-6.txt3.23 KBdamiankloip
#5 2022087-5.patch6.16 KBdamiankloip
#5 interdiff-2022087-5.txt3.62 KBdamiankloip
#3 2022087-3.patch4.36 KBdamiankloip
#3 interdiff-2022087-3.txt2.54 KBdamiankloip
d8.module-class-discovery.patch1.82 KBdamiankloip
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

amateescu’s picture

amateescu CreditAttribution: amateescu commented

Yes! I think this is immensly useful :)

Status: Needs review » Needs work

The last submitted patch, d8.module-class-discovery.patch, failed testing.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
interdiff-2022087-3.txt2.54 KB
2022087-3.patch4.36 KB

Actually fixing the tests for annotation discovery will also cover what this patch adds.

EclipseGc’s picture

EclipseGc CreditAttribution: EclipseGc commented
Status: Needs review » Needs work

Been saying we should do this for a while now. Big ++ from me. The only problem is that this should be done to Drupal/Core/Plugin/Discovery/AnnotatedClassDiscovery, not the Component class.

Eclipse

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Status: Needs work » Needs review
FileSize
interdiff-2022087-5.txt3.62 KB
2022087-5.patch6.16 KB

Ok, that makes sense. We will pretty much have to copy the method over verbatim as I have it in my patch.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
FileSize
interdiff-2022087-6.txt3.23 KB
2022087-6.patch3.95 KB

How about this instead? After speaking to Eclipse on IRC, we should preserve the class inheritance a bit better.

interdiff is based on patch from #3

EclipseGc’s picture

EclipseGc CreditAttribution: EclipseGc commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Plugin system

if 6 passes, I'm totally RTBC

Eclipse

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
FileSize
interdiff-2022087-8.txt456 bytes
2022087-8.patch3.94 KB

There was one stray empty line, thanks amateescu!

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Status: Reviewed & tested by the community » Needs review
FileSize
interdiff-2022087-9.txt948 bytes
2022087-9.patch4.01 KB

Ok, also speaking to alexpott on IRC, we should only add the module key if it hasn't already been set.

amateescu’s picture

amateescu CreditAttribution: amateescu commented
Status: Needs review » Reviewed & tested by the community

Yep, I think it's only fair to not override this property if the plugin annotation already provides it.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented

+1, nice regex

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

Committed 0360c07 and pushed to 8.x. Thanks!

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented

Hmm.. this doesn't make very much sense for plugins provided by Drupal/Core. Couldn't we have gone with something generic like 'owner' or 'provider'?

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented

I think only modules can provide plugins currently (correct me if I'm wrong)? Until that changes I think this is good. If it does, maybe this should be reviewed..

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented

Well if Drupal/Core can't provide plugins then that's certainly a regression: #1828616: AnnotatedClassDiscovery is not finding plugins in Drupal\(Core|Component)\COMPONENT_NAME\Plugin directory :-)

#1821846: Consider better naming conventions for plugin types owned by includes doesn't seem like it's going anywhere lately, but I still think this particular bit is unnecessary.

$plugin['module'] == 'Core' // WTF??

$plugin['owner'] == 'Core'
$plugin['owner'] == 'node' // makes sense, IMO

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Status: Fixed » Needs review
FileSize
2022087-16.patch4.39 KB

Yes, sorry. There is one implementation of this in Core nowdays - Validation constraints!

So, we should just change to this?

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
Status: Needs review » Reviewed & tested by the community

Yay, thanks for the quick turnaround. I hadn't found the validation constraint plugins myself, but thanks for digging that up.

Assuming this is green, it's RTBC.

EclipseGc’s picture

EclipseGc CreditAttribution: EclipseGc commented

Seems somewhat pedantic to me, but I'm also not really opposed so++

We should probably file followups to remove the module annotation from as many plugins as possible now in favor of owner.

Eclipse

msonnabaum’s picture

msonnabaum CreditAttribution: msonnabaum commented

Is "owner" the right word here? Usually we use that to refer to users. What about "provider" instead?

effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia commented
Status: Reviewed & tested by the community » Needs review

Usually we use that to refer to users.

Specifically, that's the only way the term is used in HEAD. Well, except for in the theme system, where it's used to refer to a file, but that's a separate thing we might want to fix in a different issue.

"owner" does have history as referring to a module name in CTools and I think some D8 CMI issues, but not in any HEAD code. Given that, I agree with #19's suggestion of 'provider'. Or if someone has other naming ideas, lets consider them.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
FileSize
interdiff-2022087-21.txt4.37 KB
2022087-21.patch4.44 KB

Here is a patch to change it to 'provider' too.

I really don't care either way, and agree with eclipses comment in #18. Someone choose one, and let's just go with it. The expression 'Bigger fish to fry' springs to mind :)

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented 
+++ b/core/lib/Drupal/Core/Plugin/Discovery/AnnotatedClassDiscovery.phpundefined
@@ -69,27 +69,27 @@ public function getDefinitions() {
+   *   The namespace to extract the owner from.

I guess this string should also refer to provider.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
FileSize
interdiff-2022087-24.txt657 bytes
2022087-24.patch4.44 KB

Good spot. Minimal comment change, leaving as rtbc.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

Committed e865959 and pushed to 8.x. Thanks!

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

(not verified) CreditAttribution: commented
Issue summary: View changes

Doesn't need tests.