-
-
Notifications
You must be signed in to change notification settings - Fork 46
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
[WIP] Added factory functionality, see #25 #26
Conversation
$this->parent = $parent; | ||
/// XXX don't get it | ||
/// is it intended to get different instances for same id? | ||
$this->instances[$id] = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. For container it's always the same instance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. It's OK.
Looks good to me. Let's finish it. |
} else { | ||
throw new InvalidConfigException('Unexpected object definition type: ' . gettype($definition)); | ||
} | ||
$object = $this->create($id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How this can function? There is no create()
method declaration either at AbstractContainer
not at Container
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry. Didn't test it at all. It has to be build()
.
I want to get feedback and then do it properly.
*/ | ||
class Factory extends AbstractContainer | ||
{ | ||
public function create($id, array $config = []) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Factory is explicitly non-PSR thing. You've got it right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And if ContainerInterface
seems not suitable for Factory
it can be fixed easily by removing the interface from AbstractContainer
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've posted example config in #25
There should not be Yii::$container
in the end. The Container
should be used in transitional period.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. In 2.2 or later we should drop Yii::$container
and only use dependencies via Injector.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
return $this->buildFromDefinition($config); | ||
} | ||
if ($this->parent !== null) { | ||
return $this->parent->build($id, $config); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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]))
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see.
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:
protected build($id, $config)
protected buildFromDefinition($definition)
public create($id, $config)
- new instance every timeprivate $instances
public get($id)
- same instance every timeNo
sameEveryTime
flag was needed.Factory has
create
but notget
and Container vice versa.