Problem/Motivation

As a follow-up to #2065571: Add YAML Plugin discovery, let's remove the hook-based plugin discovery. It's not being used in core and between yaml and annotation-based discovery (with derivatives) it would seem like 2 ways is sufficient and more makes the DX worse.

Proposed resolution

Remove the \Drupal\Core\Plugin\Discovery\HookDiscovery class and any references to it in comments or documentation.

Remaining tasks

TODO

API changes

Remove \Drupal\Core\Plugin\Discovery\HookDiscovery so discovery should be more standardized on YAML or annotation discovery

#2065571: Add YAML Plugin discovery

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pwolanin’s picture

Status: Active » Needs review
FileSize
2.67 KB

setting to needs-review for tests, but should be considered postponed until the yaml one is committed.

tim.plunkett’s picture

Initial profiling of #2028109: Convert hook_stream_wrappers() to tagged services. led us to consider using HookDiscovery. But now we're not sure.

In addition, I think HookDiscovery is a nice stepping stone, and could be useful for contrib.

pwolanin’s picture

If we want to make Drupal 8 approachable for new developers, I think constraining the possible equivalent answers to "how to I do X" is important, and my sense is that we will be in general using plugins widely.

If we didn't find a reason to use this in core, and it seems like yaml discovery (with caching at least) would be equally fast, why do we really need both?

pwolanin’s picture

Issue summary: View changes

Updated issue summary.

pwolanin’s picture

Issue summary: View changes

Updated issue summary.

pwolanin’s picture

#1: no-hook-2074407-1.patch queued for re-testing.

chx’s picture

Status: Needs review » Reviewed & tested by the community

YES. One consistent way to do it all is win.

dawehner’s picture

Just a note, we still have the InfoHookDecorator.

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs work

So this issue and #2078405: Revert YamlDiscovery if it doesn't make sense as first-class discovery are directly contradictory.
I think we should leave all three and stop pretending we know what's best for contrib.

pwolanin’s picture

Title: Remove hook-based plugin discovery once we have yaml discovery » Remove hook-based plugin discovery and focus on using jsut annotations and derivatives
Status: Needs work » Needs review

Changing the title based on the actual discussion with chx.

I personally don't have a string opinion. Trying to paraphrase the suggestion from chx is that annotation discovery is optimal since it keeps the meta-data with the class itself, yet it's a little harder to figure out initially than something like yaml or a hook, so devs for 8 will tend to use those even when they are not a good match for the problem.

After considering again the local task and action ones, I am starting to agree that just doing some derivatives based on yaml makes more sense than making that the base discovery.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Considering that I've chosen to won't fix #2078405: Revert YamlDiscovery if it doesn't make sense as first-class discovery, I'm okay with doing this after all.
Setting back to RTBC per #5

tim.plunkett’s picture

Title: Remove hook-based plugin discovery and focus on using jsut annotations and derivatives » Remove hook-based plugin discovery

To clarify, I'd rather we keep this in core for use in contrib.
But as long as we have something other than annotations (in this case, YAML), I'm not going to fight anyone over this.

However, it should probably get sign-off from either EclipseGC, neclimdul, or effulgentsia, none of which are in this thread.

neclimdul’s picture

I would also like to leave it for any possible use in contrib. They probably _should_ use one of the other methods but as the supporting system we're not really in control and should support valid options.

chx’s picture

It's contrib I really badly want NOT to be available for. Drupal 8 is hard. Let's NOT provide more than one way for people to understand. It's bad.

tim.plunkett’s picture

We have a DefaultPluginManager that codifies our best practice, which is annotations.
It is not our business to babysit contrib by removing working and useful implementations.

At the least it would be useful to someone porting their module, to use as a stepping stone.

chx’s picture

Assigned: Unassigned » effulgentsia

Let's see whether I can get Alex into the thread.

effulgentsia’s picture

Status: Reviewed & tested by the community » Postponed

I'd at least like this postponed on #2028109: Convert hook_stream_wrappers() to tagged services., since I can't yet tell from the profiling data on that issue if it'll be needed or not.

EclipseGc’s picture

So, I tried to respond to this over here: https://drupal.org/node/2078405#comment-7815063

