Document service collector tags and the service_collector tag

Removing that tag, because although I think this is a valid critical based on the parent issue, it's not accurate to say that it's been triaged by the branch maintainers yet.

Good start! The documentation looks good, but shouldn't it be going on \Drupal\Core\Extension\ModuleUninstallValidatorInterface not ModuleInstallerInterface ? #2264047: [meta] Document service definition tags says that the docs should go on the interface associated with the tag, and if the documentation is correct, that's the Uninstall interface right?

Since there's a patch, needs review.

The patch is not yet complete. It includes only one of the service tags detailed in the summary.

So far it looks good though!

So the question is - what happens when we don't control the interface.
So for http_client_subscriber - this is the Guzzle\Event\SubscriberInterface - we can't add documentation to that.
So a solution might be to extend their interface with an empty one - like in the attached patch.
The same applies for other such as:

• logger (Psr\Log\LoggerInterface)
• mime_type_guesser (Symfony\Component\HttpFoundation\File\MimeType\MimeTypeGuesser)
• non_lazy_route_filter (Symfony\Cmf\Component\Routing\NestedMatcher\RouteFilterInterface)
• non_lazy_route_enhancer (Symfony\Cmf\Component\Routing\Enhancer\RouteEnhancerInterface)

Haven't done these until we decide that is the desired approach.

@larowlan / #11 - check the parent issue summary, which has suggestions for what to do in that case (item 1(c)). Hopefully that will answer your question. Basically: find a relevant class that we own and document it there, referring to the interface that services of that type need to implement. Adding a new interface just for purposes of documentation is not a good idea.

So back to the patch in #10 -- it's looking pretty good! A few suggestions:

a)

+ * @service_tag breadcrumb_builder
+ *   Services tagged breadcrumb_builder must implement
+ *   \Drupal\Core\Breadcrumb\BreadcrumbBuilderInterface.

This doesn't describe what the service tag means. Keep in mind that on api.drupal.org, each service tag will have its own page, so the description needs to stand alone. Check the parent issue for standards.

b) Others with the same problem:
config.factory.override

c)

+ * @service_tag service_collector
+ *   Services tagged service_collector create new tags then find all
+ *   corresponding tagged services and add a method call for each to the
+ *   consuming/collecting service definition.

Is there an interface that these services must implement? If not, we need to be explicit about what methods the classes need to have on them in order to work. Presumably something tagged 'service_collector' will have some method or methods that will get called somewhere, so hopefully there is an interface; if not we really do need to document what those methods are.

RE #13, OK, then let's put in a reference to TaggedHandlersPass. Something like "See \Drupal\Core\DependencyInjection\Compiler\TaggedHandlersPass for information on how to define a service collector." (again, keeping in mind that these little @service_tag items need to stand alone).

Thanks!

Also just another note on the docs for service collector: it's a rather run-on sentence:

Services tagged service_collector create new tags then find all corresponding tagged services and add a method call for each to the consuming/collecting service definition.

How about:

Services tagged service_collector create new tags, find all corresponding tagged services, and add a method call for each to the consuming/collecting service definition.

Although... what does that last part really mean? How can it add methods to a service definition? How do you figure out what the consuming/collecting service definition is? When is this method called? It's very confusing... if it is explained better in TaggedHandlersPass, maybe this should just say:

Services tagged service_collector define service tags and collect the services tagged with those tags. See \Drupal\Core\DependencyInjection\Compiler\TaggedHandlersPass for more information.

But... I just took a look at
https://api.drupal.org/api/drupal/core!lib!Drupal!Core!DependencyInjecti...

I don't actually see any documentation there about how to define a service collector class. At all. Where is this documentation? Maybe we need to point to the ::process() method instead of the class, or maybe we need to add more to the class header? To me if I didn't know anything about service collectors (and basically you can assume that I don't!), I sure wouldn't have a clue how to go about defining one after reading this class documentation page. The process() method docs say a bit more but it's not really a recipe for how to do this.

So to summarize (sorry for all the comments): My recommendation for the service collector tag:

a) Put this into the @service_tag description:

Services tagged service_collector define service tags and collect the services tagged with those tags. See \Drupal\Core\DependencyInjection\Compiler\TaggedHandlersPass for more information.

b) Fix the TaggedHandlersPass class documentation so that it contains information that someone who didn't know what service collectors do or how to define one could learn that.

Or else write a separate topic with @defgroup, and change the reference in (a) to point to that.

I think some of the information is currently in the ::process() method docs, and it could be removed from there.

c) We should probably update the part of
https://api.drupal.org/api/drupal/core!modules!system!core.api.php/group...
that talks about service collectors to point to this same documentation created in (b). Alternatively, maybe the best thing would actually be in (b) to add this documentation to the defgroup service_tag? It could go in a @section of its own... that is actually probably the best solution.

Would you like me to take a try at writing this, and then you can correct all of my mistakes? :)

Oh sorry, missed #15. You cannot use @see. However:
- You can use @link ... @endlink
- If you just put in a full class name it will become a link.

