thephpleague / container

Small but powerful dependency injection container
http://container.thephpleague.com
MIT License
844 stars 102 forks source link

Potential bug with composing and composed dependencies both aliased with interfaces #265

Open elazar opened 5 days ago

elazar commented 5 days ago

This may admittedly be a misunderstanding on my part regarding the expected behavior of this library.

See this branch. I've tried running the test case that it adds against the 4.x branch, the 4.2.3 tag, and the branch from #259 (leading me to believe this isn't an issue with recursive dependency resolution), but all yield the same result. See the test failure output against my branch (which is based on the 4.x branch) shown below.

➜  container git:(nested-interface-dependencies) ./vendor/bin/phpunit --filter testContainerAddsAndGetsNestedInterfaceDependency tests/ContainerTest.php

PHPUnit 8.5.40 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.2.24 with Xdebug 3.3.1
Configuration: container/phpunit.xml
Error:         This version of PHPUnit does not support code coverage on PHP 8

E                                                                   1 / 1 (100%)

Time: 106 ms, Memory: 6.00 MB

There was 1 error:

1) League\Container\Test\ContainerTest::testContainerAddsAndGetsNestedInterfaceDependency
ArgumentCountError: Too few arguments to function League\Container\Test\Asset\Bay::__construct(), 0 passed and exactly 1 expected

container/tests/Asset/Bay.php:11
container/src/Definition/Definition.php:212
container/src/Definition/Definition.php:175
container/src/Definition/Definition.php:154
container/src/Definition/DefinitionAggregate.php:79
container/src/Container.php:161
container/src/Container.php:111
container/tests/ContainerTest.php:33

If I pass the $container variable in the test to var_dump() immediately before the call to its get() method that causes the test failure, this is the output.

container/tests/ContainerTest.php:32:
class League\Container\Container#324 (5) {
  protected $defaultToShared =>
  bool(false)
  protected $definitions =>
  class League\Container\Definition\DefinitionAggregate#296 (2) {
    protected $definitions =>
    array(3) {
      [0] =>
      class League\Container\Definition\Definition#16 (8) {
        protected $alias =>
        string(40) "League\Container\Test\Asset\BayInterface"
        protected $concrete =>
        string(31) "League\Container\Test\Asset\Bay"
        protected $shared =>
        bool(false)
        protected $tags =>
        array(0) {
        }
        protected $arguments =>
        array(0) {
        }
        protected $methods =>
        array(0) {
        }
        protected $resolved =>
        NULL
        protected $container =>
        NULL
      }
      [1] =>
      class League\Container\Definition\Definition#17 (8) {
        protected $alias =>
        string(40) "League\Container\Test\Asset\BazInterface"
        protected $concrete =>
        string(31) "League\Container\Test\Asset\Baz"
        protected $shared =>
        bool(false)
        protected $tags =>
        array(0) {
        }
        protected $arguments =>
        array(0) {
        }
        protected $methods =>
        array(0) {
        }
        protected $resolved =>
        NULL
        protected $container =>
        NULL
      }
      [2] =>
      class League\Container\Definition\Definition#23 (8) {
        protected $alias =>
        string(31) "League\Container\Test\Asset\Bay"
        protected $concrete =>
        string(31) "League\Container\Test\Asset\Bay"
        protected $shared =>
        bool(false)
        protected $tags =>
        array(0) {
        }
        protected $arguments =>
        array(1) {
          [0] =>
          string(40) "League\Container\Test\Asset\BazInterface"
        }
        protected $methods =>
        array(0) {
        }
        protected $resolved =>
        NULL
        protected $container =>
        NULL
      }
    }
    protected $container =>
          ...

  }
  protected $providers =>
  class League\Container\ServiceProvider\ServiceProviderAggregate#25 (3) {
    protected $providers =>
    array(0) {
    }
    protected $registered =>
    array(0) {
    }
    protected $container =>
          ...

  }
  protected $inflectors =>
  class League\Container\Inflector\InflectorAggregate#22 (2) {
    protected $inflectors =>
    array(0) {
    }
    protected $container =>
          ...

  }
  protected $delegates =>
  array(0) {
  }
}

This behavior appears to be specific to these circumstances.

  1. One concrete class that implements an interface has a required constructor parameter that implements another interface.
  2. The container includes aliases from each of the two interfaces to their respective concrete class implementations and the argument of the composing class constructor.
philipobenito commented 5 days ago

Hi,

It is actually by design, to avoid situations that could result in recursion hell, currently it doesn't expand to find another alias for the concrete argument, it just resolves it.

