Container::has() doesn't work for service aliases (consequently, decorating services doesn't work)

Wow, looks nice.

Note: ContainerBuilder::getDefinition is broken with aliases too. See TestServiceProvider. Someone should fix that perhaps too.

Yeah, IMHO we should fix both here and check whether our implementation of aliases is actually properly in general.

Yes, that bug is correct.

I had found this while porting back to service_container module, but forgot to create an issue before X-MAS break.

Sorry :/

https://github.com/LionsAd/service_container/pull/97/files#diff-24218d4b...

is the correct line - all other alias implementations are correct - I checked that.

@chx: But Drupal\Core\DependencyInjection has no getDefinition() override for Symfony\Component\DependencyInjection\Container. Do you mean we need to fix Symfony? What am I missing?

#10: Yes, getDefinition() will need to be fixed or won't fix upstream.

All right, are you suggesting we fix it on our side and open an upstream issue?

So I looked into solving this. (Not sure this would be a novice by any chance). First let's define what we want. Our Container does only one level of alias resolution, eg:

  public function get($id, $invalid_behavior = ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE) {
    if (isset($this->aliases[$id])) {
      $id = $this->aliases[$id];
    }

The patch suggest one level of alias resolution also in has(). Based on TestServiceProvider that chx pointed out, we would need infinite level alias resolution, something like this:

  /**
   * {@inheritdoc}
   */
  public function getDefinition($id) {
    while ($this->hasAlias($id)) {
      $id = (string) $this->getAlias($id);
    }
    parent::getDefinition($id);
  }

TestServiceProvider had this code for a similar purpose:

        for ($id = $original_id; $container->hasAlias($id); $id = (string) $container->getAlias($id));

Which one do we want to do? One level like all the container does at many places or multilevel, in which case we need to fix a lot more.

Don't base your decisions on TestServiceProvider ; it's just something I hacked together. But note

        if (!array_key_exists($id, $this->definitions) && isset($this->aliasDefinitions[$id])) {
            return $this->get($this->aliasDefinitions[$id]);
        }

this is recursive in ContainerBuilder::get. Whether it's an accident or intentional, who knows. Certainly Container::get is not recursive.

Recommendation: don't be recursive.

All right, added an override for getDefinition() and updated the test. I did not find a way to get the id from the definition, it does not seem to have a method for it sadly, so I kept the alias resolver for the id but not recursively anymore to mirror the resolver in getDefinition(). Let's see if this works :)

The D8 committers all agreed that this issue is a major bug. Thanks everyone for working on this.

Any reviews, comments? :) With the modified test service provider I believe we have full test coverage.

Alright :)

With the modified test service provider I believe we have full test coverage.

Seriously, are we really falling back into the old days of having implicit test coverage. I guess we want to hate us in the future again.