@cilefen and I just took a stab at this, hopefully the attached patch handles all the current service_collector service_tag annotations.

We went through the list in the description in reverse order, here are the notes for each.

twig.loader - non-Drupal, WE SKIPPED IT
twig.extension - interface not in Drupal, so added to TwigEnvironment
- note that \Twig_Environment::addExtension has no support for priority, while the service definition for twig.extension in core.services.yml has a priority
theme_negotiator - tagged in ThemeNegotiatorInterface
string_translator - tagged in TranslatorInterface.php
- has a prioirty, reflected in comment
route_processor_outbound - tagged in OutboundRouteProcessorInterface
- has a priority
path_processor_outbound - tagged in OutboundPathProcessorInterface, with priority
path_processor_inbound - tagged in InboundPathProcessorInterface, with priority
paramconverter - tagged in ParamConverterManagerInterface
page_cache_response_policy - tagged in ResponsePolicyInteraface
page_cache_request_policy - tagged in RequestPolicyInterface
non_lazy_route_filter - symphony, skipped
non_lazy_route_enhancer - symphony, skipped
module_install.uninstall_validator - (cilefen did this in patch 20) (modified comment)
mime_type_guesser - interface in Symphony, added to MimeTypeGuesser.php class, with priority
logger - interace is Psr\Log\LoggerInterface, added to LoggerChannelFactory, with priority
http_client_subscriber - is GuzzleHttp\Event\SubscriberInterface, added to \Drupal\Core\Http\Client.
config.factory.override - cilefen added to ConfigFactoryOverrideInterface (AD: not sure about the comment)
cache_tags_invalidator - cilefen added to CacheTagsInvalidatorInterface
breadcrumb_builder - cilefen added to BreadcrumbBuilderInterface (AD added required priority)
authentication_provider - cilefen added to AuthenticationProviderInterface (AD added the optional priority)
- Note: there's an mandatory $id in addProvider(), double check that this gets passed properly (not from service container defintion) see related to commit a704796c and https://www.drupal.org/node/2250243
service_collector - cilefen added to TaggedHandlersPass

Oops. OK well... while you were working on the last couple of patches, I just wrote the documentation from #17. Here's an interdiff that will hopefully apply... I was working off the patch in #20.

Also regarding the question in #20, I don't think an @link is necessary since it's defined on that interface.

Looks like the interdiff works. Here's the patch in #23 plus my interdiff in #24.

I read through the patch in #23/#26. It all looks really good to me! A few small nitpicks:

a)

+++ b/core/lib/Drupal/Core/Http/Client.php
@@ -14,6 +14,11 @@
 
 /**
  * Drupal default HTTP client class.
+ *
+ * @service_tag http_client_subscriber
+ *   Services subscribing to http client events must be tagged with
+ *   http_client_subscriber and must implement
+ *   \GuzzleHttp\Event\SubscriberInterface.

I think that should be "... subscribing to HTTP client events..."?

b)

+++ b/core/lib/Drupal/Core/Logger/LoggerChannelFactory.php
@@ -13,6 +13,10 @@
 
 /**
  * Defines a factory for logging channels.
+ *
+ * @service_tag logger
+ *   Logging services must be tagged with logger (with optional priority) and
+ *   must implement Psr\Log\LoggerInterface.

Last line I think it should be \Psr\... ?

c) A couple of the @service_tag items got put in the @file blocks instead of class/interface blocks:
core/lib/Drupal/Core/PathProcessor/InboundPathProcessorInterface.php
core/lib/Drupal/Core/PathProcessor/OutboundPathProcessorInterface.php

d)

+++ b/core/lib/Drupal/Core/RouteProcessor/OutboundRouteProcessorInterface.php
@@ -11,6 +11,11 @@
 
 /**
  * Defines an interface for classes that process the outbound route.
+ *
+ * @service_tag route_processor_outbound
+ *   Services that process the outbound route be tagged with
+ *   route_processor_outbound (with an optional priority) and must implement
+ *   \Drupal\Core\RouteProcessor\OutboundRouteProcessorInterface.
  */

"must" is missing from "... that process the outbound route be tagged"

e)

+++ b/core/lib/Drupal/Core/Theme/ThemeNegotiatorInterface.php
@@ -24,6 +24,11 @@
  * other theme negotiators. By convention, a priority of "1000" is used in
  * these cases; see \Drupal\Core\Theme\AjaxBasePageNegotiator and
  * core.services.yml for an example.
+ *
+ * @service_tag theme_negotiator
+ *   Services which determine the active theme must be tagged with
+ *   theme_negotiator and must implement
+ *   \Drupal\Core\Theme\ThemeNegotiatorInterface.

which -> that

1. +++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/TaggedHandlersPass.php
   @@ -36,46 +36,19 @@
   +   * @link service_tag Service Tags @endlink for more information.
   

   where does this link go?

