Skip to content
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

DI injection via method signature #9565

Closed
wants to merge 6 commits into from

Conversation

mdmunir
Copy link
Contributor

@mdmunir mdmunir commented Sep 1, 2015

#9476 I hope i am doing right.

@samdark samdark self-assigned this Sep 2, 2015
@samdark samdark added this to the 2.0.7 milestone Sep 2, 2015
@kathysledge
Copy link
Contributor

👍 Looks good. Could you add the following to the tests?

$result = $controller->runAction('aksi5', $params);
$this->assertEquals(['d426', 'independent', 'other_qux'], $result);
...
public function actionAksi5($q, Bar $bar, QuxInterface $quxApp)
{
    return [$q, $bar->foo, $quxApp->quxMethod()];
}

@samdark
Copy link
Member

samdark commented Sep 4, 2015

In #9476 I've meant general purpose method injection, not necessary controller but probably it's OK.

}
}

class FakeController extends Controller
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to move these into separate files.

@mdmunir
Copy link
Contributor Author

mdmunir commented Sep 4, 2015

@samdark ya i know. But how to do that?

class MyObj extends Object
{
    public function test(Cache $cache)
    {
        $cache->set();
    }
}

$obj = new MyObj();
$obj->test(); // <- will always error

Other scenario

class MyObj extends Object
{
    protected function test(Cache $cache) // as protected
    {
        $cache->set();
    }

    public __call($name,$args)
    {
        $ref = getReflectionMethod($name);
        $args = injectArgument($ref,$args);
        return call_function_array([$this,$name],$args);
    }
}

$obj = new MyObj();
$obj->test(); 

I am not sure its a good scenario

@kathysledge
Copy link
Contributor

I think one-step at a time. First get it working well with controller actions and the modified tests passing. Then it can be put through its paces after merging. Once that is stable, some thought can be put in to a more generic method version. Just my 2¢.

@mdmunir
Copy link
Contributor Author

mdmunir commented Sep 4, 2015

@df2 do you need another test before i add new commit?

@samdark
Copy link
Member

samdark commented Sep 4, 2015

Agree.

@kathysledge
Copy link
Contributor

@mdmunir I was just thinking the one above with the url parameter first and then the service variables.

@samdark
Copy link
Member

samdark commented Sep 4, 2015

Overall it works well. One thing is, for example, if I define method signature like the following

public function actionIndex(BooleanValidator $validator)

and then pass ?validator=123 via request, Yii raises an exception. I think if an argument is type-hinted, it should not receive any value.

@@ -84,6 +84,13 @@ public function bindActionParams($action, $params)
unset($params[$name]);
} elseif ($param->isDefaultValueAvailable()) {
$args[] = $actionParams[$name] = $param->getDefaultValue();
} elseif (($c = $param->getClass()) !== null) {
$type = $c->getName();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better to use $class instead of $c.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also $type could be renamed to $className.

@mdmunir
Copy link
Contributor Author

mdmunir commented Sep 4, 2015

@samdark I think the same. Prefer change the order of checking. Check type at first before $params avaliable.

            $name = $param->getName();
            if (($class = $param->getClass()) !== null) {
                $className = $class->getName();
                if (Yii::$app->has($name) && ($obj = Yii::$app->get($name)) instanceof $className) {
                    $args[] = $actionParams[$name] = $obj;
                }else{
                    $args[] = $actionParams[$name] = Yii::$container->get($className);
                }
            }elseif (array_key_exists($name, $params)) {
                if ($param->isArray()) {
                    $args[] = $actionParams[$name] = (array) $params[$name];
                } elseif (!is_array($params[$name])) {
                    $args[] = $actionParams[$name] = $params[$name];
                } else {
                    throw new BadRequestHttpException(Yii::t('yii', 'Invalid data received for parameter "{param}".', [
                        'param' => $name,
                    ]));
                }
                unset($params[$name]);

@samdark
Copy link
Member

samdark commented Sep 4, 2015

Yep. The case I've described should be tested.

@mdmunir
Copy link
Contributor Author

mdmunir commented Sep 4, 2015

    // FakeController
    public function actionAksi6($q, EmailValidator $validator)
    {
        return [$q, $validator->validate($q), $validator->validate('misbahuldmunir@gmail.com')];
    }

    // Test
    $params = ['fromGet'=>'from query params','q'=>'d426','validator'=>'avaliable'];
    $result = $controller->runAction('aksi6', $params);
    $this->assertEquals(['d426', false, true], $result);

?

@samdark
Copy link
Member

samdark commented Sep 4, 2015

Yes.

{
return [$q, $validator->validate($q), $validator->validate('misbahuldmunir@gmail.com')];
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing \n?

@samdark
Copy link
Member

samdark commented Sep 7, 2015

The code looks OK to me now (except really minor formatting issues). @mdmunir do you want to attempt extending DI docs?

@mdmunir
Copy link
Contributor Author

mdmunir commented Sep 7, 2015

You do @samdark, i am not good in english 😀

@samdark
Copy link
Member

samdark commented Sep 7, 2015

OK.

@samdark
Copy link
Member

samdark commented Sep 7, 2015

Found another thing. While web controllers are good we haven't covered console controllers.

@mdmunir
Copy link
Contributor Author

mdmunir commented Sep 7, 2015

Ya, i miss it. Should i take it in this PR or create new PR?

@samdark
Copy link
Member

samdark commented Sep 7, 2015

This one is OK.

@samdark samdark closed this Sep 27, 2015
@samdark
Copy link
Member

samdark commented Sep 27, 2015

Merged b702006. Thank you!

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 51887c8 on mdmunir:9476-di-via-method-signature into ** on yiisoft:master**.

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

Successfully merging this pull request may close these issues.

None yet

5 participants