I'm happy to keep the conversation on this issue though, but my initial response was there, so I thought I'd post it here.

Eclipse

pwolanin’s picture

Status: Postponed » Reviewed & tested by the community

Since the patch there is now using annotation-based discovery, it seems this was not needed.

catch’s picture

Status: Reviewed & tested by the community » Needs review

The profiling on that issue isn't conclusive yet.

neclimdul’s picture

Since this is open again, I'll go ahead and voice the opinion I've mentioned in IRC. Its just my opinion but I think we should leave the discovery even if we don't use it. We are providing the framework for contrib and even if core doesn't use it is a valid option for contrib. Providing it in core means there is a centralized, highly maintained version.

unpublished. forgot i already said this.

andypost’s picture

Status: Needs review » Needs work
Issue tags: +needs profiling

taggin

tim.plunkett’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -needs profiling

This issue doesn't need profiling. #2028109: Convert hook_stream_wrappers() to tagged services. does. But that issue instead can debate between annotations and YAML now that #2065571: Add YAML Plugin discovery went in.

So excluding

I think HookDiscovery is a nice stepping stone, and could be useful for contrib.

from #2, this is back to RTBC.

pwolanin’s picture

So, in discussing with neclimdul - a clear reason to remove this is it's not used or tested in core. If someone steps up to at least write a thorough test case so we know the functionality is up to date, I'd be willing to reconsider. Or, let's remove it now and add it back later together with the test?

effulgentsia’s picture

I don't think core should provide implementations of interfaces that neither it uses, nor is considered recommended practice for others to use. If the only argument is to satisfy people who like info hooks, but for no solid technical reason, or as a transition step for contrib developers, then I think HookDiscovery belongs in CTools or some other contrib project that those modules can list as a dependency. However, I believe that HookDiscovery does have a solid technical reason: it's runtime faster than the other discoveries, and when you're unable to cache the result, and you need to get the definitions frequently, you need that speed. However, that difference should be showing up more clearly on #2028109: Convert hook_stream_wrappers() to tagged services.; I wonder why it's not.

effulgentsia’s picture

Assigned: effulgentsia » Unassigned

Unassigning myself from the issue. I certainly do not support this being committed until we're clearer on what the speed difference is between HookDiscovery and the other alternatives.

catch’s picture

Status: Reviewed & tested by the community » Needs review

Yes the profiling on #2028109: Convert hook_stream_wrappers() to tagged services. should happen before this goes in. There might be some things we can do if there's a regression there, like using CacheCollector for plugin caches, but let's understand the differences before ripping it out.

Comparing YAML and annotations is not going to tell us if hook discovery is faster on runtime.

neclimdul’s picture

So I sort of agree with effulgentsia but not entirely. This is a bit more verbose version of the argument I made to pwolanin on IRC because its relevant.

<rant>
I think in this case there is a scale of "recommended" that is made clear by the discussion around stream_wrappers and the other discussions around YAML discovery. Furthermore, we should be looking at core as the framework to support contrib not its own thing and let contrib solve its own problems. Cases where we haven't done this and have only focused on core during development have sometimes been _very_ painful for contrib during release.

Because we recommend Annotations doesn't mean the recommendation will meets your needs. That is why its a recommendation not a requirement. Annotations don't meet local tasks needs, we use YAML instead. Annotations don't seem to meet stream wrappers needs and hooks look like they might so we're considering them. Even more important, in contrib, everyone looking at hooks can contribute to the core implementations and if there's a weakness make it better. Everyone wins.

The fact is, Plugins are a framework for building tools and the discovery interface exists not to support the interesting hackery we do with proxy classes but specifically to support building your tool to support different metadata needs. If there's a valid use case for this in contrib, which I think there is, we should be able to consider leaving it on that grounds alone.
Furthermore it is also actually important that we take this stance on our API's so that we are constantly looking at what we need to do to support contrib. We need to have multiple solid implementations of interfaces like this in core to expose possible weaknesses without requiring us to keep things like poll.module around just so we can have a token implementation in core. This is one of the benefits we can gain through having testing and we should take advantage of it.
</rant>

