Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Throw an exception if unknown parameter passed to a method #96

Open
BoShurik opened this issue May 16, 2024 · 4 comments
Open

Throw an exception if unknown parameter passed to a method #96

BoShurik opened this issue May 16, 2024 · 4 comments

Comments

@BoShurik
Copy link

What steps will reproduce the problem?

class Foo
{
    public function __construct(public Bar $bar)
    {
    }
}

class Bar
{
    public function __construct(public string $value = 'default')
    {
    }
}

$config = ContainerConfig::create()
    ->withDefinitions([
        Foo::class => [
            'class' => Foo::class,
            '__construct()' => [
                'baz' => Reference::to('bar'), // Typo here
            ],
        ],
        'bar' => [
            'class' => Bar::class,
            '__construct()' => [
                'value' => 'custom',
            ],
        ],
    ]);

$container = new Container($config);

var_dump($container->get(Foo::class));

What is the expected result?

Exception with message, like:
Unknown parameter $baz in Foo::__construct() method. Available paraemeters: $bar

What do you get instead?

New instance of Foo with wrong Bar

class Foo#11 (1) {
  public Bar $bar =>
  class Bar#10 (1) {
    public string $value =>
    string(7) "default"
  }
}

Additional info

Q A
Version master
PHP version N/A
Operating system N/A
@vjik
Copy link
Member

vjik commented May 16, 2024

This check should only be performed when validation is enabled.

@viktorprogger
Copy link

viktorprogger commented May 16, 2024

@vjik I'm not agree with you. It's misconfiguration: when you try to set a non-existent constructor parameter, container should throw an exception. Instead it passes a value to a random existent parameter. This behavior is not intuitive. It leads to bugs, which are very difficult to solve.
For me these two cases are equal, so their behavior should be equal too (throwing of an exception):

// Container definition
Foo::class => [
  '__construct()' => ['baz' => true],
],

// Direct object creation
new Foo(baz: true);

@BoShurik
Copy link
Author

The problem with such definition is that you will get the exception only if you try to get an instance. Validation can catch it on container initialisation

both-is-good-both

@vjik
Copy link
Member

vjik commented May 16, 2024

Instead it passes a value to a random existent parameter. This behavior is not intuitive. It leads to bugs, which are very difficult to solve.

That's not how container works. Currently, if you pass a non-existent argument, then it will be ignored.

I mistakenly thought, that for implementation we will need to use extra reflection (what leads to a decrease in performance), but no. Add compare count used arguments here is enough. Seems, it is cheap operation.

Validation can catch it on container initialisation

It's good idea. But won't it be too long even for the development environment? Theoretically we can make it as separate method, and then use it in CI.

@vjik vjik transferred this issue from yiisoft/di May 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants