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.

Files: 
CommentFileSizeAuthor
#24 2022087-24.patch4.44 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 56,775 pass(es).
[ View ]
#24 interdiff-2022087-24.txt657 bytesdamiankloip
#21 2022087-21.patch4.44 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 57,958 pass(es).
[ View ]
#21 interdiff-2022087-21.txt4.37 KBdamiankloip
#16 2022087-16.patch4.39 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 58,051 pass(es).
[ View ]
#9 2022087-9.patch4.01 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 57,743 pass(es).
[ View ]
#9 interdiff-2022087-9.txt948 bytesdamiankloip
#8 2022087-8.patch3.94 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#8 interdiff-2022087-8.txt456 bytesdamiankloip
#6 2022087-6.patch3.95 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#6 interdiff-2022087-6.txt3.23 KBdamiankloip
#5 2022087-5.patch6.16 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#5 interdiff-2022087-5.txt3.62 KBdamiankloip
#3 2022087-3.patch4.36 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 58,014 pass(es).
[ View ]
#3 interdiff-2022087-3.txt2.54 KBdamiankloip
d8.module-class-discovery.patch1.82 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] 57,803 pass(es), 8 fail(s), and 0 exception(s).
[ View ]

Comments

amateescu’s picture

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

Status:Needs work» Needs review
Issue tags:-Needs tests
StatusFileSize
new2.54 KB
new4.36 KB
PASSED: [[SimpleTest]]: [MySQL] 58,014 pass(es).
[ View ]

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

EclipseGc’s picture

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

Status:Needs work» Needs review
StatusFileSize
new3.62 KB
new6.16 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

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

StatusFileSize
new3.23 KB
new3.95 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

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

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

if 6 passes, I'm totally RTBC

Eclipse

damiankloip’s picture

StatusFileSize
new456 bytes
new3.94 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

There was one stray empty line, thanks amateescu!

damiankloip’s picture

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new948 bytes
new4.01 KB
PASSED: [[SimpleTest]]: [MySQL] 57,743 pass(es).
[ View ]

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

amateescu’s picture

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

+1, nice regex

alexpott’s picture

Status:Reviewed & tested by the community» Fixed

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

tstoeckler’s picture

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

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

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

Status:Fixed» Needs review
StatusFileSize
new4.39 KB
PASSED: [[SimpleTest]]: [MySQL] 58,051 pass(es).
[ View ]

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

So, we should just change to this?

tstoeckler’s picture

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

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

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

effulgentsia’s picture

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

StatusFileSize
new4.37 KB
new4.44 KB
PASSED: [[SimpleTest]]: [MySQL] 57,958 pass(es).
[ View ]

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

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

StatusFileSize
new657 bytes
new4.44 KB
PASSED: [[SimpleTest]]: [MySQL] 56,775 pass(es).
[ View ]

Good spot. Minimal comment change, leaving as rtbc.

alexpott’s picture

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

Issue summary:View changes

Doesn't need tests.