Help Topics discovery cannot be decorated easily

Adding note about the standard alter being too late in the process.

I am not concerned about decoration for Tours for instance (which use YAML discovery and probably they are decoratable? I haven't checked), or for the hook_help() area on admin/help (since any module can implement hook_help and get their module overview in that section). I'm only concerned here with the plugins for the Help Topics area. I'd like the Configurable Help topics to integrate, and it's kind of a pain now (can be done only by overriding the Help Topic plugin manager service).

This is not the same as the Help Section plugin manager. That plugin type uses classes/namespaces/annotations for discovery, not YAML... so easy for any module to provide as many sections as they want.

Thanks for commenting! Huh, tagged services! That is a good idea, since I think as a Project, Drupal is trying to get away from hooks. I'll look into making a patch for that. Should be easy to test by creating a test module that has such a tagged service, and a simple class that just adds one additional plugin to the discovered list.

I have thought about this some more, and I am less sure about using service tags.

Plugin discovery decorator classes typically have constructors that take as their sole argument an existing DiscoveryInterface class, and then they need to implement the 4 methods on DiscoveryInterface. See https://www.drupal.org/docs/8/api/plugin-api/discovery-decorators for more information about Decorators. So if there were 2 classes that wanted to decorate in our getDiscovery, we'd need to do something like:

$discovery = new class1($discovery);
$discovery = new class2($discovery);

so the first one would decorate the base discovery, and the 2nd one would decorate that result.

But this is not how service classes usually work -- they are typically instantiated by the container, with constructor arguments specified in the services.yml file, and maintained instantiated in the container. These discovery classes aren't really "services" in that sense, and we'd be trying to use the service tag mechanism to specify a list of classes that we would want to instantiate outside the container, rather than what the container expects.

I'm not saying it can't be done this way... I guess we would call the container's findTaggedServiceIds() method to find the IDs, and then instead of the usual call to instantiate the service, we'd instead call getDefinition() to figure out what class was specified in the service definition, and instantiate it ourselves.

But it does seem a bit weird/bad/dangerous to put things in the services.yml file, that if someone actually tried to do a \Drupal::service() on them, they would crash because the container wouldn't be able to instantiate them... Maybe it's better to use a hook after all? Not sure... thoughts? Do we have any other instances of "services" in Core that cannot actually be instantiated by the container?

@larowlan suggested in Slack that we could use service tags, but mark the services as 'private'. This would prevent anyone from using $container->get() or \Drupal::service() on them.

However, I looked at the documentation for Symfony about private services (much the same in 3.4 as it is in 4, we are currently on 3.4):
https://symfony.com/doc/3.4/service_container/alias_private.html

So marking a service as 'private' means that you have to get it via dependency injection rather than get(), and that the container manages its instantiation. Which is *not* what we want here really -- we don't want the container to manage instantiation.

I really think it's a better philosophical match to the problem here to use a hook (which will allow modules to tell us "Please instantiate this class name and use it as a decorator" and not service tags, which would be open to someone trying to use that service name in some kind of dependency injection, and thereby asking the container to instantiate it, which really wouldn't work.

This has nothing to do with search (at least not directly). It has to do with just discovering the topics to put in admin/help in the Help Topics section. Which is the HelpTopicsPluginManager class, whose discovery this issue is about decorating from contrib.

The problem is that if you put something in a services.yml file, the Container expects to be able to instantiate it. But these classes definitely need to NOT be instantiated by the container. They need to be instantiated in the getDiscovery() method, with the new class instance from one passed into the constructor of the next. As in the issue summary:

      $decorators = ;// get the list of decorator class names somehow, like from a hook ;
      foreach ($decorators as $decorator) {
        $this->discovery = new $decorator($this->discovery);
      }

That is just not how service collectors work.

So, I went ahead and made a patch that uses a hook. This patch is 4 lines of actual code, and all the rest is tests, documentation, and test code. See what you think...

If the consensus is still that we should use private services and service tags, then it is fairly easy to rewrite the patch to do that.

Yeah, we could do that. Just to note that it's in a test class... But sure. Anyway, think about the services thing. If we're sticking with the hook, I'll make that change. If not, I'll make a new patch that uses service tags instead.

I think what bothers me is we're doing something different to almost every other plugin system, which doesn't aid discoverability or allow for reuse of existing knowledge.

Yes indeed, hence this issue. But just to clarify... On the earliest patches where help topics were plugins, each help topic was in a YAML file that looked very much like a config entity dump. Then we changed to having topic body in a Twig file, and each module would have one mymodule.help_topics.yaml file with all the meta-data in it. Both of those systems allowed for ContainerDerivativeDiscoveryDecorator.

Then I think it was alexpott had the idea of putting the meta-data into the Twig file, with custom discovery, and we lost ContainerDerivativeDiscoveryDecorator.

I don't think we ever had annotated class discovery.

So... OK, I think we could use https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Plugin%21...
Or rather, we'd make a subclass that would figure out the directories to scan
So we would do something like this in getDiscovery():

// assume $discovery is our custom Twig discovery.
// and that we've figured out what directories to scan, which would be active modules.
$discovery = new YamlDiscoveryDecorator($discovery, 'help_topics', $yaml_directories);
$discovery = new ContainerDerivativeDiscoveryDecorator($discovery);

Then contrib modules could make a mymodule.help_topics.yml file, and either put some plugins in there directly, or a deriver.

I'll make a patch that does that. I think I agree that this would be better than the hook idea, and put it more in line with other plugin systems.

OK, here we go. This patch uses the existing core YamlDiscoveryDecorator, along with ContainerDerivativeDiscoveryDecorator.

I didn't make an interdiff, as it's a pretty different patch from the previous version. The only thing in common is the fake test plugin class, which is unchanged from the previous patch. See what you think... it is definitely more Standard Drupal Core Plugin API.

Test passes locally... I tested both putting in a direct plugin in the YAML and a plugin deriver. Both of them work fine.

Fixing coder message (removed one newline from end of one file -- didn't bother with an interdiff, I just edited the patch file).

Regarding #19 item 1, I'll add an example.

Regarding #20...

item 1: I will add a comment to explain that the base HelpTopicsDiscovery allows themes and modules and core to provide topics. This is unlike most other plugins we have in Core. As a note, I also renamed that local variable from $directories to $all_directories, to contrast it with the local $module_directories variable.

item 2: I don't think we need to allow themes to decorate (profiles could, because they are returned along with modules from the module handler). To me, decoration is like hooks or other plugins -- generally, that is module territory, because it is altering behavior.

item 3: In this test deriver class, we don't need to add a base definition. It's doing its own totally fake thing for the test, kind of like a unit test stub class, which is why it didn't even use DeriverBase. I think it's best just to keep it simple.

Anyway, here's a patch that hopefully improves the documentation/comments.

Do we need a change notice for this? We apparently didn't make one for #2920309: Add experimental module for Help Topics so I'm not sure we need one now... all of the classes in help_topics are marked @internal so I think we don't do change notices until we remove that?

In any case, updating the issue summary.