2. +++ b/core/modules/system/core.api.php
   @@ -2044,14 +2044,47 @@ function hook_display_variant_plugin_alter(array &$definitions) {
   + * @section collectors Collecting tagged services
   

   this isn't the same link keywords

ah, that was added in #2316861: Write a @defgroup for service_tag

Yes, the link goes to the topic page:
https://api.drupal.org/api/drupal/core!modules!system!core.api.php/group...
The new section in core.api.php is inside this @defgroup.

1. +++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/TaggedHandlersPass.php
   @@ -36,46 +36,19 @@
   -   * @code
   -   * tags:
   -   *   - { name: breadcrumb_builder, priority: 100 }
   -   * @endcode
   

   priority example didn't move.

2. +++ b/core/modules/system/core.api.php
   @@ -2044,14 +2044,47 @@ function hook_display_variant_plugin_alter(array &$definitions) {
   + *     - (optional) The found service's priority, if the method accepts a second
   + *       parameter and its name is $priority. If this is omitted, the default
   + *       weighting is used.
   

   is this the same priority mentioned below?

   - And any other parameters defined on the collecting method signature.

3. +++ b/core/modules/system/core.api.php
   @@ -2044,14 +2044,47 @@ function hook_display_variant_plugin_alter(array &$definitions) {
   + * - When defining a service to be collected, you can use a 'priority'
   + *   attribute on its service tag; this value will be passed to the collector
   + *   method, if supported.
   

   cilefen says priority is and example of another parameter, but not all will have it, and they could have other ones supported.

   Maybe make this more generically worded, and then add "for example 'priority' something something"

4. +++ b/core/modules/system/core.api.php
   @@ -2044,14 +2044,47 @@ function hook_display_variant_plugin_alter(array &$definitions) {
   + * There are several service collectors in Drupal Core. You can find them in
   + * the core.services.yml file by searching for services with the
   + * service_collector tag. The Core method that is responsible for actually
   + * finding the tagged services is
   + * \Drupal\Core\DependencyInjection\Compiler\TaggedHandlersPass::process().
   

   is this the reason more examples are not here? cause we have enough examples in core?

+++ b/core/modules/system/core.api.php
@@ -2044,14 +2044,47 @@ function hook_display_variant_plugin_alter(array &$definitions) {
+ * - Define the service class, including either an addHandler method or the
+ *   method you specified in the 'call' attribute. Typically this method will
+ *   add the found service object to a list, and other functions on the service
+ *   class will later operate on the list of found services.

where is the class name specified, where to put the class? (is it in the *.services.yml file where the tag definition is?

maybe we need more context lines in the @code chunk so we can see where "the class" is specified.

@cilefen and I talked this over, we have an idea.

patch coming. (or a comment coming)

[edit: order of the params might not matter. TaggedHandlersPass::process() has a for each and does some wierd stuff with positions]

RE #31, we don't want to mix up the attributes that go on the service collecting class's service definition, and the attributes (priority) that go on the services being collected. And I didn't really think that the example of how to specify the priority was all that illuminating that it needed to be kept, given that we had another example of how to add the attributes on the collecting service (the topic is about how to define a collecting service anyway).

And the wording on the priority argument for the service collector method was basically taken from the documentation on the class. If it's wrong, please fix it.

And yes, I don't think we need to put lots of examples in the documentation, and pointing people to where they can find actual working examples is better. Right?

RE #32, that's part of defining a service, which we have documentation on in the Services and Dependency Injection topic. There's a reference just above this section that says to see that for details of how to define a service.

RE #34, the interdiff is really garbled... I will wait until you're done before commenting on this! But... PLEASE let's not define examples fully in the docs. Point people to an actual class and/or actual methods. Much better.

Oh and regarding complete code examples... A great idea for drupal.org pages. A lousy idea for api.drupal.org and doc blocks in the code. Really please don't do this.

plan for @cilefen and me today is to make the handbook page, get the wording and concepts solid or us, and then go back to this patch (and the code the wording is taken from) and make it concise but useful.

So this issue was originally created as critical as a clone of #2264047: [meta] Document service definition tags which in turn was critical as part of #2238935: [meta] Complete missing documentation for special strings and metadata like annotation keys, routing parameters, tagged services, etc., but as @tim.plunkett has pointed out, this API itself is already well-documented in the codebase and the API is not going to be implemented all that often. Furthermore, you can discover what these services are for by looking at the services core provides (though having more central documentation on api.d.o will definitely make it easier to discover). Based on that, downgrading to major.

@jhodgon, can you confirm that this makes sense?

I don't think I was the one to mark these docs issues Critical. ;) Major is fine, but let's get it done anyway.

I made some edits on the d.o page https://www.drupal.org/node/2239393 -- parts of it were garbled/confusing/backwards I think, plus there were some typos and HTML style fixes (such as that terminology should be using EM tags not STRONG, and code should use CODE not strong). Check out the diffs and see what you think?

OK then, we still need a patch for this issue. The last one needs work, to bring it in line with the d.o page.

Actually we should wait to complete this issue, at least the part that does @service_tag, until we have formally adopted the policy on the parent issue.

What is this waiting on? Which issue?

Right, thanks!