DI container #2788

Merged
merged 12 commits into from Mar 21, 2014

Conversation

Projects
None yet
5 participants
@qiangxue
Member

qiangxue commented Mar 18, 2014

Fixes #503

For performance reason, automatic constructor injection is not implemented.

The most obvious BC breaking change to existing code is the renaming from Application::getComponent() to get().

  • refactor existing app components to make use of Instance
  • controller creation should make use of container
  • guide titled "Dependency injection and service locator"
framework/di/Instance.php
+ */
+ public static function of($id, ContainerInterface $container = null)
+ {
+ return new self($id, $container);

This comment has been minimized.

@Ragazzo

Ragazzo Mar 18, 2014

Contributor

maybe new static ?

@Ragazzo

Ragazzo Mar 18, 2014

Contributor

maybe new static ?

This comment has been minimized.

@qiangxue

qiangxue Mar 18, 2014

Member

We do not expect Instance to be subclassed since it's usage is mainly via static method calls.

@qiangxue

qiangxue Mar 18, 2014

Member

We do not expect Instance to be subclassed since it's usage is mainly via static method calls.

This comment has been minimized.

@Ragazzo

Ragazzo Mar 18, 2014

Contributor

well, but better to leave this ability to user as for me . Because Html helper in almost all cases will not be overrided but it has static .

@Ragazzo

Ragazzo Mar 18, 2014

Contributor

well, but better to leave this ability to user as for me . Because Html helper in almost all cases will not be overrided but it has static .

framework/di/ContainerTrait.php
+ */
+ public function set($typeOrID, $definition)
+ {
+ if ($notShared = $typeOrID[0] === '*') {

This comment has been minimized.

@Ragazzo

Ragazzo Mar 18, 2014

Contributor

can we mark not-shared as ! ? It will be more intuitive .

@Ragazzo

Ragazzo Mar 18, 2014

Contributor

can we mark not-shared as ! ? It will be more intuitive .

This comment has been minimized.

@qiangxue

qiangxue Mar 18, 2014

Member

* means "many", while ! can be interpreted differently, depending on the context (it could mean important or "not").

@qiangxue

qiangxue Mar 18, 2014

Member

* means "many", while ! can be interpreted differently, depending on the context (it could mean important or "not").

This comment has been minimized.

@Ragazzo

Ragazzo Mar 18, 2014

Contributor

yes, it means not - not shared.

@Ragazzo

Ragazzo Mar 18, 2014

Contributor

yes, it means not - not shared.

This comment has been minimized.

@qiangxue

qiangxue Mar 18, 2014

Member

Well, it depends on what word is used after not. It may cause confusion. Also !db reads more like not db if you want to interpret it as "not".

@qiangxue

qiangxue Mar 18, 2014

Member

Well, it depends on what word is used after not. It may cause confusion. Also !db reads more like not db if you want to interpret it as "not".

This comment has been minimized.

@Ragazzo

Ragazzo Mar 18, 2014

Contributor

maybe ~ ? because * almost always means all and it confusing, while ~ can mean different constraints .

@Ragazzo

Ragazzo Mar 18, 2014

Contributor

maybe ~ ? because * almost always means all and it confusing, while ~ can mean different constraints .

This comment has been minimized.

@qiangxue

qiangxue Mar 18, 2014

Member

Only when * is used alone, it means all. When it is used with other things, it means repeating.
~ is almost reserved to represent "home directory".

@qiangxue

qiangxue Mar 18, 2014

Member

Only when * is used alone, it means all. When it is used with other things, it means repeating.
~ is almost reserved to represent "home directory".

This comment has been minimized.

@Ragazzo

Ragazzo Mar 18, 2014

Contributor

but as you said it means repeating, and repeating many times like multiply command . But we actually dont repeat anything we want to not allow to repeat .

@Ragazzo

Ragazzo Mar 18, 2014

Contributor

but as you said it means repeating, and repeating many times like multiply command . But we actually dont repeat anything we want to not allow to repeat .

This comment has been minimized.

@qiangxue

qiangxue Mar 18, 2014

Member

notShared = a new instance will be created each time get() is called. So it does mean repeating.

@qiangxue

qiangxue Mar 18, 2014

Member

notShared = a new instance will be created each time get() is called. So it does mean repeating.

This comment has been minimized.

@Ragazzo

Ragazzo Mar 18, 2014

Contributor

ok

@Ragazzo

Ragazzo Mar 18, 2014

Contributor

ok

@Ragazzo

This comment has been minimized.

Show comment
Hide comment
@Ragazzo

Ragazzo Mar 18, 2014

Contributor

Do we really need Instance ? since Container already creates components if needed .

Contributor

Ragazzo commented Mar 18, 2014

Do we really need Instance ? since Container already creates components if needed .

@samdark

This comment has been minimized.

Show comment
Hide comment
@samdark

samdark Mar 18, 2014

Member

https://github.com/container-interop/container-interop created by many framework representatives. At least Zend Framework, Laravel, Aura, Joomla Framework, Acclimate, Molajo, Mouf, PHP-DI participated. Worth checking.

Member

samdark commented Mar 18, 2014

https://github.com/container-interop/container-interop created by many framework representatives. At least Zend Framework, Laravel, Aura, Joomla Framework, Acclimate, Molajo, Mouf, PHP-DI participated. Worth checking.

@Ragazzo

This comment has been minimized.

Show comment
Hide comment
@Ragazzo

Ragazzo Mar 18, 2014

Contributor

currently there is nothing in that repo except several files, where is all discussion or what need to be checked ?

Contributor

Ragazzo commented Mar 18, 2014

currently there is nothing in that repo except several files, where is all discussion or what need to be checked ?

@samdark

This comment has been minimized.

Show comment
Hide comment
@samdark

samdark Mar 18, 2014

Member

Interface and meta. It seems we already have get and has btw.

Member

samdark commented Mar 18, 2014

Interface and meta. It seems we already have get and has btw.

@Ragazzo

This comment has been minimized.

Show comment
Hide comment
@Ragazzo

Ragazzo Mar 18, 2014

Contributor

i see, not a big discussion overall . Maybe we need to add has method .

Contributor

Ragazzo commented Mar 18, 2014

i see, not a big discussion overall . Maybe we need to add has method .

@samdark

This comment has been minimized.

Show comment
Hide comment
@samdark

samdark Mar 18, 2014

Member

We have it.

Member

samdark commented Mar 18, 2014

We have it.

@Ragazzo

This comment has been minimized.

Show comment
Hide comment
@Ragazzo

Ragazzo Mar 18, 2014

Contributor

right ) maybe then ArrayAccess ? Could be useful )

Contributor

Ragazzo commented Mar 18, 2014

right ) maybe then ArrayAccess ? Could be useful )

