Improve authentication manager service construction to support custom global service providers

Tagged as contrib project blocker because at the moment a custom ServiceProvider class is necessary in order to add new global authentication providers.

One simple idea berdir had in mind which we could start with is to put the default into a container parameter, so you can override it much easier.

That works well for per-site use-cases. It does not help with contrib modules providing new default providers. Perhaps this should be configuration instead?

1. +++ b/core/core.services.yml
   @@ -7,6 +7,7 @@ parameters:
   +  authentication.global_providers: {}
   
   @@ -1200,6 +1201,7 @@ services:
   +    arguments: ['%authentication.global_providers%']
   
   +++ b/core/lib/Drupal/Core/Authentication/AuthenticationManager.php
   @@ -75,7 +75,7 @@ class AuthenticationManager implements AuthenticationProviderInterface, Authenti
   -  public function __construct($global_providers = ['cookie' => TRUE]) {
   +  public function __construct($global_providers = []) {
        $this->globalProviders = $global_providers;
      }
   

   I think we do not need the container parameter if we can set the global flag on the tag. That means we could remove the constructor completely.

2. +++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/TaggedHandlersPass.php
   @@ -94,16 +94,20 @@ public function process(ContainerBuilder $container) {
   +        $other_params = [];
   

   I'd use $extra_params or $additional_params as the name.

3. +++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/TaggedHandlersPass.php
   @@ -94,16 +94,20 @@ public function process(ContainerBuilder $container) {
   -          if ($param->getName() == 'id') {
   +          else if ($param->getName() == 'id') {
   

   We still need to use if here because the preceding condition is targeting the type-hint, not the parameter name. Also change the conditions to use the identity operator (===), instead of equality.

Oh, and we probably should add test coverage to TaggedHandlersPassTest.

And we probably should add back the AuthenticationManagerInterface because of the addProvider() method. I've removed it prematurely.

Thanks! Test coverage looks great now for the TaggedServiceProvider.

At devdays we also discuss the weird magic we use to derive the provider_id from the service name (i.e. the service id of an authentication provider must follow the schema authentication.provider_id and thus cannot be prefixed with the module name). A better way to resolve that problem would be to add another extra parameter provider_id and explicitly specify it in every service definition. E.g.:

  user.authentication.cookie:
    class: Drupal\user\Authentication\Provider\Cookie
    arguments: ['@session_configuration', '@database']
    tags:
      - { name: authentication_provider, provider_id: 'cookie', priority: 0, global: TRUE }

Thanks to the new extra parameters it will likely be enough to simply rename the $id paramater to $provider_id in AuthenticationManager::addProvider. Thought that might be a BC break. If we want to avoid this, then we'd need to add the an optional $provider_id parameter and initialize it with the current magic if NULL.

An explicit $provider_id on the service definition also would make it simpler for contrib to collect all provider ids (e.g. #2456283: AuthenticationManager::getSortedProviders is protected).

Great!

+++ b/core/lib/Drupal/Core/Authentication/AuthenticationManagerInterface.php
@@ -0,0 +1,29 @@
+  /**
+   * Adds a provider to the array of registered providers.
+   *
+   * @param \Drupal\Core\Authentication\AuthenticationProviderInterface $provider
+   *   The provider object.
+   * @param string $provider_id
+   *   Identifier of the provider.
+   * @param int $priority
+   *   The providers priority.
+   * @param bool $global
+   *   TRUE if the provider is to be applied globally on all routes.
+   */
+  public function addProvider(AuthenticationProviderInterface $provider, $priority = 0, $provider_id, $global = FALSE);

Can you modify the function signature such that mandatory $provider_id is before the optional $priority? Also specify in the @param that $priority and $global are not required (prefix with (optional)).

+++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/TaggedHandlersPass.php
@@ -148,6 +157,9 @@ public function process(ContainerBuilder $container) {
           ksort($arguments);
+          if (isset($extra_arguments[$id])) {
+            $arguments += $extra_arguments[$id];
+          }

I think we may want to merge the arguments before sorting.

1. +++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/TaggedHandlersPass.php
   @@ -125,6 +130,10 @@ public function process(ContainerBuilder $container) {
   +            $extra_arguments[$id][$pos] = isset($attributes[0][$name]) ? $attributes[0][$name] : NULL;
   

   I suspect that this overrides the default value of an optional extra paremeter. That parameter might then receive NULL instead of the default value.

2. +++ b/core/tests/Drupal/Tests/Core/DependencyInjection/Compiler/TaggedHandlersPassTest.php
   @@ -242,6 +364,16 @@ class InvalidConsumer {
   +  public function addHandler(HandlerInterface $instance, $priority = 0, $extra1, $extra2) {
   ...
   +  public function addWithId(HandlerInterface $instance, $id, $priority = 0, $extra1, $extra2) {
   ...
   +  public function addWithDifferentOrder(HandlerInterface $instance, $extra1, $priority = 0, $extra2) {
   

   I do not intend to hold off the patch but it is a bit weird to have mandatory parameters after optional ones. In fact that makes the optional ones mandatory also. Perhaps add extra paremeters with default values?

I removed this from the IS scope considering that is is easy to swap out the authentication manager service with a single authentication provider where needed.

In fact it would be nice if we did that automatically. This might be easy to implement with a service alias: By default authentication points to user.authentication.cookie. If number of services with the authentication_provider tag is > 1, then authentication points to authentication.manager. This could be easily be implemented in a simple additional compiler pass.

The benefit would be that the wast majority of sites would automatically run without the authentication manager.

So, I profiled how much #32 would bring us performance wise by hard-wiring the cookie authentication into authentication service by hand. In fact we save 65 function calls and 30 kilobyte for authenticated users (wall time report does not look reliable). Difference for anonymous users was zero function calls. I had to hack xhprof-kit index-perf.php in order to inject the session cookie directly via $_COOKIE superglobal.

=== baseline-8.0.x..8.0.x compared (553a3c1b20302..553a3c4aea704):

ct  : 43,786|43,786|0|0.0%
wt  : 126,756|128,081|1,325|1.0%
mu  : 19,262,960|19,262,960|0|0.0%
pmu : 19,428,496|19,428,496|0|0.0%

=== baseline-8.0.x..8.0.x-hard-wired-cookie compared (553a3c1b20302..553a3c6029bec):

ct  : 43,786|43,721|-65|-0.1%
wt  : 126,756|126,741|-15|-0.0%
mu  : 19,262,960|19,232,000|-30,960|-0.2%
pmu : 19,428,496|19,396,272|-32,224|-0.2%

That said I think the performance gain is too little to justify adding another compiler pass.

The changes in TaggedHandlerPass are fully covered by unit tests, this is good to go.

Updated the issue summary and issue title.

I think we do not need a new change record for that, but update: https://www.drupal.org/node/2453449

AFAIK we do change record updates after commit, therefore resetting to RTBC.

Ok, thanks. I've added the pending api changes/additions to the change record.

Thanks. Updated and (re-)published the change record.