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

Errors when parameters are passed by reference #306

Closed
bradynpoulsen opened this Issue Sep 1, 2015 · 3 comments

Comments

Projects
None yet
2 participants
@bradynpoulsen
Contributor

bradynpoulsen commented Sep 1, 2015

DI\Container::make fails silently and returns null when provided argument is not provided by reference, but the dependency expects it to be passed by reference. In example

class A
{
}

class B
{
    public function __construct(A &$aInstance, SomeOtherDependencies $baz)
    {
    }
}

$foo = $container->get('A');
// do some stuff to $foo
try
{
    $bar = $container->make('B', [
        'aInstance' => $foo
    ]);
}
catch(DependencyException $e)
{
    // never gets fired
}
echo is_object($bar) ? get_class($bar) : gettype($bar); // prints 'NULL'

$barReal = $container->make('B', [
    'aInstance' => &$foo
]);
echo is_object($bar) ? get_class($bar) : gettype($bar); // prints 'B'

Whereas, DI\FactoryInterface defines that a DI\DependencyException will be thrown when there is an error while resolving the entry; however, the exception is not thrown

Whereas, \ReflectionParameter::isPassedByReference() is available to check if the parameter should be passed by reference

Therefore,

  1. DI\Container::make should throw a DI\DependencyException if DI\Definition\Resolver\ObjectCreator::createInstance is unable to provide an object of a class after calling \ReflectionClass::newInstanceArgs(array $args)
  2. I believe the DI\Definition\Resolver\ParameterResolver::resolveParameters() should check if \ReflectionParameter::isPassedByReference() is true
@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Sep 1, 2015

Member

Hi, thanks for the report.

  1. DI\Container::make should throw a DI\DependencyException

Yep definitely agree. I have to admit I have never used the container with parameters passed by reference, I'm surprised it doesn't even throw an exception but I've never tested it :/

Ideally though it should work and not throw an exception or return null.

  1. I believe the DI\Definition\Resolver\ParameterResolver::resolveParameters() should check if \ReflectionParameter::isPassedByReference() is true

to check if the parameter should be passed by reference

The parameter doesn't need to be passed by reference when creating the instance. It's already marked with & in the method declaration, it just needs to be passed as a normal parameter. Maybe it doesn't work because it's using the reflection… I need to look into it, I can't say right now. See also this example:

class A {
    public $foo;
}

class B {
    public function __construct(A &$a) {
        $a->foo = 'hello';
    }
}

$a = new A();
$b = new B($a);
echo $a->foo; // hello
Member

mnapoli commented Sep 1, 2015

Hi, thanks for the report.

  1. DI\Container::make should throw a DI\DependencyException

Yep definitely agree. I have to admit I have never used the container with parameters passed by reference, I'm surprised it doesn't even throw an exception but I've never tested it :/

Ideally though it should work and not throw an exception or return null.

  1. I believe the DI\Definition\Resolver\ParameterResolver::resolveParameters() should check if \ReflectionParameter::isPassedByReference() is true

to check if the parameter should be passed by reference

The parameter doesn't need to be passed by reference when creating the instance. It's already marked with & in the method declaration, it just needs to be passed as a normal parameter. Maybe it doesn't work because it's using the reflection… I need to look into it, I can't say right now. See also this example:

class A {
    public $foo;
}

class B {
    public function __construct(A &$a) {
        $a->foo = 'hello';
    }
}

$a = new A();
$b = new B($a);
echo $a->foo; // hello

@mnapoli mnapoli added the bug label Sep 1, 2015

@mnapoli mnapoli added this to the 5.1 milestone Sep 1, 2015

@bradynpoulsen

This comment has been minimized.

Show comment
Hide comment
@bradynpoulsen

bradynpoulsen Sep 1, 2015

Contributor

Following up on checking \ReflectionParameter::isPassedByReference():

DI\Definition\Resolver\ParameterResolver has multiple locations that it would loose it's reference. Please see lines #62, #65, and #91.

Is my understanding of those assignment statements in PHP correct? I apologize that I didn't clarify that in my initial suggestion.

Contributor

bradynpoulsen commented Sep 1, 2015

Following up on checking \ReflectionParameter::isPassedByReference():

DI\Definition\Resolver\ParameterResolver has multiple locations that it would loose it's reference. Please see lines #62, #65, and #91.

Is my understanding of those assignment statements in PHP correct? I apologize that I didn't clarify that in my initial suggestion.

@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Sep 2, 2015

Member

OK I see what you meant! Indeed the reference might be lost at points like that, I was thinking of the actual method/constructor call.

Member

mnapoli commented Sep 2, 2015

OK I see what you meant! Indeed the reference might be lost at points like that, I was thinking of the actual method/constructor call.

@mnapoli mnapoli closed this in 69ae17d Sep 3, 2015

mnapoli added a commit that referenced this issue Sep 3, 2015

Merge pull request #307 from bradynpoulsen/bug/null_on_parameter_pass…
…_by_reference

Fixed PHP-DI#306 modifying ParameterResolver and ObjectCreator

@mnapoli mnapoli changed the title from Make when pass by reference to Errors when parameters are passed by reference Sep 6, 2015

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