yiisoft / injector

PSR-11 compatible injector
https://www.yiiframework.com/
BSD 3-Clause "New" or "Revised" License
43 stars 18 forks source link

Injector cannot resolve non-instantiable argument with a default value #30

Closed mnavarrocarter closed 3 years ago

mnavarrocarter commented 3 years ago

What steps will reproduce the problem?

Create a method with a typed object with a private constructor (the object is only instantiable via named constructors) but set the default argument to null. Example:

class SomeService 
{
    public function __construct(SetCookie $cookie = null) { ... }
}

Then, call Injector::make on the class name.

What is the expected result?

I expect the injector to be able to resolve even when the class cannot be instantiated since there is a default value that can be passed.

What do you get instead?

I get an error saying that the class cannot be instantiated.

Additional info

Q A
Version 1.0.2
PHP version 7.4
Operating system ubuntu

I can implement the improvement in a PR if you are willing to merge it. I think this is a very reasonable expectation.

samdark commented 3 years ago

Would you please pull request a failing test? It's not completely clear about your case. You have a public constructor in your example but you're talkin about private one.

mnavarrocarter commented 3 years ago

@samdark Thanks for your suggestion. Writing the test case I realised it passes, so there is something else I'm doing wrong on my end. Thanks for your time!

mnavarrocarter commented 3 years ago

Well, having said that I think this call could be improved:

    /**
     * @param ResolvingState $state
     * @param null|string $class
     * @param bool $isVariadic
     * @return bool True if argument resolved
     * @throws ContainerExceptionInterface
     */
    private function resolveObjectParameter(ResolvingState $state, ?string $class, bool $isVariadic): bool
    {
        $found = $state->resolveParameterByClass($class, $isVariadic);
        if ($found || $isVariadic) {
            return $found;
        }
        if ($class !== null) {
            // Maybe we should catch a possible exception from the inner container and attempt to fallback to something else?
            $argument = $this->container->get($class);
            $state->addResolvedValue($argument);
            return true;
        }
        return false;
    }

Maybe we should catch a possible exception from the inner container get() call and attempt to fallback to something else, like a possible default argument?

I acknowledge the fact that this is very particular to my issue, but exceptions are documented in the PSR-11 spec so maybe is not a good idea to ignore them.

samdark commented 3 years ago

@mnavarrocarter would you please pull request a test with your specific case? It is still not clear what is it about.

roxblnfk commented 3 years ago

Maybe we should catch a possible exception from the inner container get() call and attempt to fallback to something else, like a possible default argument?

It happens on a higher level https://github.com/yiisoft/injector/blob/4f89e76c433a7bf28b516705973ab0d158b092e9/src/Injector.php#L184-L188

mnavarrocarter commented 3 years ago

@samdark @roxblnfk Don't mind me. There is not a problem or bug with this. :)