Helper containers #974
Helper containers #974
Conversation
"symfony/event-dispatcher": "~2.1||~3.0", | ||
"symfony/translation": "~2.3||~3.0", | ||
"symfony/yaml": "~2.1||~3.0", | ||
"symfony/class-loader": "~2.1||~3.0", |
stof
Dec 21, 2016
Member
I would actually stop aligning constraint to avoid such kind of diffs
I would actually stop aligning constraint to avoid such kind of diffs
everzet
Dec 21, 2016
•
Author
Member
Agreed. I grown to dislike this style too. Was just going through motions.
Agreed. I grown to dislike this style too. Was just going through motions.
{ | ||
if (method_exists($definition, 'isShared')) { | ||
return $definition->isShared(); | ||
} else if (method_exists($definition, 'getScope')) { |
stof
Dec 21, 2016
Member
if
is enough thanks to the return. And I'm not even sure we need this second check
if
is enough thanks to the return. And I'm not even sure we need this second check
everzet
Dec 21, 2016
Author
Member
To stop static analysis tools (like Scrutinizer) from complaining about non-existing method :(
To stop static analysis tools (like Scrutinizer) from complaining about non-existing method :(
* | ||
* @return bool | ||
* | ||
* @deprecated remove after upgrading to Symfony 2.8+ |
stof
Dec 21, 2016
Member
you should not mark it as deprecated, as you should not call deprecated APIs internally. And this is a private function anyway.
This looks more like a TODO than a deprecation here btw
you should not mark it as deprecated, as you should not call deprecated APIs internally. And this is a private function anyway.
This looks more like a TODO than a deprecation here btw
everzet
Dec 21, 2016
Author
Member
Good shout. I kinda use these interchangeably.
Good shout. I kinda use these interchangeably.
$references = $this->processor->findAndSortTaggedServices($container, self::HELPER_CONTAINER_TAG); | ||
|
||
foreach ($references as $reference) { | ||
if ($this->isDefinitionShared($container->getDefinition($reference))) { |
stof
Dec 21, 2016
Member
should be (string) $reference
as getDefinition
expects a string (the id), not a Reference object
should be (string) $reference
as getDefinition
expects a string (the id), not a Reference object
everzet
Dec 21, 2016
•
Author
Member
Implicit conversion VS explicit conversion. I'm easier with this case than the one you mentioned next. In this particular case it is indeed unclear that what I am expected to pass to $container->getDefinition(...)
is a string, not an object. I'd be happy to make it explicit.
Implicit conversion VS explicit conversion. I'm easier with this case than the one you mentioned next. In this particular case it is indeed unclear that what I am expected to pass to $container->getDefinition(...)
is a string, not an object. I'd be happy to make it explicit.
stof
Dec 21, 2016
Member
Thus Symfony never explicitly mentions that getDefinition
supports castable objects. If it works today, it is purely because of an implementation detail (probably because of using lowercase
which supports it). So it means that passing a castable object is not covered by the BC policy (as it is a case not respecting the documented contract). So making it explicit is necessary to be future-proof
Thus Symfony never explicitly mentions that getDefinition
supports castable objects. If it works today, it is purely because of an implementation detail (probably because of using lowercase
which supports it). So it means that passing a castable object is not covered by the BC policy (as it is a case not respecting the documented contract). So making it explicit is necessary to be future-proof
everzet
Dec 21, 2016
Author
Member
Thanks for explanation. Makes perfect sense and I would change it in this case.
Thanks for explanation. Makes perfect sense and I would change it in this case.
foreach ($references as $reference) { | ||
if ($this->isDefinitionShared($container->getDefinition($reference))) { | ||
throw new WrongServicesConfigurationException(sprintf( | ||
'Container services must not be configured as shared, but `@%s` is.', $reference |
stof
Dec 21, 2016
Member
same here about using the string
same here about using the string
everzet
Dec 21, 2016
Author
Member
This again can be considered implicit VS explicit. Except in this case I obviously tell sprintf
I need a string (@%s
). Hence, I'd argue conversion is already explicit here and additional conversion would only confuse things.
That is unless you have a different tech/clarity reason I haven't thought of yet :)
This again can be considered implicit VS explicit. Except in this case I obviously tell sprintf
I need a string (@%s
). Hence, I'd argue conversion is already explicit here and additional conversion would only confuse things.
That is unless you have a different tech/clarity reason I haven't thought of yet :)
stof
Dec 21, 2016
Member
yeah, I was not sure about it being necessary here.
yeah, I was not sure about it being necessary here.
everzet
Dec 21, 2016
Author
Member
Cool. I'll introduce casting only to the getDefinition(...)
case then.
Cool. I'll introduce casting only to the getDefinition(...)
case then.
* | ||
* @author Konstantin Kudryashov <ever.zet@gmail.com> | ||
*/ | ||
interface ServiceContainer extends ContainerInterface |
stof
Dec 21, 2016
Member
what is the goal of this interface, when the code actually expects Interop\Container\ContainerInterface
anyway ?
what is the goal of this interface, when the code actually expects Interop\Container\ContainerInterface
anyway ?
everzet
Dec 21, 2016
Author
Member
I was trying to give people who don't care about Interop\Container\ContainerInterface
a clear, transparent entry.
I was trying to give people who don't care about Interop\Container\ContainerInterface
a clear, transparent entry.
everzet
Dec 21, 2016
Author
Member
I'm actually not super-attached to it.
I'm actually not super-attached to it.
everzet
Dec 21, 2016
•
Author
Member
Actually, having separate interface breaks the second general principle. So I'll go ahead and drop Behat\Behat\HelperContainer\ServiceContainer
.
Actually, having separate interface breaks the second general principle. So I'll go ahead and drop Behat\Behat\HelperContainer\ServiceContainer
.
@stof thanks for your feedback, as always. I fixed everything you highlighted. |
This looks good - have you checked to see how it interacts with Symfony2Extension? Specialised syntax in the config file, i.e. |
|
||
Rules: | ||
- A single optional container is allowed per suite | ||
- Having container enables you to use its services as context arguments via `@name` syntax |
ciaranmcnulty
Dec 21, 2016
Contributor
'Having a container"
'Having a container"
* | ||
* @author Konstantin Kudryashov <ever.zet@gmail.com> | ||
*/ | ||
final class AggregateFactory implements SuiteScopedResolverFactory |
ciaranmcnulty
Dec 21, 2016
Contributor
I'd rather this was called Composite, after the design pattern. Aggregate risks confusion with the DDD concept, or other usages where an aggregate is a collection
I'd rather this was called Composite, after the design pattern. Aggregate risks confusion with the DDD concept, or other usages where an aggregate is a collection
* | ||
* @author Konstantin Kudryashov <ever.zet@gmail.com> | ||
*/ | ||
final class NullFactory implements SuiteScopedResolverFactory |
ciaranmcnulty
Dec 21, 2016
Contributor
There might be better naming - noop
from the comment makes sense, or maybe empty
?
There might be better naming - noop
from the comment makes sense, or maybe empty
?
everzet
Dec 21, 2016
Author
Member
If we follow the pattern naming from the previous comment, we keep following it for null objects :)
If we follow the pattern naming from the previous comment, we keep following it for null objects :)
*/ | ||
private function createContainerFromString($settings) | ||
{ | ||
if ('@' === mb_substr($settings, 0, 1)) { |
ciaranmcnulty
Dec 21, 2016
Contributor
A similar check takes place in ServicesResolverFactory
- can these be done in one place? Perhaps a value object?
A similar check takes place in ServicesResolverFactory
- can these be done in one place? Perhaps a value object?
everzet
Dec 21, 2016
Author
Member
Potentially. But that's too small of an op to worry about reuse at this point.
Potentially. But that's too small of an op to worry about reuse at this point.
@ciaranmcnulty |
If I have services option and the symfony2extension is there fallback, or are all @-params interpreted as non-Symfony services? |
@ciaranmcnulty service-based resolvers come first as they are more specific (as in local to suites). I'd avoid using |
Step to remove initializers, and use real contexts Behat/Behat#974 https://github.com/Behat/Behat/blob/master/features/helper_containers.feature#L239
Step to remove initializers, and use real contexts Behat/Behat#974 https://github.com/Behat/Behat/blob/master/features/helper_containers.feature#L239
Narrative
Since I introduced support for multiple contexts in 3.0, the most asked question among the Behat newcomers goes along the lines of "how do I share state between multiple contexts?". I was reluctant to provide a systematic, built-in answer to this question for two reasons:
Late push for container interoperability made me believe I finally have an answer to sharing state without giving Behat responsibility it ultimately wasn't built for. On top of that, recently conducted by me coaching, pairing and conversations with people inside and across the communities (thanks @tooky) made me realize that state/behaviour sharing still has very legitimate use-cases in many situations. Especially for Modelling by Example and similar workflows.
I believe this PR to be an elegant answer to the need without breaking very clear responsibility context Behat has. This PR is me finally trying to answer the question most community members keep asking.
General principles
Before starting with the spec, I decided on two balancing principles that the solution must follow:
Enter helper containers
Helper containers are service containers that you can configure to provide shared services for your contexts. And those services indeed be shared between your contexts, with all state and behaviour. Here's simple rules I followed in designing them:
@name
syntaxservices
optionInterop\Container\ContainerInterface
services
setting@name
syntaxHow to use a helper container
The rest of this PR will go in more general details on how to use the feature. For more detailed info and examples, please read the feature file.
Built-in container
The simplest way of using helper container is to define a service (or services) that would be injected (and shared) between your contexts in the scope of a single scenario:
This definition will make Behat automatically instantiate
SharedService
before each scenario and pass it as a first argument (via@...
notation) to bothFirstContext
andSecondContext
- exact same instance.There's even a bit more verbose syntax that allows you to provide custom arguments and even factory method for each class:
Please note that built-in container does not allow you to use defined services as other service arguments. This is to both keep the built-in container away from becoming a full-blown service container and to nudge you towards external container in cases where you do need a deeply nested, highly-wired dependency tree.
External container class
Alternatively, you can use a custom container class that implements
Interop\Container\ContainerInterface
:Don't forget the class itself. You can even place it near you context classes (in
features/bootstrap
):And yes, you can utilise custom factory methods in those too:
Container from extension
The third and the last way is to use containers provided by your favourite extensions. Like that:
The example above doesn't yet work, because it requires extension authors to actually provide that container explicitly from withing their config. But I hope they'll soon follow. Feel free to help them :).
Further details
For further details, as usual, I advise you to dive into the feature file itself. It provides ultimate information, reasoining and examples behind this feature. Oh, yeah, it also is a stellar automated test to make sure I implemented feature correctly in the first place :)
As usual, feedback is greatly appreciated.