auraphp / Aura.Di

Dependency Injection System
MIT License
349 stars 63 forks source link

add option `throws_on_auto_resolve_failed` and setters to Container and Factory #88

Closed takyam-git closed 9 years ago

takyam-git commented 9 years ago

I love auto-resolve constructor arguments with type hinted. But a argument which non-TypeHinted will be null makes debug difficult.

//define classes
class Foo {
    public function __construct(Bar $bar, $val1, $val2) {}
}
class Bar {}
//setup di params
$di->params['Foo'] = ['val1' => 'defaultValue'];

//---try newInstance---

// expect usage: $foo = $di->newInstance('Foo', ['val2' => 'VALUE2']);
// but calls something like this, I forgot pass val2.
$foo = $di->newInstance('Foo');
 //(maybe:) $bar is new Bar(), $val1 is 'defaultValue', $val2 is null

This example throws nothing, so I can't catch my mistake! I want to catch my mistake.


Sorry I am not good in English. Thank you for read.

pmjones commented 9 years ago

Your English is fine. :-) Would you mind if we made it so that failure to auto-resolve always throws an exception? Or would that break the previously-existing tests?

pmjones commented 9 years ago

On further reading, your use case is exactly why I don't like auto-resolution. To recap:

class Foo {
    public function __construct(Bar $bar, $val1, $val2) {}
}

$di->params['Foo'] = ['val1' => 'defaultValue'];

$foo = $di->newInstance('Foo');

// $foo->bar is new Bar(), $foo->val1 is 'defaultValue', $foo->val2 is null

An auto-resolver cannot tell if you forgot to specify $di->params['bar'], or if you intentionally did not specify it.

So, if the auto-resolver throws an exception when you do not specify a param, then the whole point of auto-resolution is lost (i.e., not having to specify things). You then have to specify everything, and the auto-resolver no longer auto-resolves.

Do you get what I mean here?

koriym commented 9 years ago

@takyam-git もし英語でわからないところがあったら通訳します! @pmjones I'm ready to help him with language.

takyam-git commented 9 years ago

I want thing is simple.

class A { //has not arguments.
    public function __construct(){}
}
class B { //has argument with default value.
    public function __construct($arg = 'defaultValue'){}
}
class C { //has argument with `$di->params` default value
    public function __construct($arg){}
}
$di->params['C'] = ['arg' => 'defaultValue'];
class X {
    /** @var string|null */
    private $d;
    /**
     * @param A $a will auto-resolve!
     * @param B $b will auto-resolve!
     * @param C $c will auto-resolve!
     * @param string|null $d you must pass a param
     */
    public function __construct(A $a, B $b, C $c, $d){ $this->d = $d; }
    public function isSomething(){
        return $this->d !== null;
    }
}

//-----------

$d = 'something';
$x = $di->newInstance('X');  //This is miss code!

The correct code is $di->newInstance('X', ['d' => $d]) But I did not notice my mistake, because X gets null which is correct param type. This thing makes debugging hard.


@korym ありがとうございます! pmjonesさんが言ってる事を、正確に読み取れてる自信は無いのですが、タイプヒント無い引数に対してnullをセットされてしまうと、デバッグが難しくなるので、それを回避したいです。といった内容が伝わると嬉しいです。

koriym commented 9 years ago

@takyam-git 伝わっています!

しかし@pmjones 氏は「しかしもし依存をセットしてない時に例外を投げるなら、それでは全ての依存に対してセットする必要があって、auto-resolveの意味がなくなってしまう。」とのこと。

takyam-git commented 9 years ago

ありがとうございます! 確かにauto-esolveの意味は薄れるのですが、上の例のようにA,B,Cクラスの設定がされていれば、Xクラスの設定は不要になる程度のauto-resolveが欲しかった感じです。 現状ですと、auto-resolveをオフにした場合には、Xについても設定が必要になると思いますが、そういった煩雑さを大部分解消できると思っています。

pmjones commented 9 years ago

Ah, I see! Auto-resolution only for class type hints, and not for anything else. Yes, I think that's a fair feature request.

Unfortunately, I cannot implement it in the 2.x branch, because it will be a BC break.

However, we can implement it in the 3.x branch, as it has not had a stable release yet. Would a 3.x implementation serve your purposes?

koriym commented 9 years ago

@takyam-git なるほど、良いPRだと思います! 2.xでの対応はBCブレイクで難しいですが3.xではどうか?との事です。

@pmjones +1 from me too.

pmjones commented 9 years ago

This feature has been added in the 3.x branch with commit https://github.com/auraphp/Aura.Di/commit/76adea14bc0bca46ff2f26deefbda086fa833e9c

pmjones commented 9 years ago

Thanks gentlemen; let me know if the new commit helps or not.

koriym commented 9 years ago

Thanks @takyam-git @pmjones

takyam-git commented 9 years ago

@pmjones @koriym Thank you!!