Does that make sense? Essentially the container isn't even considering your definition with the alias using the concrete class name, it expects explicitness.

There are ways to do it, like using a closure factory for the concrete argument and resolving from the container again.

That being said, I'm now considering a way this might be able to be done, but it would be something I look at for the 5.x release I'm working on.

Sent from Proton Mail Android

-------- Original Message -------- On 26/10/2024 02:52, Matthew Turland wrote:

This may admittedly be a misunderstanding on my part regarding the expected behavior of this library.

See this branch. I've tried running the test case that it adds against the 4.x branch, the 4.2.3 tag, and the branch from #259 (leading me to believe this isn't an issue with recursive dependency resolution), but all yield the same result. See the test failure output against my branch (which is based on the 4.x branch) shown below.

➜ container git:(nested-interface-dependencies) ./vendor/bin/phpunit --filter testContainerAddsAndGetsNestedInterfaceDependency tests/ContainerTest.php

PHPUnit 8.5.40 by Sebastian Bergmann and contributors.

Runtime: PHP 8.2.24 with Xdebug 3.3.1 Configuration: container/phpunit.xml Error: This version of PHPUnit does not support code coverage on PHP 8

E 1 / 1 (100%)

Time: 106 ms, Memory: 6.00 MB

There was 1 error:

1) League\Container\Test\ContainerTest::testContainerAddsAndGetsNestedInterfaceDependency ArgumentCountError: Too few arguments to function League\Container\Test\Asset\Bay::__construct(), 0 passed and exactly 1 expected

container/tests/Asset/Bay.php:11 container/src/Definition/Definition.php:212 container/src/Definition/Definition.php:175 container/src/Definition/Definition.php:154 container/src/Definition/DefinitionAggregate.php:79 container/src/Container.php:161 container/src/Container.php:111 container/tests/ContainerTest.php:33

If I pass the $container variable in the test to var_dump() immediately before the call to its get() method that causes the test failure, this is the output.

container/tests/ContainerTest.php:32: class League\Container\Container#324 (5) { protected $defaultToShared => bool(false) protected $definitions => class League\Container\Definition\DefinitionAggregate#296 (2) { protected $definitions => array(3) { [0] => class League\Container\Definition\Definition#16 (8) { protected $alias => string(40) "League\Container\Test\Asset\BayInterface" protected $concrete => string(31) "League\Container\Test\Asset\Bay" protected $shared => bool(false) protected $tags => array(0) { } protected $arguments => array(0) { } protected $methods => array(0) { } protected $resolved => NULL protected $container => NULL } [1] => class League\Container\Definition\Definition#17 (8) { protected $alias => string(40) "League\Container\Test\Asset\BazInterface" protected $concrete => string(31) "League\Container\Test\Asset\Baz" protected $shared => bool(false) protected $tags => array(0) { } protected $arguments => array(0) { } protected $methods => array(0) { } protected $resolved => NULL protected $container => NULL } [2] => class League\Container\Definition\Definition#23 (8) { protected $alias => string(31) "League\Container\Test\Asset\Bay" protected $concrete => string(31) "League\Container\Test\Asset\Bay" protected $shared => bool(false) protected $tags => array(0) { } protected $arguments => array(1) { [0] => string(40) "League\Container\Test\Asset\BazInterface" } protected $methods => array(0) { } protected $resolved => NULL protected $container => NULL } } protected $container => ...

} protected $providers => class League\Container\ServiceProvider\ServiceProviderAggregate#25 (3) { protected $providers => array(0) { } protected $registered => array(0) { } protected $container => ...

} protected $inflectors => class League\Container\Inflector\InflectorAggregate#22 (2) { protected $inflectors => array(0) { } protected $container => ...

} protected $delegates => array(0) { } }

This behavior appears to be specific to these circumstances.

  • One concrete class that implements an interface has a required constructor parameter that implements another interface.
  • The container includes aliases from each of the two interfaces to their respective concrete class implementations and the argument of the composing class constructor.

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you are subscribed to this thread.Message ID: @.***>

spider-mane commented 1 day ago

@philipobenito I'm not sure I follow here. The pull request #254 that removed support for recursive resolution had the stated intention of preventing infinite loops when provided with a nonexistent class. Before that PR, recursive resolution was fully supported and I've had to fix my projects to version 4.2.0 in order to avoid problems. The pull request that I submitted #259 restores support for recursion while also preventing infinite loops in the event that the class provided does not exist. I don't quite get the rationale you've provided here against recursion, but either way #254 on its own introduced breaking changes that I don't think should have been included in a patch release.