Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[DependencyInjection] Make method (setter) autowiring configurable#20167

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
fabpot merged 1 commit intosymfony:masterfromdunglas:autowire_setter_config
Dec 13, 2016

Conversation

@dunglas
Copy link
Member

@dunglasdunglas commentedOct 5, 2016•
edited
Loading

QA
Branch?master
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?maybe?
Tests pass?yes
Fixed tickets#19631
LicenseMIT
Doc PRsymfony/symfony-docs#7041

Follow up of#19631. Implements#19631 (comment):

Edit: the last supported format:

services:foo:class:Fooautowire:['__construct', 'set*']# Autowire constructor and all settersautowire:true# Converted by loaders in `autowire: ['__construct']` for BCautowire:['foo', 'bar']# Autowire only `foo` and `bar` methods

Outdated:

autowire:true# constructor autowiringautowire:[__construct, setFoo, setBar]# autowire whitelisted methods onlyautowire:'*'# autowire constructor + every setters (following existing rules for setters autowiring)
  • Allow to specify the list of methods in the XML loader
  • Add tests for the YAML loader

chalasr, rvanlaak, ro0NL, yceruto, and dkarlovi reacted with thumbs up emoji
* Is the definition autowired?
*
* @return bool
*/
Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Should I deprecate this method?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Not sure it's worth it.

dunglas reacted with thumbs up emoji
@fabpot
Copy link
Member

👍

@dunglas
Copy link
MemberAuthor

The syntax for the XML loader is:

<?xml version="1.0" encoding="utf-8"?><containerxmlns="http://symfony.com/schema/dic/services"xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"xsi:schemaLocation="http://symfony.com/schema/dic/services http://symfony.com/schema/dic/services/services-1.0.xsd">    <services>        <serviceid="autowire_star"class="Foo"autowire="*" />        <serviceid="autowire_array"class="Foo">            <autowire>setFoo</autowire>            <autowire>bar</autowire>        </service>    </services></container>

Does it looks good to everybody?

sstok reacted with thumbs up emoji

