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

Add `Container::create()` method #23

Open
klimov-paul opened this Issue Mar 6, 2018 · 19 comments

Comments

Projects
None yet
3 participants
@klimov-paul
Member

klimov-paul commented Mar 6, 2018

At the present state Container is almost useless in Yii Framework scope. It is unable to replace old Container from Yii 2.0.x.
In particular there is no way to create an object from array definition taking into account possible singleton definitions or predefined configurations.

Previously this task was implemented inside Container::get() method, which accepted constructor arguments and object config as additional parameters:

$foo = $container->get(Foo::class, ['constructor argument'], ['someField' => 'some-value']);

Accepting PSR for the container makes impossible to use get() for this purpose.

I suggest a separated method create() (or createObject()) should be introduced to incapsulate former object creation logic.
Method signature:

public function create(array $definition) : object

This method should check for registered definitions and use Container::build() to create final object.

@klimov-paul klimov-paul added this to the 1.0 milestone Mar 6, 2018

@samdark

This comment has been minimized.

Member

samdark commented Mar 6, 2018

Seems to be the same as #22.

@samdark

This comment has been minimized.

Member

samdark commented Mar 6, 2018

Introducing create() makes you use container explicitly which defeats its purpose turning it into service locator... I'd like to avoid it.

@klimov-paul

This comment has been minimized.

Member

klimov-paul commented Mar 6, 2018

I am not sure I understand.
For me in current state Container is useless. I am unable to build object from its definition, because this code is hidden inside protected method Container::build().
If I have an object deifnition like:

[
    '__class' => Foo::class,
    'name' => 'something',
]

how I can create an object from it?

At the present state the only option I see is setting this definition with some unique name to Container and get it from it:

$definition = [
    '__class' => Foo::class,
    'name' => 'something',
];

$id = uniqid();
$container->set($id, $definition);

$object = $container->get($id);

I do not see why shortcut method create() makes anything worse in comparison with this code.

@samdark

This comment has been minimized.

Member

samdark commented Mar 6, 2018

  1. You need to put your definition in container.
  2. You type-hint in controller via Injector never calling get yourself since that makes your class container-aware.
@klimov-paul

This comment has been minimized.

Member

klimov-paul commented Mar 6, 2018

So I should always create a method with type-hint and invoke it via Injector - this does not feels right.

For example: how behaviors for the component should be initialized with such approach?

https://github.com/yiisoft/yii2/blob/master/framework/base/Component.php#L749-L751

@klimov-paul

This comment has been minimized.

Member

klimov-paul commented Mar 6, 2018

How we can define default settings for the widgets to be applied during rendering?
For example to attach yii\jquery\ActiveFormClientScriptBehavior to every ActiveForm instance?

@klimov-paul

This comment has been minimized.

Member

klimov-paul commented Mar 6, 2018

How I can define and use 2 separated instances of yii\db\Connection with such approach?
Should I define 2 fake classes or interfaces to be used for type-hinting and container IDs?

@samdark

This comment has been minimized.

Member

samdark commented Mar 6, 2018

How we can define default settings for the widgets to be applied during rendering?
For example to attach yii\jquery\ActiveFormClientScriptBehavior to every ActiveForm instance?

Could be the job of container-aware factory.

How I can define and use 2 separated instances of yii\db\Connection with such approach?
Should I define 2 fake classes or interfaces to be used for type-hinting and container IDs?

Ideally connection should be turned into connection pool service. It already is when used with master-slave config.

Both solutions are from the strict world when we make services container unaware as it was intended to be. In 2.1 we could use the same approach as in 2.0 to make transition easier but that solution should not be part of yiisoft/di.

@klimov-paul

This comment has been minimized.

Member

klimov-paul commented Mar 7, 2018

Could be the job of container-aware factory.

So I should create yet another entity - a factory to perform the old task?
And inside it I should duplicate the entire content of Container::build() method?

Yet still my first question is unanswered: how behaviors for the component should be initialized?
https://github.com/yiisoft/yii2/blob/master/framework/base/Component.php#L749-L751

How GridView should instantiate its columns?
https://github.com/yiisoft/yii2/blob/master/framework/grid/GridView.php#L569-L576

Both solutions are from the strict world when we make services container unaware as it was intended to be

I do not need some PSR fancy toy - I need a solution to the common tasks, which appear around project development. This entire repository worth nothing unless these problems are solved.

@samdark

This comment has been minimized.

Member

samdark commented Mar 7, 2018

Via similar factories or the same single factory.

This entire repository worth nothing unless these problems are solved.

The problems in question aren't responsibility of DI container if we are speaking about general purpose DI container. Of course, we have to solve these but I don't think it should be done in the container itself else it will be used directly so:

  1. Most of project classes will depend on container directly.
  2. More than that, there will be dependency on this specific container instead of PSR interface.
@klimov-paul

This comment has been minimized.

Member

klimov-paul commented Mar 7, 2018

In case object creation from array definition is not a responsibility of the container as you claim, then its method build() should be removed.

If you wish to make everything strict, the Container can not instantiate any object itself. It should use some ObjectFactory, which will convert array configuration into a object then, while Contaner should work around it.

To be 'only a container' Container class should be stripped from object instantiation logic. It should only store the definion and return it by demand - nothing more.
If it converts array definion into an object this means it already behaves like Service Locator, which you claim it is not.

@samdark

This comment has been minimized.

Member

samdark commented Mar 7, 2018

I haven't said that build() should not be part of the container. I said that it should not be called by end users directly. That's why it's protected.

If it converts array definion into an object this means it already behaves like Service Locator, which you claim it is not.

Only if the build method is used directly and container is referenced explicitly in the end user code.

@samdark

This comment has been minimized.

Member

samdark commented Mar 7, 2018

The goal of dependency inversion is that something else is injecting dependencies into a class instead of the class pulling its dependencies by itself. This way the class stays clear of dependency management logic and has no dependencies on any container implementation. Developer may use no container at all and pass already instantiated dependencies to the class manually.

@SilverFire

This comment has been minimized.

Member

SilverFire commented Mar 8, 2018

Oww, that will hurt 🤕

@samdark I do strongly agree with all the arguments you have, but there are a lot (270K occurrences) of using Yii::createObject() as a factory. IMO dropping this feature will become the most frequent stopper in 2.0→2.1 migration process.

I think the best we can do in 2.1 is to discourage users from using Yii::createObject() by adding @deprecated or @internal with clear description why going this way is, probably, not the best idea.

@klimov-paul

This comment has been minimized.

Member

klimov-paul commented Mar 8, 2018

I haven't said that build() should not be part of the container. I said that it should not be called by end users directly. That's why it's protected.

This does not make any sense.
You put object instantiation: conversion of array definition into object inside the Container. This makes Container to be an object factory. But when I ask to use container factory ability, you say it is forbidden.
So in the end Container is a factory, but it can not be used as a factory - what kind of nonsense is this?

You claim I should create some external container-aware factory, if I wish to re-use container defintion for multiple object instantiation. But I can not see any way of doing this: Container will automatically convert any array definition into an object during get() invocation, so I am unable to get pure object definition for my factory. And even if mange to get this defintion, I will have to duplicate entire code from Container::build() to actually convert the final definition, I have built, into an object.

Is a encouraging of code duplication also a part of PSR?
As far as I understand PSR-11 provides unified way to inject object parameters to the method invocation, but I can not see it putting any restriction on the extra container behavior.

If there is a proper way to solve the tasks demanded by framework structure and project development, which I, in my ignorance, can not see, then provide an explicit code snippet for them.

@klimov-paul

This comment has been minimized.

Member

klimov-paul commented Mar 9, 2018

And so the conversation is running circles...

Twice I have asked for the example of merging developer-defined object definition with the container preset and got no answer.
Twice I have asked for the clear way to convert array definion into object instance and got the answer it is a bad practice and should be forbidden.

It is clear you have made your decision and do not want to hear me. You wish to drop one of the base Yii core principles of manipulating by array defintions instead of objects, which is a basis of lazy loading framework mechanism in favor of explicit factory-horde approach.

Whether this is for good or ill is not for me to judge. But clearly further discussion of this matter it pointless.

@klimov-paul klimov-paul closed this Mar 9, 2018

@SilverFire SilverFire reopened this Mar 9, 2018

@SilverFire

This comment has been minimized.

Member

SilverFire commented Mar 9, 2018

I think this issue should not be closed.

@samdark what do you think about my comment above?

@samdark

This comment has been minimized.

Member

samdark commented Mar 9, 2018

It makes perfect sense to put compatibility layer into framework itself, not yiisoft/di.

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