framework/di/Container.php
+ * use yii\di\Container;
+ * use yii\di\Instance;
+ *
+ * interface UserFinderInterface

This comment has been minimized.

@samdark

samdark Mar 18, 2014

Member

Need to mention that it has to be a separate file.

@samdark

samdark Mar 18, 2014

Member

Need to mention that it has to be a separate file.

@samdark

This comment has been minimized.

Show comment
Hide comment
@samdark

samdark Mar 18, 2014

Member

Overall looks good to me. It should be unit-tested properly since it's the very core thing.

Member

samdark commented Mar 18, 2014

Overall looks good to me. It should be unit-tested properly since it's the very core thing.

framework/base/Module.php
@@ -130,15 +131,14 @@ public function __construct($id, $parent = null, $config = [])
/**
* Getter magic method.
- * This method is overridden to support accessing components
- * like reading module properties.
+ * This method is overridden to support accessing components like reading properties.
* @param string $name component or property name
* @return mixed the named property value
*/
public function __get($name)

This comment has been minimized.

@cebe

cebe Mar 18, 2014

Member

shouldn't this be part of the trait?

@cebe

cebe Mar 18, 2014

Member

shouldn't this be part of the trait?

This comment has been minimized.

@qiangxue

qiangxue Mar 18, 2014

Member

Not quite sure as it assumes the existence of parent::__get().

@qiangxue

qiangxue Mar 18, 2014

Member

Not quite sure as it assumes the existence of parent::__get().

framework/di/Container.php
+ *
+ * interface UserFinderInterface
+ * {
+ * function findUser();

This comment has been minimized.

@cebe

cebe Mar 18, 2014

Member

should be public function.

@cebe

cebe Mar 18, 2014

Member

should be public function.

This comment has been minimized.

@qiangxue

qiangxue Mar 18, 2014

Member

All interface methods must be public.

@qiangxue

qiangxue Mar 18, 2014

Member

All interface methods must be public.

+ * function findUser();
+ * }
+ *
+ * class UserFinder extends Object implements UserFinderInterface

This comment has been minimized.

@cebe

cebe Mar 18, 2014

Member

also shouldn't classes that are called components in this example extend from Component for better understanding? I know it does not make a difference from functional point of view.

@cebe

cebe Mar 18, 2014

Member

also shouldn't classes that are called components in this example extend from Component for better understanding? I know it does not make a difference from functional point of view.

framework/test/DbFixture.php
@@ -35,7 +35,7 @@ public function init()
{
parent::init();
if (is_string($this->db)) {
- $this->db = Yii::$app->getComponent($this->db);
+ $this->db = Yii::$app->get($this->db);

This comment has been minimized.

@cebe

cebe Mar 18, 2014

Member

shouldn't this code be rewritten to use Instance::ensure?

@cebe

cebe Mar 18, 2014

Member

shouldn't this code be rewritten to use Instance::ensure?

@nkt

This comment has been minimized.

Show comment
Hide comment
@nkt

nkt Mar 19, 2014

Why not just is_callable?

Why not just is_callable?

qiangxue added a commit that referenced this pull request Mar 21, 2014

@qiangxue qiangxue merged commit 3d0a266 into master Mar 21, 2014

@qiangxue qiangxue deleted the feature-container branch Mar 21, 2014

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