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

[DependencyInjection] Support local binding #22187

Closed
wants to merge 7 commits into
base: 3.4
from

Conversation

Projects
None yet
9 participants
@GuilhemN
Contributor

GuilhemN commented Mar 27, 2017

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #22167, #23718
License MIT
Doc PR

A great idea came out on Slack about local bindings.
We could allow injecting services based on type hints on a per service/file basis:

services:
    _defaults:
        bind:
            BarInterface: '@usual_bar'

    Foo:
        bind:
            BarInterface: '@alternative_bar'
            $quz: 'quzvalue'

This way, @usual_bar will be injected in any parameter type hinted as BarInterface (in a constructor or a method signature), but only for this service/file.
Note that bindings could be unused, giving a better solution than #22152 to #21711.

As named parameters are usable in arguments, bindings could be usable in arguments too:

services:
    Foo:
        arguments:
            BarInterface: '@bar'

Named parameters aren't supported yet.

Edit:

Note that bindings could be unused

Current behavior is throwing an exception when a binding is not used at all, in no services of a file if it was inherited from _defaults or in no services created from a prototype.
It will pass if the bindings are all used in at least one service.

@nicolas-grekas

The target looks good to me, named args should be handled to make the feature complete to me.
Here are some friendly comments to help move forward.

Show outdated Hide outdated src/Symfony/Component/DependencyInjection/Compiler/ResolveBindingsPass.php
*/
protected function processValue($value, $isRoot = false)
{
if (!$value instanceof Definition || empty($value->getBindings())) {

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Mar 28, 2017

Member

empty($value->getBindings()) > !$bindings = $value->getBindings()

@nicolas-grekas

nicolas-grekas Mar 28, 2017

Member

empty($value->getBindings()) > !$bindings = $value->getBindings()

Show outdated Hide outdated src/Symfony/Component/DependencyInjection/Compiler/ResolveBindingsPass.php
$class = $parameterBag->resolveValue($class);
}
$bindings = $value->getBindings();

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Mar 28, 2017

Member

to be removed, see above

@nicolas-grekas

nicolas-grekas Mar 28, 2017

Member

to be removed, see above

Show outdated Hide outdated src/Symfony/Component/DependencyInjection/Compiler/ResolveBindingsPass.php
$parameterBag = $this->container->getParameterBag();
if ($class = $value->getClass()) {
$class = $parameterBag->resolveValue($class);

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Mar 28, 2017

Member

should be removed (also from ResolveNamedArgumentsPass): params have already been resolved at this stage.

@nicolas-grekas

nicolas-grekas Mar 28, 2017

Member

should be removed (also from ResolveNamedArgumentsPass): params have already been resolved at this stage.

Show outdated Hide outdated src/Symfony/Component/DependencyInjection/Compiler/ResolveBindingsPass.php
foreach ($calls as $i => $call) {
list($method, $arguments) = $call;
$method = $parameterBag->resolveValue($method);

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Mar 28, 2017

Member

same comment (and same cleanup to do in ResolveNamedArgumentsPass)

@nicolas-grekas

nicolas-grekas Mar 28, 2017

Member

same comment (and same cleanup to do in ResolveNamedArgumentsPass)

ksort($arguments);
$calls[$i][1] = $arguments;
}
}

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Mar 28, 2017

Member

$bindings should be allowed to contain $named args, so that it can be used with scalar/array params also.
$bindings could also be validated: what about throwing when a key doesn't match any existing class/interface? That would help people spot typos easily.

@nicolas-grekas

nicolas-grekas Mar 28, 2017

Member

$bindings should be allowed to contain $named args, so that it can be used with scalar/array params also.
$bindings could also be validated: what about throwing when a key doesn't match any existing class/interface? That would help people spot typos easily.

This comment has been minimized.

@GuilhemN

GuilhemN Mar 28, 2017

Contributor

$bindings should be allowed to contain $named args, so that it can be used with scalar/array params also.

Yes I have to do that :)

$bindings could also be validated: what about throwing when a key doesn't match any existing class/interface? That would help people spot typos easily.

What if the class/interface doesn't exist/is not loaded yet? Is this an unsupported case?

@GuilhemN

GuilhemN Mar 28, 2017

Contributor

$bindings should be allowed to contain $named args, so that it can be used with scalar/array params also.

Yes I have to do that :)

$bindings could also be validated: what about throwing when a key doesn't match any existing class/interface? That would help people spot typos easily.

What if the class/interface doesn't exist/is not loaded yet? Is this an unsupported case?

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Mar 28, 2017

Member

What if the class/interface doesn't exist

unsupported yes, DX is way more important than supporting funky edge cases

@nicolas-grekas

nicolas-grekas Mar 28, 2017

Member

What if the class/interface doesn't exist

unsupported yes, DX is way more important than supporting funky edge cases

