Create a single Container CompilerPass to collect + add handlers to consumer service definitions

Kick-starting this.

Hm. And this looks sub-optimal...

Converted RegisterBreadcrumbBuilderPass.

How can we avoid the \Drupal::service() call?

So, perhaps, rather this?

Added ContainerTagResolver.

A perhaps simpler alternative to this would be the following:

1. Remove the @tag_resolver service. Instead:
2. Establish a mapping between tag names (on handlers) and managers; i.e., tag name == manager service ID.
3. Establish a standard method name for adding handlers to a manager; e.g., addHandler()
4. Make the compiler pass (1) collect all tagged services, (2) check whether a manager service exists with the specified ID, and if so (3) register addHandler() calls for the manager service definition.

'cos that's not trivial to parse, example:

  breadcrumb:
    class: Drupal\Core\Breadcrumb\BreadcrumbManager
...
  system.breadcrumb.default:
    class: Drupal\system\PathBasedBreadcrumbBuilder
    tags:
      - { name: breadcrumb, priority: 0 }

Note: Tag name == Manager service ID.

Followed by a compiler pass (sans abstraction for clarity):

  foreach ($container->findTaggedServiceIds('breadcrumb') as $id => $params) {
    $manager->addMethodCall('addHandler', array(
      new Reference($id),
      isset($params[0]['priority']) ? $params[0]['priority'] : 0,
    ));
  }

In turn, BreadcrumbManager:

class BreadcrumbManager implements TaggedHandlerInjectionInterface {

  public function addHandler($instance, $priority) {
    $this->builders[$priority][] = $instance;
  }

}

→ Standardized manager/handlers pattern.

The chance for a name clash for a tag name to false-positive match an unintended service ID is pretty close to zero, IMHO.

Thoughts?

That concept in code. :-)

Fixed other breadcrumb handler service definitions.

(Serious WTF @ those priorities.)

Hm. Looks like we talked past each other then. :-/

The idea really was to simplify and replace all of those one-off compiler passes that are just collecting tagged services, in order to supply them to a manager service (by adding a method call to the manager service definition on the container).

That is, because having to duplicate the exact same code for every single tag into a new compiler pass class doesn't really make sense, DX-wise.

My original hope was that we'd be able to just collect the tagged service IDs as container parameters, but it turned out that I forgot about service instantiation (cf. #2, #4). That approach also did not work, because a parameter would not exist at all in case no services are tagged, which in turn causes a "missing parameter dependency" exception upon compiling the container.

What's wrong with the concept proposed in #5?

I think it improves DX and establishes a clear and standardized pattern for all Manager ← Handlers implementations. Those implementations already exist in HEAD, this proposal only simplifies and harmonizes them?

1. Leverage tag attributes to eliminate all concerns. :-)
2. Converted router_matcher:route_filter (RegisterRouteFiltersPass).
3. Converted theme.negotiator:theme_negotiator (ThemeNegotiatorPass).

Adjusting issue title.

D'oh. Sorry!

Fixed another stale variable. Manually tested installer this time. :)

Fixed BreadcrumbManager does not require handlers to implement ChainBreadcrumbBuilderInterface, but only *itself*.

Updated issue summary to reflect the proposed solution.

Now... to show the true DX improvement:

1. Throw an exception for unmet handler requirement unless in 'prod' kernel environment.
2. Add proper interface requirements to compiler passes.
3. Improved phpDoc and added example code.
4. Converted path_processor_manager:inbound + outbound.
5. Converted route_processor_manager:route_processor_outbound.
6. Converted string_translation:string_translator.
7. Converted config.factory:config.factory.override.
8. Converted twig:twig.extension.
9. Converted router.dynamic:route_enhancer.

Thanks, awesome! :-)

Attached patch just adds a @todo to CoreServiceProvider, which was part of the interdiff in #21 already, but which I forgot to actually commit ;)

Nope, there's no constant for the kernel environment.

#2226761: Change all default settings and config to fast/safe production values might introduce one (or two), but it's not clear yet, whether those will actually map 1:1 to a DrupalKernel $environment.

Given that Symfony uses 'prod' and 'dev' as two primary environment names all over the place in their docs, I'm also not sure whether I like the idea of introducing constants.

#2237681: Revise Guzzle client configuration and event registration has landed, so:

Converted new http_client:http_client_subscriber.

I'm working on a great simplification that resolves the concerns + additionally involves a massive DX win, patch coming up shortly.

Here we go:

1. Added high-level documentation to explain and disambiguate the concept, and facilitate naming.

2. Renamed tag attributes: 'compiler_pass' → 'service_consumer', 'method' → 'call'.

3. Be smart: Automatically determine required interface and whether $priority is accepted.

+ test coverage + simplified test code.

Alright, after some further discussion in IRC...

1. Renamed 'service_consumer' to 'service_collector'.
2. Improved/polished the conceptual disambiguation in the class phpDoc.

@Crell: I've adjusted the docs. FWIW, I originally had the aspect of "plugins are configurable / depend on configuration" in there, but removed it, because that's actually not true; the plugin system is able to operate with just a discovery + plugin instances only (sans configuration). In fact, that comes very very close to this pattern of collected/aggregated handlers, but the key difference is the declarative configuration in code. A plugin-based implementation would simply consume/register all available plugins that have been discovered, since there is no declarative negotiation of what to use in between. (I assume that's what you meant, too; just paraphrasing in words I can understand. :-))

Updating issue summary accordingly.

35: container.tags_.35.patch queued for re-testing.

Merged 8.x.

I'm pretty happy with this patch now. Back to RTBC? :)

Created https://drupal.org/node/2247531

Testbot fluke.

Removed potentially premature @todos.

Nevertheless, created:

#2250239: Remove needless ContainerAware dependency from ParamConverterManager
#2250243: Remove needless ContainerAware dependency from AuthenticationManager

Merged 8.x.

oh LOL, I spotted that myself some days ago already, but forgot to fix it - thanks! :-D

Closely related: #2251113: Use container parameters instead of settings