ZF2-46: Definition/Compiler marks all setter-injection method arguments as optional

Description

Reproducing code (from http://pastie.org/2325111)

Target of setter injection:


<?php
namespace DiTest;

class ObjectOne
{
    protected $_objTwo = null;
    
    /**
     * Set Dependency
     * @param ObjectTwo $o
     * @return ObjectOne 
     */
    public function setTwo(ObjectTwo $o)
    {
        $this->_objTwo = $o;
        return $this;
    }
}

Definition Compiler:


<?php
// Compile DI definitions
$compiler = new Zend\Di\Definition\Compiler();
$compiler->addCodeScannerDirectory(
    new Zend\Code\Scanner\DirectoryScanner(
        APPLICATION_PATH . "/../library/DiTest"
    )
);
$definitions = $compiler->compile();

DiTest\ObjectTwo is an empty class.

Resulting Definition for DiTest\ObjectOne (print_r):


[DiTest\ObjectOne] => Array
    (
        [superTypes] => Array
            (
            )

        [instantiator] => __construct
        [injectionMethods] => Array
            (
                [setTwo] => Array
                    (
                        [o] => Array
                            (
                                [0] => DiTest\ObjectTwo
                                [1] => 1                 //<-- Paramter $o marked as optional, which it isn't
                                [2] => 1
                            )

                    )

            )

    )

Comments

Possible fix:


diff --git a/library/Zend/Di/Definition/Compiler.php b/library/Zend/Di/Definition/Compiler.php
index 5cdb446..d1523ad 100644
--- a/library/Zend/Di/Definition/Compiler.php
+++ b/library/Zend/Di/Definition/Compiler.php
@@ -257,8 +257,8 @@ class Compiler
                 $params[$paramName][] = null;
             }

-            if ($introspectionType == IntrospectionRuleset::TYPE_SETTER && $rules['paramCanBeOptional']) {
-                $params[$paramName][] = true;
+            if ($introspectionType == IntrospectionRuleset::TYPE_SETTER && !$rules['paramCanBeOptional']) {
+                $params[$paramName][] = false;
             } else {
                 $params[$paramName][] = $p->isOptional();
             }

The first part of the existing logic says that "If method is setter and parameter can be optional, then it is optional", whereas it should be more along the lines of: "If method is setter and parameter cannot be optional, then it is not optional"

I've placed this fix in my ZF2 fork: https://github.com/adamlundrigan/zf2/…

Confirmed. I went ahead and adjusted the unit tests in my fork to check for this, and with Adam's fix, the tests pass.

https://github.com/EvanDotPro/zf2/…

I've opened pull request #321 to have this fix + contributed tests merged into zendframework/zf2:master

Looking at this a bit deeper, it seems that "optional" in terms of Zend\Di is not strictly based off whether the parameter is literally optional with a default value in the setter (the parameter in a setter is generally always going to be required, otherwise you would not be calling the setter). For that reason, this fix is probably not exactly correct.

What "optional" seems to mean here, is that the DI container should not allow an instance of a class without some property being defined in the DI definition / configuration.

Interestingly, reversing the logic as Adam has in his patch in Zend\Di\Definition\RuntimeDefinition does actually fix the failing assertion in the Zend\Di\DependencyInjector test. This could be to another developer making the same assumption in this bug report, and coding some part of Zend\Di based on that assumption. I'll keep digging.

So, at the time of the writing of Zend\Di, there was plenty of code out there, including code matthew was writing for the example ZF2 quickstart that had setters that accepted type-hinted objects that were also not null. They match the setter pattern: set[A-Z]\w(), but the author would not necessarily consider the presence of this method to mean there is a hard dependency from the class its in on the class in the typehint.

The unit test currently failing is similar to the code below. Also in the code below, you'll see the modification to the IntrosepctionRuleset that would make it behavior like you are expecting:



<?php
class X
{
    public $one = null;
    public $two = null;
    public function setOne($one)
    {
        $this->one = $one;
    }
    public function setTwo($two)
    {
        $this->two = $two;
    }
}

class Y
{
    public $x = null;
    public function setX(X $x)
    {
        $this->x = $x;
    }
}


$di = new Zend\Di\DependencyInjector;
$im = $di->getInstanceManager();

// START change of the introspection rules
$ir = $di->getDefinition()->getIntrospectionRuleset();
$ir->addSetterRule('paramCanBeOptional', false);
// END change of the introspection rules

$im->setParameters('X', array('one' => 1, 'two' => 2));
$y = $di->newInstance('Y');
var_dump($y);

The question is, is this the correct default value for the introspection ruleset? Do we go stricter or less strict out of the box? Basically, how do we handle situations like this?

-ralph

After further work with Zend\Di, and discussions with Evan on this issue, i've determined the issue isn't with Zend\Di but with the way I was using it.