Show outdated Hide outdated src/Symfony/Component/DependencyInjection/Loader/XmlFileLoader.php
foreach ($defaults['bindings'] as $binding) {
if (!$binding->hasAttribute('id')) {
throw new InvalidArgumentException(sprintf('The binding service id for tag "<defaults>" in %s must be defined.', $file));

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Mar 28, 2017

Member

should allow $named args

@nicolas-grekas

nicolas-grekas Mar 28, 2017

Member

should allow $named args

Show outdated Hide outdated src/Symfony/Component/DependencyInjection/Loader/XmlFileLoader.php
@@ -333,6 +341,22 @@ private function parseDefinition(\DOMElement $service, $file, array $defaults =
$definition->addAutowiringType($type->textContent);
}
$bindingNodes = $this->getChildren($service, 'binding');
if (empty($bindingNodes) && !empty($defaults['bindings'])) {

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Mar 28, 2017

Member

empty()

@nicolas-grekas
Show outdated Hide outdated src/Symfony/Component/DependencyInjection/Loader/XmlFileLoader.php
$bindings = array();
foreach ($bindingNodes as $binding) {
if (!$binding->hasAttribute('id')) {
throw new InvalidArgumentException(sprintf('The "id" attribute for binding type "%s" for service "%s" in %s must be defined.', $type, (string) $service->getAttribute('id'), $file));

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Mar 28, 2017

Member

$named args

@nicolas-grekas

nicolas-grekas Mar 28, 2017

Member

$named args

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Mar 28, 2017

Member

which means we need a type="service/collection" attribute on xml

@nicolas-grekas

nicolas-grekas Mar 28, 2017

Member

which means we need a type="service/collection" attribute on xml

Show outdated Hide outdated src/Symfony/Component/DependencyInjection/Loader/XmlFileLoader.php
throw new InvalidArgumentException(sprintf('The "id" attribute for binding type "%s" for service "%s" in %s must be defined.', $type, (string) $service->getAttribute('id'), $file));
}
$bindings[$binding->getAttribute('type')] = new Reference($binding->getAttribute('id'));

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Mar 28, 2017

Member

type should renamed to eg "key" (to allow "type" to be used as suggested previously)

@nicolas-grekas

nicolas-grekas Mar 28, 2017

Member

type should renamed to eg "key" (to allow "type" to be used as suggested previously)

Show outdated Hide outdated src/Symfony/Component/DependencyInjection/Loader/YamlFileLoader.php
$definition->setBindings($bindings);
} elseif (isset($defaults['bindings'])) {
$definition->setBindings($defaults['bindings']);

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Mar 28, 2017

Member

I'd expect default bindings to be merged with service ones, don't you?

@nicolas-grekas

nicolas-grekas Mar 28, 2017

Member

I'd expect default bindings to be merged with service ones, don't you?

@nicolas-grekas nicolas-grekas added this to the 3.3 milestone Mar 28, 2017

@GuilhemN

This comment has been minimized.

Show comment
Hide comment
@GuilhemN

GuilhemN Mar 28, 2017

Contributor

@nicolas-grekas thanks for you comments, they should all be fixed now 😃

Contributor

GuilhemN commented Mar 28, 2017

@nicolas-grekas thanks for you comments, they should all be fixed now 😃

Show outdated Hide outdated src/Symfony/Component/DependencyInjection/Compiler/ResolveBindingsPass.php
}
foreach ($bindings as $key => $binding) {
if ($key && '$' === $key[0]) {

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Mar 28, 2017

Member

$key can be a number at this stage (as bogus as it would be) - then $key[0] would break
!isset($key[0]) || '$' === $key[0] instead?

@nicolas-grekas

nicolas-grekas Mar 28, 2017

Member

$key can be a number at this stage (as bogus as it would be) - then $key[0] would break
!isset($key[0]) || '$' === $key[0] instead?

This comment has been minimized.

@GuilhemN

GuilhemN Mar 28, 2017

Contributor

A number should not be supported so I'd say isset($key[0]) && '$' === $key[0], thanks!

@GuilhemN

GuilhemN Mar 28, 2017

Contributor

A number should not be supported so I'd say isset($key[0]) && '$' === $key[0], thanks!

Show outdated Hide outdated src/Symfony/Component/DependencyInjection/Compiler/ResolveBindingsPass.php
continue;
}
if (null !== $binding && !$binding instanceof Reference) {

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Mar 28, 2017

Member

what about allowing Definition also?

@nicolas-grekas

nicolas-grekas Mar 28, 2017

Member

what about allowing Definition also?

This comment has been minimized.

@GuilhemN

GuilhemN Mar 28, 2017

Contributor

👍

@GuilhemN

GuilhemN Mar 28, 2017

Contributor

👍

Show outdated Hide outdated src/Symfony/Component/DependencyInjection/Compiler/ResolveBindingsPass.php
throw new InvalidArgumentException(sprintf('Invalid value for binding type "%s" for service "%s": expected null, an instance of %s or an instance of %s, %s given.', $key, (string) $this->currentId, Reference::class, Definition::class, gettype($binding)));
}
if (!class_exists($key) && !interface_exists($key)) {

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Mar 28, 2017

Member

missing false as 2nd arg of interface_exists

@nicolas-grekas

nicolas-grekas Mar 28, 2017

Member

missing false as 2nd arg of interface_exists

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Mar 28, 2017

Member

but thinking more about this, you told me you would prefer throwing when a binding is never used.
we can do that: we just need to move the deep clone that is done in FileLoader to ResolveDefinitionInheritancePass!
I'd prefer this over this class_exists check myself (because it makes things context-sensitive)

@nicolas-grekas

nicolas-grekas Mar 28, 2017

Member

but thinking more about this, you told me you would prefer throwing when a binding is never used.
we can do that: we just need to move the deep clone that is done in FileLoader to ResolveDefinitionInheritancePass!
I'd prefer this over this class_exists check myself (because it makes things context-sensitive)

Show outdated Hide outdated src/Symfony/Component/DependencyInjection/Compiler/ResolveBindingsPass.php
}
if (null !== $binding && !$binding instanceof Reference && !$binding instanceof Definition) {
throw new InvalidArgumentException(sprintf('Invalid value for binding type "%s" for service "%s": expected null, an instance of %s or an instance of %s, %s given.', $key, (string) $this->currentId, Reference::class, Definition::class, gettype($binding)));

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Mar 28, 2017

Member

for binding "key" (reusing vocabulary from XmlLoader)

@nicolas-grekas

nicolas-grekas Mar 28, 2017

Member

for binding "key" (reusing vocabulary from XmlLoader)

Show outdated Hide outdated src/Symfony/Component/DependencyInjection/Compiler/ResolveBindingsPass.php
}
if (!class_exists($key) && !interface_exists($key)) {
throw new InvalidArgumentException(sprintf('Invalid binding type "%s" for service "%s": this class/interface does not exist.', $key, $this->currentId));

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Mar 28, 2017

Member

s/type/key
this class or interface

@nicolas-grekas

nicolas-grekas Mar 28, 2017

Member

s/type/key
this class or interface

Show outdated Hide outdated src/Symfony/Component/DependencyInjection/Compiler/ResolveBindingsPass.php
}
foreach ($reflectionMethod->getParameters() as $key => $parameter) {
if (isset($arguments[$key])) {

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Mar 28, 2017

Member

array_key_exists

@nicolas-grekas

nicolas-grekas Mar 28, 2017

Member

array_key_exists

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Mar 28, 2017

Member

in fact, AutowirePass has a trick here, which we should account for: null is not autowired, but an empty string with a non-internal type-hint triggers autowiring - we should do the binding here also in this case

@nicolas-grekas

nicolas-grekas Mar 28, 2017

Member

in fact, AutowirePass has a trick here, which we should account for: null is not autowired, but an empty string with a non-internal type-hint triggers autowiring - we should do the binding here also in this case

This comment has been minimized.

@GuilhemN

GuilhemN Mar 29, 2017

Contributor

updated

@GuilhemN

GuilhemN Mar 29, 2017

Contributor

updated

Show outdated Hide outdated ...ony/Component/DependencyInjection/Compiler/ResolveNamedArgumentsPass.php
@@ -50,21 +44,39 @@ protected function processValue($value, $isRoot = false)
$resolvedArguments[] = $argument;
continue;
}
if ('' === $key || '$' !== $key[0]) {
if ('' === $key || ('$' !== $key[0] && !class_exists($key) && !interface_exists($key))) {

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Mar 28, 2017

Member

here we don't need the class_exists check: we should just throw when a binding is not used (as done for $named args already), this will make things context-free again

@nicolas-grekas

nicolas-grekas Mar 28, 2017

Member

here we don't need the class_exists check: we should just throw when a binding is not used (as done for $named args already), this will make things context-free again

Show outdated Hide outdated ...ony/Component/DependencyInjection/Compiler/ResolveNamedArgumentsPass.php
$resolvedArguments[$j] = $argument;
continue 2;
}
}
throw new InvalidArgumentException(sprintf('Unable to resolve service "%s": method "%s::%s" has no argument named "%s". Check your service definition.', $this->currentId, $class, $method, $key));
throw new InvalidArgumentException(sprintf('Unable to resolve service "%s": method "%s::%s" has no argument type hinted as "%s". Check your service definition.', $this->currentId, $value->getClass(), $method, $key));

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Mar 28, 2017

Member

type-hinted (with a dash)

@nicolas-grekas

nicolas-grekas Mar 28, 2017

Member

type-hinted (with a dash)

Show outdated Hide outdated src/Symfony/Component/DependencyInjection/Compiler/ResolveBindingsPass.php
}
$calls = $value->getMethodCalls();
$calls[] = array(null, $value->getArguments()); // constructor

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Mar 28, 2017

Member

instead of null: $class->hasConstructor() ? $class->getConstructor()->name : '__construct'
which implies moving the getReflectionClass call outside of the getReflectionMethod call, which is nice because it is currently in a loop for no reason.
Same on ResolveNamedArgsPass of course.

@nicolas-grekas

nicolas-grekas Mar 28, 2017

Member

instead of null: $class->hasConstructor() ? $class->getConstructor()->name : '__construct'
which implies moving the getReflectionClass call outside of the getReflectionMethod call, which is nice because it is currently in a loop for no reason.
Same on ResolveNamedArgsPass of course.

@lunetics

This comment has been minimized.

Show comment
Hide comment
@lunetics

lunetics Mar 30, 2017

+1, how much is left to do? can't await it!

lunetics commented Mar 30, 2017

+1, how much is left to do? can't await it!

@GuilhemN

This comment has been minimized.

Show comment
Hide comment
@GuilhemN

GuilhemN Mar 30, 2017

Contributor

For me it's ready.
Maybe we can do something about #22187 (comment) but I don't see how to improve this part of the code for now.

Contributor

GuilhemN commented Mar 30, 2017

For me it's ready.
Maybe we can do something about #22187 (comment) but I don't see how to improve this part of the code for now.

@ro0NL
services:
    Foo:
        arguments:
            BarInterface: '@bar'

Not sure if BarInterface has a special meaning here? given only integer or $named arguments are allowed.

edit: ah i see the change in ResolveNamedArguments now 👍

Show outdated Hide outdated src/Symfony/Component/DependencyInjection/Compiler/ResolveBindingsPass.php
continue;
}
if (null !== $binding && !$binding instanceof Reference && !$binding instanceof Definition) {

This comment has been minimized.

@ro0NL

ro0NL Mar 30, 2017

Contributor

shouldnt this check go first?

@ro0NL

ro0NL Mar 30, 2017

Contributor

shouldnt this check go first?

This comment has been minimized.

@GuilhemN

GuilhemN Mar 30, 2017

Contributor

No, any value is allowed for named arguments.
Though i have to add it in the other pass.

@GuilhemN

GuilhemN Mar 30, 2017

Contributor

No, any value is allowed for named arguments.
Though i have to add it in the other pass.

This comment has been minimized.

@ro0NL

ro0NL Mar 30, 2017

Contributor

that is true 👍

@ro0NL

ro0NL Mar 30, 2017

Contributor

that is true 👍

Show outdated Hide outdated src/Symfony/Component/DependencyInjection/Compiler/ResolveBindingsPass.php
$calls = $value->getMethodCalls();
$constructor = $reflectionClass->getConstructor();
$calls[] = array(null !== $constructor ? $constructor->name : '__construct', $value->getArguments()); // constructor

This comment has been minimized.

@ro0NL

ro0NL Mar 30, 2017

Contributor

comment not needed imo. this is obvious.

@ro0NL

ro0NL Mar 30, 2017

Contributor

comment not needed imo. this is obvious.

@nicolas-grekas

here are some comments for another iteration.
can you look at descriptors also please?

Show outdated Hide outdated src/Symfony/Component/DependencyInjection/Compiler/ResolveBindingsPass.php
}
if (!class_exists($key) && !interface_exists($key, false)) {
throw new InvalidArgumentException(sprintf('Invalid binding key "%s" for service "%s": this class/interface does not exist.', $key, $this->currentId));

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Apr 1, 2017

Member

class or interface (instead of the slash)

@nicolas-grekas

nicolas-grekas Apr 1, 2017

Member

class or interface (instead of the slash)

Show outdated Hide outdated src/Symfony/Component/DependencyInjection/Compiler/ResolveBindingsPass.php
$class = $value->getClass();
if (!$class) {
throw new InvalidArgumentException(sprintf('Unable to resolve service "%s": the class is not set.', $this->currentId));

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Apr 1, 2017

Member

no need for this block, autowire pass does:
if (!$reflectionClass = $this->container->getReflectionClass($definition->getClass(), true)) {
which is enough

@nicolas-grekas

nicolas-grekas Apr 1, 2017

Member

no need for this block, autowire pass does:
if (!$reflectionClass = $this->container->getReflectionClass($definition->getClass(), true)) {
which is enough

Show outdated Hide outdated src/Symfony/Component/DependencyInjection/Compiler/ResolveBindingsPass.php
throw new InvalidArgumentException(sprintf('Unable to resolve service "%s": the class is not set.', $this->currentId));
}
if (!$reflectionClass = $this->container->getReflectionClass($class)) {

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Apr 1, 2017

Member

see previous comment

@nicolas-grekas

nicolas-grekas Apr 1, 2017

Member

see previous comment

Show outdated Hide outdated src/Symfony/Component/DependencyInjection/Compiler/ResolveBindingsPass.php
}
if (array_key_exists('$'.$parameter->name, $bindings)) {
$arguments[$key] = $bindings['$'.$parameter->name];

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Apr 1, 2017

Member

to help spot typos in named args, what about keeping a pass-wide property of all named args and their usages, and throw for unused ones at the end of the pass?

@nicolas-grekas

nicolas-grekas Apr 1, 2017

Member

to help spot typos in named args, what about keeping a pass-wide property of all named args and their usages, and throw for unused ones at the end of the pass?

Show outdated Hide outdated ...ony/Component/DependencyInjection/Compiler/ResolveNamedArgumentsPass.php
if (null === $parameters) {
if (!$class) {
throw new InvalidArgumentException(sprintf('Unable to resolve service "%s": the class is not set.', $this->currentId));

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Apr 1, 2017

Member

not needed, already checked just below

@nicolas-grekas

nicolas-grekas Apr 1, 2017

Member

not needed, already checked just below

@GuilhemN

This comment has been minimized.

Show comment
Hide comment
@GuilhemN

GuilhemN Apr 1, 2017

Contributor

can you look at descriptors also please?

As for named arguments the updated arguments are shown by the descriptors, so I agree with you, I don't think we need to support the bindings in the descriptors.

to help spot typos in named args, what about keeping a pass-wide property of all named args and their usages, and throw for unused ones at the end of the pass?

I found a way to throw an error when a binding is used nowhere so no need to do this anymore.

Contributor

GuilhemN commented Apr 1, 2017

can you look at descriptors also please?

As for named arguments the updated arguments are shown by the descriptors, so I agree with you, I don't think we need to support the bindings in the descriptors.

to help spot typos in named args, what about keeping a pass-wide property of all named args and their usages, and throw for unused ones at the end of the pass?

I found a way to throw an error when a binding is used nowhere so no need to do this anymore.

fabpot added a commit that referenced this pull request Apr 4, 2017

feature #22277 [DI] Add "factory" support to named args and autowirin…
…g (nicolas-grekas)

This PR was merged into the 3.3-dev branch.

Discussion
----------

[DI] Add "factory" support to named args and autowiring

| Q             | A
| ------------- | ---
| Branch?       | 3.3
| Bug fix?      | yes
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

To me, factories are expected to work named arguments.
Doing so also unlocks supporting them when autowiring, and will benefit #22187 soon.

Commits
-------

27470de [DI] Add "factory" support to named args and autowiring
Show outdated Hide outdated src/Symfony/Component/DependencyInjection/Compiler/ResolveBindingsPass.php
}
}
if ($value->getFactory() || null !== $constructor) {

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Apr 5, 2017

Member

missing logic update?

@nicolas-grekas

nicolas-grekas Apr 5, 2017

Member

missing logic update?

This comment has been minimized.

@GuilhemN

GuilhemN Apr 5, 2017

Contributor

yes 👍

@GuilhemN

GuilhemN Apr 5, 2017

Contributor

yes 👍

Show outdated Hide outdated ...ony/Component/DependencyInjection/Compiler/ResolveNamedArgumentsPass.php
if (null === $parameters) {
$r = $this->getReflectionMethod($value, $method);
$class = $r instanceof \ReflectionMethod ? $r->class : $this->currentId;
$parameters = $r->getParameters();
}
if ('$' === $key[0]) {

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Apr 5, 2017

Member

breaks when $key === ''?

@nicolas-grekas

nicolas-grekas Apr 5, 2017

Member

breaks when $key === ''?

This comment has been minimized.

@GuilhemN

GuilhemN Apr 5, 2017

Contributor

yes, added a isset check

@GuilhemN

GuilhemN Apr 5, 2017

Contributor

yes, added a isset check

@GuilhemN

This comment has been minimized.

Show comment
Hide comment
@GuilhemN

GuilhemN Apr 5, 2017

Contributor

In case it can convince people, this could be used to have services truly private, not even injectable in third party code:

services:
    _defaults:
        bind:
            Vendor\BarInterface: !service { class: Vendor\Bar }

     Vendor\Foo: ~ # public service needing BarInterface

This could also be used safely in third party bundles as it does not depend on the app configuration (whereas autowiring).

Also when you use autowiring and you want to use a different implementation than the default one:

services:
    _defaults:
        autowire: true
        bind:
            Symfony\Component\Translation\TranslatorInterface: "@app.my_translator"

     App\Foo: ~

This can be used with or without autowiring.

Contributor

GuilhemN commented Apr 5, 2017

In case it can convince people, this could be used to have services truly private, not even injectable in third party code:

services:
    _defaults:
        bind:
            Vendor\BarInterface: !service { class: Vendor\Bar }

     Vendor\Foo: ~ # public service needing BarInterface

This could also be used safely in third party bundles as it does not depend on the app configuration (whereas autowiring).

Also when you use autowiring and you want to use a different implementation than the default one:

services:
    _defaults:
        autowire: true
        bind:
            Symfony\Component\Translation\TranslatorInterface: "@app.my_translator"

     App\Foo: ~

This can be used with or without autowiring.

@nicolas-grekas

👍 LGTM, should make it into 3.3 IMHO, since this belongs to the same scope of features that we added already.

@nicolas-grekas nicolas-grekas modified the milestones: 3.4, 3.3 Apr 11, 2017

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Apr 11, 2017

Member

I'm not convinced we need that.

At some point, falling back to explicit wiring is a better option. Trying to cover all cases with abstract configuration makes configuration look awful and code more complex than needed.

When reading the config for the first, it looks like random keywords assembled in a YAML snippet.

Let's see how people are using the features already introduced in 3.3 first. And let's discuss further "improvements/changes" for 3.4.

Member

fabpot commented Apr 11, 2017

I'm not convinced we need that.

At some point, falling back to explicit wiring is a better option. Trying to cover all cases with abstract configuration makes configuration look awful and code more complex than needed.

When reading the config for the first, it looks like random keywords assembled in a YAML snippet.

Let's see how people are using the features already introduced in 3.3 first. And let's discuss further "improvements/changes" for 3.4.

@lunetics

This comment has been minimized.

Show comment
Hide comment
@lunetics

lunetics Apr 11, 2017

lunetics commented Apr 11, 2017

@GuilhemN

This comment has been minimized.

Show comment
Hide comment
@GuilhemN

GuilhemN Apr 11, 2017

Contributor

@fabpot bindings solves problems that can't be solved using autowiring and is far more convenient than manual wiring, an example is in the demo with scalar arguments in an autowired prototype, bindings would allow to simplify its file using:

services:
    AppBundle\:
        # Register all classes in the src/AppBundle directory as services
        resource: '../../src/AppBundle/{EventListener,Form/Type,Security,Twig,Utils}'
        bind:
            $locales: '%app_locales%'
            $sender: '%app.notifications.email_sender%'

Being able to define arguments for an entire file while they can be used only in some services, in other words, optional arguments is one of the major goal, which is not doable at all currently.

This is not only meant to solve autowiring conflicts, it's also pretty useful used alone.

Contributor

GuilhemN commented Apr 11, 2017

@fabpot bindings solves problems that can't be solved using autowiring and is far more convenient than manual wiring, an example is in the demo with scalar arguments in an autowired prototype, bindings would allow to simplify its file using:

services:
    AppBundle\:
        # Register all classes in the src/AppBundle directory as services
        resource: '../../src/AppBundle/{EventListener,Form/Type,Security,Twig,Utils}'
        bind:
            $locales: '%app_locales%'
            $sender: '%app.notifications.email_sender%'

Being able to define arguments for an entire file while they can be used only in some services, in other words, optional arguments is one of the major goal, which is not doable at all currently.

This is not only meant to solve autowiring conflicts, it's also pretty useful used alone.

@simensen

This comment has been minimized.

Show comment
Hide comment
@simensen

simensen Apr 24, 2017

Contributor

I think this solution looks interesting and I think it is something that makes autowiring more feasible. We can use alias to bind a specific implementation to an interface. Without contextual binding of somesort, you'll no longer be able to use autowiring at all (as far as I can tell) the instant you end up with more than one implementation of a given interface added to the container.

(I may be oversimplifying things here, but I think this is mostly accurate?)

Contributor

simensen commented Apr 24, 2017

I think this solution looks interesting and I think it is something that makes autowiring more feasible. We can use alias to bind a specific implementation to an interface. Without contextual binding of somesort, you'll no longer be able to use autowiring at all (as far as I can tell) the instant you end up with more than one implementation of a given interface added to the container.

(I may be oversimplifying things here, but I think this is mostly accurate?)

@lunetics

This comment has been minimized.

Show comment
Hide comment
@lunetics

lunetics Apr 24, 2017

@simensen exactly, currently you are bound to one! concrete implementation (via alias, which in addition is container-wide).

Local binding is config-file wise (as explained before)

lunetics commented Apr 24, 2017

@simensen exactly, currently you are bound to one! concrete implementation (via alias, which in addition is container-wide).

Local binding is config-file wise (as explained before)

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Apr 24, 2017

Member

Rebased and green.

Member

nicolas-grekas commented Apr 24, 2017

Rebased and green.

@simensen

This comment has been minimized.

Show comment
Hide comment
@simensen

simensen Apr 25, 2017

Contributor

I'd also like to point out that this isn't just for cases where you are defining an implementation of an abstract class or interface. It is also for the case where the same class might be configured multiple ways. So although the simple interface binding might not be required, this really is required if you want to have different instances of the same class used differently.

Contributor

simensen commented Apr 25, 2017

I'd also like to point out that this isn't just for cases where you are defining an implementation of an abstract class or interface. It is also for the case where the same class might be configured multiple ways. So although the simple interface binding might not be required, this really is required if you want to have different instances of the same class used differently.

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Apr 25, 2017

Member

PR updated to use "bind" instead of "bindings" in Yaml and Xml definitions (as discussed with @weaverryan offline)

Member

nicolas-grekas commented Apr 25, 2017

PR updated to use "bind" instead of "bindings" in Yaml and Xml definitions (as discussed with @weaverryan offline)

@GuilhemN

This comment has been minimized.

Show comment
Hide comment
@GuilhemN

GuilhemN Apr 25, 2017

Contributor

PR updated to use "bind" instead of "bindings" in Yaml and Xml definitions (as discussed with @weaverryan offline)

I asked @nicolas-grekas to understand this choice and here are the reasons: bind is a verb/an action which is more consistent with autowire, autoconfigure.
bind is also more usual in programming \Closure::bind(), object.bind(), ... so it's easier to understand and remember for the user.

Contributor

GuilhemN commented Apr 25, 2017

PR updated to use "bind" instead of "bindings" in Yaml and Xml definitions (as discussed with @weaverryan offline)

I asked @nicolas-grekas to understand this choice and here are the reasons: bind is a verb/an action which is more consistent with autowire, autoconfigure.
bind is also more usual in programming \Closure::bind(), object.bind(), ... so it's easier to understand and remember for the user.

@lunetics

This comment has been minimized.

Show comment
Hide comment
@lunetics

lunetics Apr 25, 2017

I wish for 3.3 🙏

lunetics commented Apr 25, 2017

I wish for 3.3 🙏

@GuilhemN

This comment has been minimized.

Show comment
Hide comment
@GuilhemN

GuilhemN May 5, 2017

Contributor

Rebased on master: bound arguments aren't allowed anymore on child definitions to be consistent with #22563.

TypedReference are now supported, bindings can be used for service subscribers.
However, I had to make a specific implementation to support bindings in controller arguments, but it provides a real benefit:

services:
    AppBundle\Controller\LuckyController:
        tags: [controller.service_arguments]
        bind:
            $logger: @monolog.logger.doctrine

    # instead of
    AppBundle\Controller\LuckyController:
        tags:
            - name: controller.service_arguments
              action: numberAction
              argument: logger
              id: monolog.logger.doctrine
Contributor

GuilhemN commented May 5, 2017

Rebased on master: bound arguments aren't allowed anymore on child definitions to be consistent with #22563.

TypedReference are now supported, bindings can be used for service subscribers.
However, I had to make a specific implementation to support bindings in controller arguments, but it provides a real benefit:

services:
    AppBundle\Controller\LuckyController:
        tags: [controller.service_arguments]
        bind:
            $logger: @monolog.logger.doctrine

    # instead of
    AppBundle\Controller\LuckyController:
        tags:
            - name: controller.service_arguments
              action: numberAction
              argument: logger
              id: monolog.logger.doctrine

@GuilhemN GuilhemN changed the base branch from master to 3.4 May 18, 2017

code changed, second round needed

@nicolas-grekas nicolas-grekas self-requested a review Jul 13, 2017

@nicolas-grekas

some remaining minor comments, still looks good to me

Show outdated Hide outdated src/Symfony/Component/DependencyInjection/Argument/BoundArgument.php
*/
final class BoundArgument implements ArgumentInterface
{
private static $count = 0;

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jul 17, 2017

Member

I propose to name this $sequence

@nicolas-grekas

nicolas-grekas Jul 17, 2017

Member

I propose to name this $sequence

Show outdated Hide outdated ...ony/Component/DependencyInjection/Compiler/ResolveNamedArgumentsPass.php
if ($key === '$'.$p->name) {
$typeHint = ProxyHelper::getTypeHint($r, $p, true);
if ($typeHint === $key) {

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jul 17, 2017

Member

ProxyHelper::getTypeHint(...) === $key

@nicolas-grekas

nicolas-grekas Jul 17, 2017

Member

ProxyHelper::getTypeHint(...) === $key

Show outdated Hide outdated ...Tests/DependencyInjection/RegisterControllerArgumentLocatorsPassTest.php
$this->assertEquals($expected, $locator->getArgument(0));
}
public function argumentNameProvider()

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jul 17, 2017

Member

should be named eg provideBindings

@nicolas-grekas

nicolas-grekas Jul 17, 2017

Member

should be named eg provideBindings

/**
* Sets bindings.
*

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jul 17, 2017

Member

Maybe add some explanations, eg:
Bindings map $named arguments or type-hints to values that should be injected in the matching parameters of the constructor and of setters.

@nicolas-grekas

nicolas-grekas Jul 17, 2017

Member

Maybe add some explanations, eg:
Bindings map $named arguments or type-hints to values that should be injected in the matching parameters of the constructor and of setters.

This comment has been minimized.

@GuilhemN

GuilhemN Jul 23, 2017

Contributor

I adapted a bit your proposal to:

Bindings map $named or FQCN arguments to values that should be
injected in the matching parameters (of the constructor, of methods
called and of controller actions).

Ok for you?

@GuilhemN

GuilhemN Jul 23, 2017

Contributor

I adapted a bit your proposal to:

Bindings map $named or FQCN arguments to values that should be
injected in the matching parameters (of the constructor, of methods
called and of controller actions).

Ok for you?

Show outdated Hide outdated src/Symfony/Component/DependencyInjection/Loader/XmlFileLoader.php
@@ -165,13 +166,17 @@ private function getServiceDefaults(\DOMDocument $xml, $file)
}
$defaults = array(
'tags' => $this->getChildren($defaultsNode, 'tag'),
'bind' => array_map(function ($v) {

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jul 17, 2017

Member

should be good on one line

@nicolas-grekas

nicolas-grekas Jul 17, 2017

Member

should be good on one line

Show outdated Hide outdated src/Symfony/Component/DependencyInjection/Loader/XmlFileLoader.php
@@ -344,6 +356,14 @@ private function parseDefinition(\DOMElement $service, $file, array $defaults)
$definition->addAutowiringType($type->textContent);
}
$bindings = $this->getArgumentsAsPhp($service, 'bind', $file);
if (isset($defaults['bind'])) {
$bindings = array_merge(unserialize(serialize($defaults['bind'])), $bindings);

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jul 17, 2017

Member

small comment needed to explain the deep clone

@nicolas-grekas

nicolas-grekas Jul 17, 2017

Member

small comment needed to explain the deep clone

Show outdated Hide outdated src/Symfony/Component/DependencyInjection/Loader/YamlFileLoader.php
throw new InvalidArgumentException(sprintf('Parameter "bind" in "_defaults" must be an array in %s. Check your YAML syntax.', $file));
}
$defaults['bind'] = array_map(function ($v) {

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jul 17, 2017

Member

one line

@nicolas-grekas
Show outdated Hide outdated src/Symfony/Component/DependencyInjection/Loader/YamlFileLoader.php
@@ -516,6 +533,19 @@ private function parseDefinition($id, $service, $file, array $defaults)
}
}
if (isset($defaults['bind']) || isset($service['bind'])) {
$bindings = isset($defaults['bind']) ? unserialize(serialize($defaults['bind'])) : array();

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jul 17, 2017

Member

comment need

@nicolas-grekas

nicolas-grekas Jul 17, 2017

Member

comment need

@GuilhemN

This comment has been minimized.

Show comment
Hide comment
@GuilhemN

GuilhemN Jul 23, 2017

Contributor

@nicolas-grekas I just fixed your comments, thanks for the review!

Contributor

GuilhemN commented Jul 23, 2017

@nicolas-grekas I just fixed your comments, thanks for the review!

@GuilhemN

This comment has been minimized.

Show comment
Hide comment
@GuilhemN

GuilhemN Jul 24, 2017

Contributor

The build failure is because this is not yet in master.

Contributor

GuilhemN commented Jul 24, 2017

The build failure is because this is not yet in master.

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Jul 31, 2017

Member

this PR description could be enhanced to emphasis the use of mapping arguments to parameters, because that's a common need, see #23718

Member

nicolas-grekas commented Jul 31, 2017

this PR description could be enhanced to emphasis the use of mapping arguments to parameters, because that's a common need, see #23718

@javiereguiluz

This comment has been minimized.

Show comment
Hide comment
@javiereguiluz

javiereguiluz Jul 31, 2017

Member

Can we improve the DX of this feature? If this comment by @GuilhemN is correct, this is how the feature works now:

services:
    _defaults:
        autowire: true
        autoconfigure: true
        public: false

        bind:
            $argument1: '%kernel.project_dir%'
            $argument2: '%app.container_param%'
            $argument3: '%app.another_param%'

    # ...

This is my proposal:

services:
    _defaults:
        autowire: true
        autoconfigure: true
        public: false
        arguments:
            $argument1: '%kernel.project_dir%'
            $argument2: '%app.container_param%'
            $argument3: '%app.another_param%'

    # ...

My reasoning:

  • This feature is about defining the default value of scalar arguments, so "_defaults.arguments" sounds natural.
  • We wouldn't introduce yet another config option ("bind").
Member

javiereguiluz commented Jul 31, 2017

Can we improve the DX of this feature? If this comment by @GuilhemN is correct, this is how the feature works now:

services:
    _defaults:
        autowire: true
        autoconfigure: true
        public: false

        bind:
            $argument1: '%kernel.project_dir%'
            $argument2: '%app.container_param%'
            $argument3: '%app.another_param%'

    # ...

This is my proposal:

services:
    _defaults:
        autowire: true
        autoconfigure: true
        public: false
        arguments:
            $argument1: '%kernel.project_dir%'
            $argument2: '%app.container_param%'
            $argument3: '%app.another_param%'

    # ...

My reasoning:

  • This feature is about defining the default value of scalar arguments, so "_defaults.arguments" sounds natural.
  • We wouldn't introduce yet another config option ("bind").
@GuilhemN

This comment has been minimized.

Show comment
Hide comment
@GuilhemN

GuilhemN Jul 31, 2017

Contributor
services:
    _defaults:
        autowire: true
        autoconfigure: true
        public: false

    bind:
        $argument1: '%kernel.project_dir%'
        $argument2: '%app.container_param%'
        $argument3: '%app.another_param%'

    # ...

bind is used under _defaults or in a service definition, it's not a special option.

@javiereguiluz arguments are not the same, they can only be used in the constructor while bindings are meant to be used in the constructor, method calls and in action arguments.
Bindings are also optional (no need to be used in all services) while arguments must be used in every service of the file/resource.

This feature is about defining the default value of scalar arguments, so "_defaults.arguments" sounds natural.

That's just one use case :) It can also be used in libraries as a totally predictable autowiring, to deal with multiple instance of the same class, to have a nicer syntax to replace services in action arguments and so on.
And as I said earlier arguments are far more limited, having a different behavior in _defaults for arguments would be weird.

Contributor

GuilhemN commented Jul 31, 2017

services:
    _defaults:
        autowire: true
        autoconfigure: true
        public: false

    bind:
        $argument1: '%kernel.project_dir%'
        $argument2: '%app.container_param%'
        $argument3: '%app.another_param%'

    # ...

bind is used under _defaults or in a service definition, it's not a special option.

@javiereguiluz arguments are not the same, they can only be used in the constructor while bindings are meant to be used in the constructor, method calls and in action arguments.
Bindings are also optional (no need to be used in all services) while arguments must be used in every service of the file/resource.

This feature is about defining the default value of scalar arguments, so "_defaults.arguments" sounds natural.

That's just one use case :) It can also be used in libraries as a totally predictable autowiring, to deal with multiple instance of the same class, to have a nicer syntax to replace services in action arguments and so on.
And as I said earlier arguments are far more limited, having a different behavior in _defaults for arguments would be weird.

@nicolas-grekas

Small remaining comments.
This should be merged now, we already identified several cases where this feature is useful and would be a nice addition.
If we don't merge, we cannot improve it later if needed - nor experience it for real.

Show outdated Hide outdated src/Symfony/Component/DependencyInjection/Compiler/ResolveBindingsPass.php
*/
protected function processValue($value, $isRoot = false)
{
if ($value instanceof TypedReference) {

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Aug 7, 2017

Member

&& $value->getType() === (string) $value because typed reference can be configured with an explicit id (as done in autowiring pass L330 in getAutowiredReference)
needs a test case

@nicolas-grekas

nicolas-grekas Aug 7, 2017

Member

&& $value->getType() === (string) $value because typed reference can be configured with an explicit id (as done in autowiring pass L330 in getAutowiredReference)
needs a test case

Show outdated Hide outdated src/Symfony/Component/DependencyInjection/Compiler/ResolveBindingsPass.php
return parent::processValue($value, $isRoot);
}
foreach ($value->getBindings() as $key => $binding) {

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Aug 7, 2017

Member

foreach ($bindings as ...

@nicolas-grekas

nicolas-grekas Aug 7, 2017

Member

foreach ($bindings as ...

Show outdated Hide outdated src/Symfony/Component/DependencyInjection/Loader/XmlFileLoader.php
$bindings = $this->getArgumentsAsPhp($service, 'bind', $file);
if (isset($defaults['bind'])) {
// deep clone, to avoid multiple process of the same instance in the
// passes

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Aug 7, 2017

Member

comment should be on one line
// deep clone, to avoid processing the same definitions several times in compiler passes

@nicolas-grekas

nicolas-grekas Aug 7, 2017

Member

comment should be on one line
// deep clone, to avoid processing the same definitions several times in compiler passes

Show outdated Hide outdated src/Symfony/Component/DependencyInjection/Loader/YamlFileLoader.php
@@ -519,6 +534,22 @@ private function parseDefinition($id, $service, $file, array $defaults)
}
}
if (isset($defaults['bind']) || isset($service['bind'])) {
// deep clone, to avoid multiple process of the same instance in the

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Aug 7, 2017

Member

see above

@nicolas-grekas

nicolas-grekas Aug 7, 2017

Member

see above

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Aug 8, 2017

Member

@javiereguiluz here is a typical example:

services:
  _defaults:
    bind:
      $projectDir: '%kernel.project_dir%'

This means that any arguments (constructor or setter) that are named $projectDir for services defined in the current file will get the kernel.project_dir value, provided the services don't themselves explicitly set any other values of course.

We could even put that specific line in the default flex config. (no, because that would trigger an "unused binding" kind of exception, we preferred to ease spotting typos here)

Do we get your vote? :)

Member

nicolas-grekas commented Aug 8, 2017

@javiereguiluz here is a typical example:

services:
  _defaults:
    bind:
      $projectDir: '%kernel.project_dir%'

This means that any arguments (constructor or setter) that are named $projectDir for services defined in the current file will get the kernel.project_dir value, provided the services don't themselves explicitly set any other values of course.

We could even put that specific line in the default flex config. (no, because that would trigger an "unused binding" kind of exception, we preferred to ease spotting typos here)

Do we get your vote? :)

@GuilhemN

This comment has been minimized.

Show comment
Hide comment
@GuilhemN

GuilhemN Aug 8, 2017

Contributor

@nicolas-grekas comments fixed :)

Contributor

GuilhemN commented Aug 8, 2017

@nicolas-grekas comments fixed :)

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas
Member

nicolas-grekas commented Aug 9, 2017

Thank you @GuilhemN.

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