1. +++ b/core/lib/Drupal/Core/DependencyInjection/ContainerBuilder.php
   @@ -95,4 +95,15 @@ public function __sleep() {
   +  public function getDefinition($id) {
   +    $id = strtolower($id);
   

   Clearly, we should document why we override the method to document that the symfony parent resolves parameters on compile time only, see https://github.com/symfony/symfony/issues/17110#issuecomment-166631116 for example

2. +++ b/core/lib/Drupal/Core/DependencyInjection/ContainerBuilder.php
   @@ -95,4 +95,15 @@ public function __sleep() {
   +    if ($this->hasAlias($id)) {
   +      $id = $this->getAlias($id);
   ...
   +    return parent::getDefinition($id);
   

   Don't we want to recursively go through aliases as the code in TestServiceProvider did? On top of that \Drupal\Tests\Core\DependencyInjection\ContainerBuilderTest needs its test coverage

3. +++ b/core/modules/simpletest/src/TestServiceProvider.php
   @@ -33,13 +33,13 @@ function register(ContainerBuilder $container) {
          foreach (['router.route_provider' => 'RouteProvider'] as $original_id => $class) {
   

   Someone should add a novice issue to get rid of that insanity.

4. +++ b/core/modules/simpletest/src/TestServiceProvider.php
   @@ -33,13 +33,13 @@ function register(ContainerBuilder $container) {
   +        $id = $container->hasAlias($original_id) ? $container->getAlias($original_id) : $original_id;
   

   Mh, so getAlias() returns an \Symfony\Component\DependencyInjection\Alias instance ...

I guess we want to hate us in the future again.

Yes, I long for that future.

Don't we want to recursively go through aliases as the code in TestServiceProvider did?

We discussed this above. I think you have a different opinion. Where does the container do recursive resolution?

Thanks, Wim.

I am taking a look what is missing here now.

The recursive alias resolving should not be needed, because the dumpers do end-to-end alias resolution.

However it might be a bug in ContainerBuilder upstream, too - that only one level is resolved.

I think #5 and #6 do not belong in this issue at all.

I unfortunately had forgotten about it as the Test and Patch had been so straightforward, so I had assumed this had been committed a long time ago, already.

If someone calls getDefinition(), it is their responsibility to ensure the alias-chain is resolved.

If someone calls has() it is the containers responsibility to check that if there is an alias then the container has the service.

So while #2 is a straightforward bug fix, the getDefinition() is a feature that should be rather be put in as a feature request upstream.

=>

RTBC for #2.

My only nit is that the aliases check should come first, so I will upload a patch to do that. (Just so that it matches the Symfony order)

RTBC - just changed the order.

To demonstrate , can we kill most of TestServiceProvider ? Pretty please? It's so much better when we actually have a demo of how to get things done.

#26: TestServiceProvider is KernelTestBase specific and has nothing to do with this.

We can however open a new issue for it.

+++ b/core/lib/Drupal/Component/DependencyInjection/Container.php
@@ -366,7 +366,7 @@ public function set($id, $service, $scope = ContainerInterface::SCOPE_CONTAINER)
+    return isset($this->aliases[$id]) || isset($this->services[$id]) || isset($this->serviceDefinitions[$id]) || array_key_exists($id, $this->services);

How come the array_key_exists() is being added?

I guess we're copying symfony...

    public function has($id)
    {
        for ($i = 2;;) {
            if ('service_container' === $id
                || isset($this->aliases[$id])
                || isset($this->services[$id])
                || array_key_exists($id, $this->services)
            ) {
                return true;
            }
            if (--$i && $id !== $lcId = strtolower($id)) {
                $id = $lcId;
            } else {
                return method_exists($this, 'get'.strtr($id, $this->underscoreMap).'Service');
            }
        }
    }

If we're going to implement the array_key_exists() check then I think a service in the container with a NULL value needs to be tested - since this is the difference between that method and the isset(). Personally I think this issue should go in without that and a separate issue should be made to discuss the array_key_exists().

Our Container does not contain this snippet from Symfony Container:

        if (null === $service) {
            if (self::SCOPE_CONTAINER !== $scope) {
                unset($this->scopedServices[$scope][$id]);
            }

            unset($this->services[$id]);
        }

I know I used a NULL set before to remove a service... but definitely other parts of Symfony Container use array_key_exists. So there's no good guidance from the Symfony side. Add an unset() ? Definitely a separate issue.

Good catch - I missed that I had changed more than the order, back to RTBC.

I'll open a follow-up for https://github.com/symfony/dependency-injection/commit/6bd9ea28a6ba75227... and https://github.com/symfony/dependency-injection/commit/a875db7ae4f22cd21... to potentially sync that in.

Those are the only relevant changes since the container was committed.

--

And yes the array_key_exists() was totally wrong here. We skip that check for performance reasons and inherently assume that NULL == 'service does not exist and needs to be re-created'. Thanks Alex and chx.

Which is fine, because if one studies the Symfony code closely it can be seen that $services[$key] can never be NULL. So the check is superfluous.

Here is the right patch.

Committed/pushed to 8.2.x and cherry-picked to 8.1.x. Thanks!

Note that even after this the alias feature is of very limited use because it doesn't move/merge service tags and although the new .inner service is set to non-public that does not help because findTaggedServiceIds doesn't check for isPublic . So if the original service was tagged with something then a) the decorated service won't be tagged appropriately b) even if you tag the outer service (the decorator) manually, findTaggedServiceIds will find the inner service which is very unlikely to be what you want.

This should at least be documented somewhere or perhaps an upstream patch submitted to make this feature more useful.

Until then, you can follow the code example in TestServiceProvider. As usual, Symfony is a poor fit to Drupal.