$methods =array();
foreach ($autowiredas$methodName) {
if ($reflectionClass->hasMethod($methodName)) {
$methods[] =$reflectionClass->getMethod($methodName);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

This won't work when using['__construct'] right?

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Copy link
Contributor

@GuilhemNGuilhemNOct 7, 2016•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Sure but it will add its arguments as a call instead of constructor arguments. And it may create weird bugs and not work at all if there are mandatory constructor arguments.

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Good catch I'll update that.

foreach ($container->getDefinitions()as$id =>$definition) {
if ($definition->isAutowired()) {
$this->completeDefinition($id,$definition);
if (false !==$autowired =$definition->getAutowired()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

What about keep using$definition->isAutowired() here (as it stay valid), remove the newly added$autowired argument fromcompleteDefinition() and use$autowired = $defintion->getAutowired(); directly into?

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

It adds some extra method calls for no reason. I prefer to keep it as is.

Copy link
Member

@chalasrchalasrOct 8, 2016•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

The current adds an extra argument tocompleteDefinition for no reason, it can be done in the method directly as you have theDefinition, and the check is doing exactly what the method call would do thus ... Up to you.


foreach (self::getSetters($reflectionClass)as$reflectionMethod) {
$metadata[$reflectionMethod->name] =self::getResourceMetadataForMethod($reflectionMethod);
foreach ($reflectionClass->getMethods(\ReflectionMethod::IS_PUBLIC)as$reflectionMethod) {
Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

@weaverryan can you take a look at this change?

Because it's now possible to autowire any method (not just only setters), I think that there is now other solution than watching all defined methods.

Copy link
Contributor

@GuilhemNGuilhemNOct 8, 2016•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

What about allowing masks? (egset*).

rvanlaak and dunglas reacted with thumbs up emoji
Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

We can indeed allow masks in the configuration, but it's not related to this code block (it is not aware of the configuration so it needs to watch all methods anyway).

GuilhemN reacted with thumbs up emoji
@dunglas
Copy link
MemberAuthor

ping @symfony/deciders. We need to choose between merging this one or reverting setter autowiring before the 3.2 release.

*/
publicfunctiongetMethods(\ReflectionClass$reflectionClass,array$autowired)
{
if (is_array($autowired)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

It's always an array as it is typehinted

*
* @return \ReflectionMethod[]
*/
publicfunctiongetMethods(\ReflectionClass$reflectionClass,array$autowired)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I think this is meant to be private

@Tobion
Copy link
Contributor

Tobion commentedOct 13, 2016•
edited
Loading

The XML loader should probably forbid to use both the autowire attribute and elements at the same time. Stuff like that doesn't make sense:

<?xml version="1.0" encoding="utf-8"?><containerxmlns="http://symfony.com/schema/dic/services"xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"xsi:schemaLocation="http://symfony.com/schema/dic/services http://symfony.com/schema/dic/services/services-1.0.xsd">    <services>        <serviceid="autowire_star"class="Foo"autowire="*">           <autowire>setFoo</autowire>            <autowire>bar</autowire>        </service>        <serviceid="autowire_array"class="Foo"autowire="false">            <autowire>setFoo</autowire>            <autowire>bar</autowire>        </service>    </services></container>


<xsd:simpleTypename="boolean_or_star">
<xsd:restrictionbase="xsd:string">
<xsd:patternvalue="(%.+%|true|false|\*)" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

A boolean is XML schema is also allowed to be 1 and 0. So this might break BC.

Copy link
MemberAuthor

@dunglasdunglasOct 13, 2016•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

This isn't a BC break because it was already not using the boolean type of XML schema but the one locally defined. I've just copied, pasted and extended the current type definition to allow using a* character:

  <xsd:simpleTypename="boolean">    <xsd:restrictionbase="xsd:string">      <xsd:patternvalue="(%.+%|true|false)" />    </xsd:restriction>  </xsd:simpleType>

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I see, it's to allow parameters. I'm pretty sure allowing dynamic values for autowiring should not be allowed and makes no sense after all. I don't see the point of it.

Copy link
MemberAuthor

@dunglasdunglasOct 13, 2016•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

What do you mean by dynamic values? It allows to write:

<serviceclass="Foo"autowire="true"/><!-- Autowire constructor only (BC)--><!-- or--><serviceclass="Foo"autowire="*"/><!-- Autowire constructor + setters-->

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I mean parameters:<service autowire="%param%"/>

Copy link
MemberAuthor

@dunglasdunglasOct 13, 2016•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Ok got it. But we cannot change it because it was already allowed since 2.8 (because it was using the localboolean type like all others parameters).

@Tobion
Copy link
Contributor

So if you explicitly autowire a method that has no arguments it is basically the same as a normal call configuration to this method? I suspect people will then misuse autowiring just to call certain methods since there are now 2 ways to do that.

@dunglas
Copy link
MemberAuthor

dunglas commentedOct 13, 2016•
edited
Loading

@Tobion thanks for the review, comments fixed now.

So if you explicitly autowire a method that has no arguments it is basically the same as a normal call configuration to this method? I suspect people will then misuse autowiring just to call certain methods since there are now 2 ways to do that.

I think that it's not the same intent:

  • When you use<autowire>, it means "always fill parameters of this method, if I add or remove some, it must not fail"
  • When you use<call>, it's the old good call without any magic

But again, and as pointed in the doc, I would discourage end users to use setter autowiring. It's a feature designed for creators of RAD frameworks and bundles.

@Tobion
Copy link
Contributor

Tobion commentedOct 13, 2016•
edited
Loading

When you use , it means "always fill parameters of this method, if I add or remove some, it must not fail"

Fine for me.

One more thing. I think we should make the programmaticDefinition methods more explicit and not use a multivalued@param bool|string|string[] $autowired parameter. IMO we need to distinguish between configuration and programmatic API.

autowire:true# constructor autowiringautowire:[__construct, setFoo, setBar]# autowire whitelisted methods onlyautowire:'*'# autowire constructor + every setters (following existing rules for setters autowiring)

makes sense as@dunglas and@nicolas-grekas pointed out. But using a magic* in a programmers API like

$fooDefinition = new Definition('Foo');$fooDefinition->setAutowired('*');

does not makse sense.

So I'd prefer to change the methods on theDefinition class similar to what@ro0NL suggested in#19631 (comment).

$definition->setAutowired(Definition::CONSTRUCTOR|Definition::SETTERS);$definition->setAutowiredMethods(array('setFoo', 'notASetter'));$definition->disableAutowiring();

The current yaml/xml loaders would just map to this explicit API.

@dunglas
Copy link
MemberAuthor

dunglas commentedOct 14, 2016•
edited
Loading

It will be difficult to make this new API backward compatible with the old one. Currentlyautowired is a boolean.

@ro0NL
Copy link
Contributor

ro0NL commentedOct 16, 2016•
edited
Loading

@dunglas what about this approach?

$def->setAutowired(false);// no autowiring$def->setAutorwired(true/**Def::CONSTRUCTOR*/);// constructor autowiring$def->setAutowired(Def::SETTERS);// setter autowiring$def->setAutorwired(array('foo'));// whitelist autowiring$def->setAutorwired(Def::CONSTRUCTOR|Def::SETTERS);// combined autowiring// optional: combined + whitelist autowiring$def->setAutorwired(Def::CONSTRUCTOR|Def::SETTERS|Def::WHITELIST,array('foo'));
autowire:false# no autowiringautowire:true | 'constructor' | constant('Def::CONSTRUCTOR')# constructor autowiringautowire:'setters'| constant('Def::SETTERS')# setter autowiringautowire:# whitelist autowiring  -fooautowire:# combined autowiringconstructor:truesetters:true# optional: combined + whitelist autowiringautowire:constructor:truesetters:truewhitelist:    -foo

@dunglas
Copy link
MemberAuthor

@ro0NL I don't want to change the format for XML and YAML because we already have reached a consensus.

Your proposal for the PHP API doesn't fix the issue raised by@Tobion: thesetAutowire method still accept several types of parameters (string,bool andstring[]); exactly like the current implementation.

Using a bit field isn't the right choice here: you cannot accumulate options. You can only choose one option between constructor autowiring, constructor + setter autowiring or a list of methods to autowire.

IMO the current implementation is good enough even if I would prefer accepting only one type forsetAutowire. It's expressive, easy to learn and flexible. All loaders (PHP, XML and YAML) also have a similar API (better learning curve).

Maybe can we just introduce aDefinition::SETTER_AUTOWIRING = '*' to avoid using a special raw string in the PHP API.

@ro0NL
Copy link
Contributor

ro0NL commentedOct 16, 2016•
edited
Loading

Fair enough. Just saying im not sure* meaningconstructor + setters is intuitive.. especially as we shouldnt promote setter injection.. right? This is making it more or less special.

autowire: '*' vs. autowire: { constructor: true, setters: true }

I'd prefer the latter in terms of less magic, extensibility and verbose configuration. But you're right, we've already reached consensus before. Just thought of this today :)

class AutowirePassimplements CompilerPassInterface
{
/**
* @var ContainerBuilder

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

really required? isn't the type hint on the constructor enough?

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Without it PHPStorm's autocompletion doesn't work, but I can remove it :)

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

There is no constructor btw (it's why it's required).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

oh right, the function I looked at isprocess
ok then

* @throws RuntimeException
*/
privatefunctioncompleteDefinition($id,Definition$definition,$autowired)
privatefunctioncompleteDefinition($id,Definition$definition,array$autowired =array('__construct'))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

since this is private, let's make the arg mandatory to make the calling code easier to read?

$this->container =$container;
foreach ($container->getDefinitions()as$id =>$definition) {
if (false !==$autowired =$definition->getAutowired()) {
if (!empty($autowired =$definition->getAutowired())) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I'm not a bug fan of usingempty. I'd preferif ($autowired = $definition->getAutowired()) {

@nicolas-grekas
Copy link
Member

nicolas-grekas commentedNov 1, 2016 via email

Final thoughts; my vote goes to setAutowire($boolOrArray)
Wouldn't be mine: union types require higher cyclomatic complexity. Havingsimple types looks better to me.

@ro0NL
Copy link
Contributor

ro0NL commentedNov 1, 2016•
edited
Loading

Perhaps should be avoided, but in this case (to me) it makes most sense from aconfiguring perspective.

Eventually a YAML/XML/PHP definition represents the same thing, and therefore should have similar (if not the same) schema/API imo.

edit: on 2nd thought :):) maybe choose the desired PHP API now (eg.setAutowiredMethods(array $methods)) and extract PHP configuration (eg.return [];) later on? Does that make sense?

@nicolas-grekas
Copy link
Member

My vote would be for reverting#17608 and merge this one for 3.3.
This one has a high impact on DX, yet DX needs some time to mature IMHO, and this maybe be too fresh for 3.2.

@dunglas
Copy link
MemberAuthor

I agree, it's too late for 3.2.

dunglas added a commit that referenced this pull requestNov 2, 2016
… add setter injection support (dunglas)" (nicolas-grekas)This PR was merged into the 3.2-dev branch.Discussion----------Revert "feature#17608 [DependencyInjection] Autowiring: add setter injection support (dunglas)"| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | yes| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets | -| License       | MIT| Doc PR        | -This reverts commit7eab6b9, reversingchanges made to35f201f.As discussed in#20167Commits-------bf91eda Revert "feature#17608 [DependencyInjection] Autowiring: add setter injection support (dunglas)"
@dunglas
Copy link
MemberAuthor

dunglas commentedNov 2, 2016•
edited
Loading

I've rebased the PR and updated method's names.

I've added two new methodssetAutowiredMethods(array $autowiredMethods) andgetAutowiredMethods() : array because the naming is better and it allows to remove the union type.

However I've keptisAutowired() : bool andsetAutowired(bool $autowired) (without deprecating them) because it doesn't hurt and it looks better in term of DX when not using method autowiring. And method autowiring should almost never be used when not developing a library or a framework.

$definition->addAutowiringType($type->textContent);
}

$autowireTags =array();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

$autowiredMethods?

Copy link
MemberAuthor

@dunglasdunglasNov 3, 2016•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I prefer to keep the current name here because this var contains the value of the XML tag.

foreach ($container->getDefinitions()as$id =>$definition) {
if ($definition->isAutowired()) {
$this->completeDefinition($id,$definition);
if ($autowired =$definition->getAutowiredMethods()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

$autowiredMethods?

* @throws RuntimeException
*/
privatefunctioncompleteDefinition($id,Definition$definition)
privatefunctioncompleteDefinition($id,Definition$definition,array$autowired)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

$autowiredMethods?

$found =array();
$regexList =array();
foreach ($autowiredas$pattern) {
$regexList[] ='/^'.str_replace('\*','.*',preg_quote($pattern,'/')).'$/i';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Perhaps use.+, but it depends ifset* should matchset(). Not sure :)

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I would say thatset() is ok but I'm not sure too.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

* comes from glob patterns, and there,* =>/.*/, we shouldn't be the one redefining old semantics people are used to since years.

ro0NL, GuilhemN, and dunglas reacted with thumbs up emoji
*
* @return Definition The current instance
*/
publicfunctionsetAutowired($autowiredMethods)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

$autowired

*/
publicfunctionsetAutowired($autowiredMethods)
{
$this->autowiredMethods =$autowiredMethods ?array('__construct') :array();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I would callsetAutowiredMethods here..

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Why? This method is just a mutator, we can avoid an extra function call and potential unwanted side effects in the future.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Fair enough.

$autowireTags[] =$type->textContent;
}

if (!empty($autowireTags)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

if (0 !== count($autowiredTags)) {?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Even better:if ($autowiredTags) {
I'm fighting against the idea that using count() provides any type information. It does not.

ro0NL and dunglas reacted with thumbs up emoji
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

You're right.. got confused by! 😞

*
* @param string $id
* @param \ReflectionClass $reflectionClass
* @param string[] $autowired
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

$autowiredMethods?

{
if ($isConstructor =$reflectionMethod->isConstructor()) {
$arguments =$definition->getArguments();
}else {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

What about removing theelse?

$arguments =array();if ($isConstructor =$reflectionMethod->isConstructor()) {$arguments =$definition->getArguments();}

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

It adds an extra assignation. Not sure it worth it.

Copy link
Contributor

@GuilhemNGuilhemNNov 5, 2016•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

As you want, that's just a detail after all ☺

if (!$parameter->isOptional()) {
thrownewRuntimeException(sprintf('Unable to autowire argument index %d ($%s) for the service "%s". If this is an object, give it a type-hint. Otherwise, specify this argument\'s value explicitly.',$index,$parameter->name,$id));
if ($isConstructor) {
thrownewRuntimeException(sprintf('Unable to autowire argument index %d ($%s) for the service "%s". If this is an object, give it a type-hint. Otherwise, specify this argument\'s value explicitly.',$index,$parameter->name,$id));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

What about reversing theif condition here to make the exception message more readable?

if (!$isConstructor) {return;}// ...

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

But I prefer using non-negative condition when possible (and it's possible here).

GuilhemN reacted with thumbs up emoji
$arguments =array();
}

$addMethodCall =false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Could you add a small comment here? I don't think it's easy to understand what contains this variable without looking deeply the code.
Maybe// Whether the method should be added to the definition as a call.

At the same time, I don't remember if it's written somewhere in the code what's this pass behavior (e.g. only autowire methods when at least one if their arguments is not to its default value), it could be worth adding it if not.

$pass =newAutowirePass();
$pass->process($container);

$this->assertEquals(array('Symfony\Component\DependencyInjection\Compiler\AutowirePass: Autowiring\'s patterns "not", "exist*" for service "foo" don\'t match any method.'),$container->getCompiler()->getLog());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

AutowirePass::class?

$this->assertTrue($def->isAutowired());
$this->assertEquals(array('__construct'),$def->getAutowiredMethods());

$def->setAutowired(array('foo'));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

setAutowiredMethods?

$found =array();
$regexList =array();
foreach ($autowiredMethodsas$pattern) {
$regexList[] ='/^'.str_replace('\*','.*',preg_quote($pattern,'/')).'$/i';
Copy link
Contributor

@ro0NLro0NLNov 5, 2016•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Should be consider allowing some other basic glob patterns, ie.? => . Could be convenient in some cases...

https://en.wikipedia.org/wiki/Glob_(programming)#Syntax

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

No need IMHO

@dunglas
Copy link
MemberAuthor

@fabpot can I merge this one? It will help for#18193 and#20738.

ro0NL reacted with hooray emoji

@nicolas-grekas
Copy link
Member

👍 for merging asap, so that we can play with the feat asap and tweak it if needed in the coming months.

Simperfit reacted with thumbs up emoji

@nicolas-grekasnicolas-grekas modified the milestones:3.3,3.xDec 12, 2016
}else {
throw$e;
// The exception code is set to 1 if the exception must be thrown even if it's a setter
if (1 ===$e->getCode() ||$isConstructor) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Can we use a specific getter/setter instead of a magic 1 value for the code?

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Done (but I find the use of a specific code less intrusive than using accessors, here we add methods to a generic exception for an implementation detail of theAutowirePass).

*
* @return Definition The current instance
*/
publicfunctionsetAutowired($autowired)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

This one could be deprecated, right?

Copy link
MemberAuthor

@dunglasdunglasDec 13, 2016•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I propose to deprecate it in another PR (if it's worth it) because we need to determine how we deal with#20648

{
publicfunctionsetFoo(Foo$foo)
{
// should be called
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

What about throwing an exception here instead of the comment?

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

This setter should be called (and shouldn't throw anything).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

nevermind :)

@fabpot
Copy link
Member

Thank you@dunglas.

@fabpotfabpot merged commit6dd53c7 intosymfony:masterDec 13, 2016
fabpot added a commit that referenced this pull requestDec 13, 2016
…configurable (dunglas)This PR was merged into the 3.3-dev branch.Discussion----------[DependencyInjection] Make method (setter) autowiring configurable| Q | A || --- | --- || Branch? | master || Bug fix? | no || New feature? | yes || BC breaks? | no || Deprecations? | maybe? || Tests pass? | yes || Fixed tickets |#19631 || License | MIT || Doc PR |symfony/symfony-docs#7041 |Follow up of#19631. Implements#19631 (comment):Edit: the last supported format:``` yamlservices:    foo:        class: Foo        autowire: ['__construct', 'set*'] # Autowire constructor and all setters        autowire: true # Converted by loaders in `autowire: ['__construct']` for BC        autowire: ['foo', 'bar'] # Autowire only `foo` and `bar` methods```Outdated:``` yamlautowire: true # constructor autowiringautowire: [__construct, setFoo, setBar] # autowire whitelisted methods onlyautowire: '*' # autowire constructor + every setters (following existing rules for setters autowiring)```- [x] Allow to specify the list of methods in the XML loader- [x] Add tests for the YAML loaderCommits-------6dd53c7 [DependencyInjection] Introduce method injection for autowiring
@fabpotfabpot mentioned this pull requestMay 1, 2017
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@fabpotfabpotfabpot approved these changes

@nicolas-grekasnicolas-grekasnicolas-grekas left review comments

@chalasrchalasrchalasr left review comments

+3 more reviewers

@TobionTobionTobion left review comments

@ro0NLro0NLro0NL left review comments

@GuilhemNGuilhemNGuilhemN left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

3.3

Development

Successfully merging this pull request may close these issues.

9 participants

@dunglas@fabpot@Tobion@ro0NL@nicolas-grekas@GuilhemN@chalasr@javiereguiluz@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp