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

Factories ignore default parameters #526

Closed
Khartir opened this Issue Oct 3, 2017 · 7 comments

Comments

Projects
None yet
4 participants
@Khartir

Khartir commented Oct 3, 2017

When using a factorie for a parameter the default values are ignored. Instead the container or the definition is injected.
Example:

$builder = new \DI\ContainerBuilder();
$builder->addDefinitions([
	'test' => \DI\factory('htmlentities')->parameter('string', '<something>'),
]);
$container = $builder->build();
echo $container->get('test');

This fails with the following exception:
DI\Definition\Exception\InvalidDefinition: Entry "test" cannot be resolved: Unable to invoke the callable because no value was given for parameter 3 ($encoding)
Parameter 2 is not mentioned because the container and the definition are injected as per this test:

class test {

	public function dummy($a = null, $b = null) {
		echo get_class($a) . PHP_EOL;
		echo get_class($b);
	}
}
$builder = new \DI\ContainerBuilder();
$builder->addDefinitions([
	'factory_test' => \DI\factory(['test', 'dummy']),
]);
$container = $builder->build();
$test = $container->get('factory_test');

This produces:
DI\Container DI\Definition\FactoryDefinition

@ccazette

This comment has been minimized.

ccazette commented Apr 9, 2018

Ran into this as well. Frustrating.

@mnapoli

This comment has been minimized.

Member

mnapoli commented Apr 9, 2018

Going over the original report I'm not sure I completely understand the problem. @ccazette could you explain it in your own words?

@ccazette

This comment has been minimized.

ccazette commented Apr 9, 2018

Sure. In short, you have to specify all factory parameters using DI\factory(...)->parameter(), even if the factory parameter is actually optional. Otherwise, you get an InvalidDefinition error, as per the report above.
In my situation, I end up having my definition files cluttered with a bunch of ->parameter('myOptionalParam', null) to circumvent the issue, as they HAVE to be passed. Does that help ?

@mmelvin0

This comment has been minimized.

mmelvin0 commented Apr 12, 2018

Also, non-optional parameters are treated as required in factories. Here's a short concrete example of both, since they are maybe the same issue.

I have this class that returns named database connections. The idea is this class is used in different applications, some of which have multiple database adapters, some of which have only one. It returns either a specific named connection or the application's default connection.

class ConnectionManager {

    public function getConnection($name = null) {
        // logic to fetch named connection, using default name if $name is null
    }

    public function __invoke(ContainerInterface $container, RequestedEntry $entry, $name = null) {
        return $this->getConnection();
    }

}

I've tried the following two definitions for injecting a connection into Service as constructor parameter $connection.

[Service::class => DI\autowire()->constructorParameter('connection', DI\factory([ConnectionManager::class, 'getConnection']))]

This fails since FactoryResolver passes ContainerInterface and RequestedEntry as the 1st and 2md arguments respectively to the factory method in the absence of any parameters in FactoryDefinition.

This sort-of makes sense since the assumption is that a factory callable needs something passed to it in order to resolve its definitions, but I think it would less magical if they were not be passed at all if they aren't type hinted.

[Service::class => DI\autowire()->constructorParameter('connection', DI\factory(ConnectionManager::class))]

This fails with: DI\Definition\Exception\InvalidDefinition: Entry "Service" cannot be resolved: Entry "<nested definition>" cannot be resolved: Unable to invoke the callable because no value was given for parameter 3 ($name).

This does not make sense because the parameter is clearly optional, and therefore should not be treated as required by FactoryResolver. Changing this behavior would be consistent with how PHP-DI treats optional parameters everywhere else.

Of course this can be trivially worked around by with parameter('name', null), but that's back to @Khartir's point.

Here is the code in question:

try {
$providedParams = [$this->container, $definition];
$extraParams = $this->resolveExtraParams($definition->getParameters());
$providedParams = array_merge($providedParams, $extraParams);
return $this->invoker->call($callable, $providedParams);
} catch (NotCallableException $e) {

@mnapoli

This comment has been minimized.

Member

mnapoli commented Apr 21, 2018

Thank you both for the details and thanks for all the pointers @mmelvin0 that helped!

I've opened #597 with a test and a fix.

@mnapoli mnapoli closed this in 05aab9c Apr 21, 2018

mnapoli added a commit that referenced this issue Apr 21, 2018

Merge pull request #597 from PHP-DI/fix-526
Fix #526 Support optional parameters in factories
@mmelvin0

This comment has been minimized.

mmelvin0 commented Apr 22, 2018

I just wanted to mention the ultimate fix for this is very elegant. That commit makes it clear that the behavior described in this issue was just a small oversight and not a design deficiency.

PHP-DI is an extraordinarily well-designed piece of software IMHO.

@mnapoli

This comment has been minimized.

Member

mnapoli commented Apr 22, 2018

😄 thanks! To be honest not all bugs are like that but yep I really enjoyed that fix ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment