[WIP] Added factory functionality, see #25 #26

hiqsol · 2018-03-18T11:56:32Z

Not finished. Just to see how it looks like.
Will finish (add tests and possibly better naming) after discussion.

Actually changes are minimal but are shown huge because of renamed file.
Tried redo in two commits - doesn't help. Don't know how to help with it.

Highlights:

AbstractContainer
- protected build($id, $config)
- protected buildFromDefinition($definition)
- Factory
  - public create($id, $config) - new instance every time
- Container
  - private $instances
  - public get($id) - same instance every time

No sameEveryTime flag was needed.
Factory has create but not get and Container vice versa.

samdark · 2018-03-18T23:00:43Z

src/Container.php

- $this->parent = $parent;
+ /// XXX don't get it
+ /// is it intended to get different instances for same id?
+ $this->instances[$id] = null;


No. For container it's always the same instance.

The goal of this line is to react to calling set() runtime i.e. it will start returning new instance even if it was obtained before with old definition.

Yes. This is what I was asking about.
Container will return different object after definition is changed.
I was not sure if it's ok. I haven't found anything about it in PSR.

Yes. It's OK.

samdark · 2018-03-18T23:02:48Z

Looks good to me. Let's finish it.

klimov-paul · 2018-03-19T12:09:59Z

src/Container.php

- } else {
- throw new InvalidConfigException('Unexpected object definition type: ' . gettype($definition));
- }
+ $object = $this->create($id);


How this can function? There is no create() method declaration either at AbstractContainer not at Container.

Sorry. Didn't test it at all. It has to be build().

I want to get feedback and then do it properly.

klimov-paul · 2018-03-19T12:14:50Z

src/Factory.php

+ */
+class Factory extends AbstractContainer
+{
+ public function create($id, array $config = [])


I don't get it: when I asked for addition of create() method to the Container at #23, @samdark refused claiming it contradicts PSR. But in case we have an AbsractContainer and its 2 descendants, having create() method inside of some of them suddenly becomes totally fine!
Does Factory class no longer implements PSR interface and thus PSR rules does not apply to it?

Yes. Factory is explicitly non-PSR thing. You've got it right.

Can you clarify in which way you separate 'PSR thing' from 'non-PSR thing'?
As far as I see, Factory IS a PSR container as it matches ContainerInterface, and thus it can be used with Injector while resolving dependency injection.
This makes your words to be standing for it just as well.

It seems you are interpreting PSR-11 as you please, depending on particular situation.

What I was concerned about is point 1.3 of the PSR:

Users SHOULD NOT pass a container into an object so that the object can retrieve its own dependencies. This means the container is used as a Service Locator which is a pattern that is generally discouraged.

My concern is that if we'll allow using container as service locator, developer would you use it exactly like that even for singleton services because it feels simpler to do that before you start taking care about high coupling.

If we'll look at meta document:

As an exception, factory objects whose only purpose is to create and return new instances may use the service locator pattern. The factory must then implement an interface so that it can itself be replaced by another factory using the same interface.

It's understandable because there's no other way to get instance configured in runtime from a factory and for that use case it's fine. That's why having separate Container (singletons only, for keeping services) and Factory (non-singletons only, for creating specific objects configured in runtime) sounds like a good solution.

And if ContainerInterface seems not suitable for Factory it can be fixed easily by removing the interface from AbstractContainer.

The only reason is that technically simpler variant is easier to use wrong (referencing container itself to obtain services from it) and no matter how well we'll document that it's bad to do it, there will be developer who will use it that way. In case of Container/Factory instead of documenting the right way of using container we're forbidding wrong use technically.

And so, we create an obstacle for ourselves at Container to heroically overcome it later with Factory!

This is madness... The Symfony-type madness, to be precise. What is the point of creating restriction, which has a bypass solution right near it, for the developer? This is pointless attempt of re-creation Java, which its code restrictions policy, using PHP.

The developer, which expects PSR container from this repository will use Container according to the PSR-11 definition anyway. The developer, which want to use Container as a factory will end up with Factory anyway. Creating 2-level container hierarchy solves nothing.
Besides, in which way developer will setup definitions and singletons with such 2-level structure?
Which instance Yii::$container should hold then Container or Factory?

I've posted example config in #25

There should not be Yii::$container in the end. The Container should be used in transitional period.

Yes. In 2.2 or later we should drop Yii::$container and only use dependencies via Injector.

The developer, which want to use Container as a factory will end up with Factory anyway. Creating 2-level container hierarchy solves nothing.

It will ensure that service Container won't be used as service locator.

klimov-paul · 2018-03-19T12:17:55Z

src/AbstractContainer.php

+ return $this->buildFromDefinition($config);
+ }
+ if ($this->parent !== null) {
+ return $this->parent->build($id, $config);


I can not see how it resolves definition handling.
In case Factory will use Container as parent, it will be unable to use its own defintions, this function will fallback to Conatiner::build() and always return a singleton or throw an exception.

No. Factory will use its own definitions as far as given ID is among those definitions.
Factory has its own definitions and defaults to parent only if (!isset($this->definitions[$id])).

[WIP] Added factory functionality, see yiisoft#25

92d814c

samdark approved these changes Mar 18, 2018

View reviewed changes

samdark added this to the 1.0 milestone Mar 18, 2018

klimov-paul suggested changes Mar 19, 2018

View reviewed changes

hiqsol mentioned this pull request Apr 9, 2018

Added Factory, see #25 #37

Merged

hiqsol closed this Apr 9, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Added factory functionality, see #25 #26

[WIP] Added factory functionality, see #25 #26

hiqsol commented Mar 18, 2018

samdark Mar 18, 2018

samdark Mar 18, 2018 •

edited

Loading

hiqsol Mar 19, 2018

samdark Mar 19, 2018

samdark commented Mar 18, 2018

klimov-paul Mar 19, 2018

hiqsol Mar 19, 2018

klimov-paul Mar 19, 2018

samdark Mar 19, 2018

klimov-paul Mar 19, 2018 •

edited

Loading

samdark Mar 19, 2018 •

edited

Loading

hiqsol Mar 19, 2018

samdark Mar 19, 2018

klimov-paul Mar 19, 2018

hiqsol Mar 19, 2018

samdark Mar 19, 2018 •

edited

Loading

samdark Mar 19, 2018 •

edited

Loading

klimov-paul Mar 19, 2018 •

edited

Loading

hiqsol Mar 19, 2018

klimov-paul Mar 19, 2018

[WIP] Added factory functionality, see #25 #26

[WIP] Added factory functionality, see #25 #26

Conversation

hiqsol commented Mar 18, 2018

Choose a reason for hiding this comment

samdark Mar 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samdark commented Mar 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klimov-paul Mar 19, 2018 • edited Loading

Choose a reason for hiding this comment

samdark Mar 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samdark Mar 19, 2018 • edited Loading

Choose a reason for hiding this comment

samdark Mar 19, 2018 • edited Loading

Choose a reason for hiding this comment

klimov-paul Mar 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samdark Mar 18, 2018 •

edited

Loading

klimov-paul Mar 19, 2018 •

edited

Loading

samdark Mar 19, 2018 •

edited

Loading

samdark Mar 19, 2018 •

edited

Loading

samdark Mar 19, 2018 •

edited

Loading

klimov-paul Mar 19, 2018 •

edited

Loading