PS- I've moved it to review not because of my concerns because I'm willing to let them pass. My concerns are about what we're arguing not the outcome. It seems like at least we should wait for the stream_wrapper discussion to resolve itself a bit rather then leaving this in the committers queue as "ready" though.

sun’s picture

I agree with @neclimdul, @effulgentsia, and @catch — there's not really a reason for removing this from core.

As suggested by others already, it would make more sense to add a test for it instead.

dawehner’s picture

We are adding things like traits in order to support more than drupal can do out of the box, so let's write a test.

Status: Needs review » Needs work
Issue tags: -DX (Developer Experience), -d8dx

The last submitted patch, hook_discovery-2074407-28.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review

#28: hook_discovery-2074407-28.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +DX (Developer Experience), +d8dx

The last submitted patch, hook_discovery-2074407-28.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
668 bytes
7.32 KB

Damn, forgot the getInfo function.

Crell’s picture

I think as long as we document what "Good" use cases are for info hook discovery, we can get away with keeping it. And making sure it works does help with the "3 implementations" rule.

That said, chx has a point that if we're moving away from hooks as a thing, and info hooks in particular, there's a lot of value to "forcing the issue" and making people convert to annotations or YAML. If we leave it in core, there *will* be lots of modules that go on using info hook discovery not because it's appropriate but because it's familiar. It could also stunt development of another alternative discovery mechanism that would be better for those cases where annotations or YAML wouldn't work.

So, yeah, I'm torn.

Alex, can you explain why info hooks are still appropriate for the stream wrapper issue? Maybe that will help with finding that "third option" that's not an info hook.

effulgentsia’s picture

can you explain why info hooks are still appropriate for the stream wrapper issue?

#2028109: Convert hook_stream_wrappers() to tagged services. has gone back and forth on whether stream wrapper definitions can be cached. I'm not currently clear on what the problem with caching them is. If we cache them, we don't need an info hook, but if we don't cache them, then I think parsing annotations or YAML on every request is too slow.

neclimdul’s picture

If we where locked into the way core releases work I'd say "stunting" would be more of a problem. In contrib things are free to move a lot quicker and we've got something like 3 years for things to evolve. A module might have 2.x release with info hooks because its more familiar and there is less conversion needed.

It would give them the chance to get their feet wet and understand maintaining a plugin system with something their comfortable with. Then they can do a 3.x release with a better meta data system once they are more comfortable. There's probably an argument to be made for providing some comfortable middle ground for contrib developers in places like this. :)

EclipseGc’s picture

I think there are solid logical arguments on both sides. With that in mind, I tend to lean towards being a bit more conservative in our approach here. Sure, having the extra Discovery class could cause confusion, but at the same time, it's not like providing metadata via an info hook is the end of the world and if the contrib module maintainer wants to move to Annotations at some later date, they can version the module or provide a derivative bc layer. In any event, we should make sure this has tests, and beyond that, it's not probably worth spending our effort or our karma on this.

Eclipse

pwolanin’s picture

Title: Remove hook-based plugin discovery » Remove hook-based plugin discovery if it doesn't have tests and some more documentation

updating title

dawehner’s picture

#32: hook_discovery-2074407-32.patch queued for re-testing.

tim.plunkett’s picture

Title: Remove hook-based plugin discovery if it doesn't have tests and some more documentation » Write tests and expand documentation for hook-based plugin discovery

As discussed at Drupalcon, we're not removing this.

tim.plunkett’s picture

Title: Write tests and expand documentation for hook-based plugin discovery » Write tests for hook-based plugin discovery
Status: Needs review » Reviewed & tested by the community
+++ b/core/tests/Drupal/Tests/Core/Plugin/Discovery/HookDiscoveryTest.php
@@ -0,0 +1,150 @@
+    $this->moduleHandler->expects($this->at(1))
...
+    $this->moduleHandler->expects($this->at(2))

This is very cool. Always learning more about PHPUnit.

---

The tests are spot on, and this issue is about rectifying the original code passing the gates. We should be able to fix the tests gate without needing to do the docs at once. So I opened #2100249: Expand documentation for hook-based plugin discovery

EclipseGc’s picture

++

The cleanup on this looks good, I'm still learning to read PHPUnit, but otherwise this seems great.

Eclipse

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.