laminas / laminas-di

Automated dependency injection for PSR-11 containers
https://docs.laminas.dev/laminas-di/
BSD 3-Clause "New" or "Revised" License
36 stars 20 forks source link

Adjust `Injector::create()` to ignore interfaces (which cannot be instantiated anyway) #38

Closed sjokkateer closed 2 years ago

sjokkateer commented 2 years ago

Signed-off-by: Remy Bos 27890746+sjokkateer@users.noreply.github.com

Q A
Documentation no
Bugfix no
BC Break no
New Feature no
RFC no
QA no

Description

Injector::create#L166-171 contains the following check:

if (! class_exists($class) || interface_exists($class)) {
    throw new ClassNotFoundException(sprintf(
        'Class or interface by name %s does not exist',
        $class
    ));
}

Given the added test (which passes/satisfies), a ClassNotFoundException::class is thrown. This is the case because the if guard short circuits (example #1) since ! class_exists($class) will always be true for any given interface. Therefor it seems that the part || interface_exists($class) could hence be removed (all tests would still pass equally).

My main confusion with this piece of code lies in the fact that it seems as if it was meant for an interface to get up to that point, based on the given if guard and exception message. While everything following the conditional indicates it not making sense. Since after the conditional, only parameters for a class will be resolved and an attempt is made to construct a class (which would ofcourse result in an error for an interface).

This confusion would be resolved by removing the || interface_exists($class) and adjusting the message of the exception that is thrown.

I have made the pull request a WIP in case you have a different opinion than the suggested. Please let me know what you think.

Ocramius commented 2 years ago

Absolute ref:

https://github.com/laminas/laminas-di/blob/1abf2cc63db68fd0f351426e07810ada531c5ada/src/Injector.php#L142-L176

@sjokkateer from my PoV, new $class(...) where $class is an interface makes absolutely no sense.

Your test should be fine, we just need to drop part of the conditional then (can be done here) :)

Also, I suggest rebasing to get your build to pass.

Thanks for investigating!