[DependencyInjection] Properly ignore invalid reference arguments in collection arguments #17132
Conversation
|
|
||
| $calls = $def->getMethodCalls(); | ||
| $this->assertCount(1, $def->getMethodCalls()); | ||
| $this->assertSame(array(1 => $baz, 2 => null), $calls[0][1][0]); |
There was a problem hiding this comment.
I'm wondering, should we preserve numerically indexed arrays (i.e. expect 0,1 as keys instead of 1,2)?
There was a problem hiding this comment.
Yes, I think that's what you would expect when you get a list of things injected as an argument imo.
There was a problem hiding this comment.
I don't have a strong opinion on this actually. I chose to preserve them as it avoids an extra logic for numerically indexed arrays and thought it won't be harmful.
Do we have a similar behavior somewhere else that can help us to choose, or do you think it might be harmful in any way ? I'm not sure it is a good idea to rely on those indexes for anything.
There was a problem hiding this comment.
It's quite common to expect a numerically array and iterate over it using e.g. for ($i = 0; $i < $len; ++$i) {...}
There was a problem hiding this comment.
Ok. I've updated the PR to always have sequential numeric keys for numerically indexed arrays.
Let me know.
…collection arguments
|
👍 |
|
We miss a test for the case where a collection item is removed. |
|
@xabbuh : What do you mean ? In https://github.com/symfony/symfony/pull/17132/files#diff-d60628dabdfe82babfcdd500cbe16712R48, |
|
Hm, you're right. I thought it was 👍 |
|
Thank you @ogizanagi. |
…e arguments in collection arguments (ogizanagi)
This PR was merged into the 3.1-dev branch.
Discussion
----------
[DependencyInjection] Properly ignore invalid reference arguments in collection arguments
| Q | A
| ------------- | ---
| Bug fix? | no
| New feature? | yes
| BC breaks? | no
| Deprecations? | no
| Tests pass? | yes
| Fixed tickets | -
| License | MIT
| Doc PR | -
With this new feature, the following configuration:
```xml
<service id="baz" class="Baz"/>
<service id="foo" class="Foo">
<argument type="collection">
<argument type="service" id="bar" on-invalid="ignore" />
<argument type="service" id="baz" />
<argument type="service" id="moo" on-invalid="null" />
</argument>
<argument type="service" id="bar" on-invalid="ignore" />
<argument type="service" id="baz" />
<argument type="service" id="moo" on-invalid="null" />
<call method="foo">
<argument type="service" id="bar" on-invalid="ignore" />
</call>
<call method="fooCollection">
<argument type="collection">
<argument type="service" id="bar" on-invalid="ignore" />
<argument type="service" id="baz" />
<argument type="service" id="moo" on-invalid="null" />
</argument>
</call>
</service>
```
will result into the following `Definition`:
```php
Definition {#64 ▼
-class: "Foo"
// […]
-calls: array:1 [▼
0 => array:2 [▼
0 => "fooCollection"
1 => array:1 [▼
0 => array:2 [▼
0 => Reference {#59 ▼
-id: "baz"
-invalidBehavior: 1
}
1 => null
]
]
]
]
// […]
#arguments: array:4 [▼
0 => array:2 [▼
0 => Reference {#63 ▼
-id: "baz"
-invalidBehavior: 1
}
1 => null
]
1 => null
2 => Reference {#56 ▼
-id: "baz"
-invalidBehavior: 1
}
3 => null
]
}
```
Invalid references are ignored instead of replaced by `null` when they are part of a collection argument.
If the collection argument is part of a method call and contains an invalid reference, the method call is kept, but the invalid reference removed.
The behavior stays the same as before for non-collection arguments.
:christmas_tree:
Commits
-------
cfc4879 [DependencyInjection] Properly ignore invalid reference arguments in collection arguments
With this new feature, the following configuration:
will result into the following
Definition:Definition {#64 ▼ -class: "Foo" // […] -calls: array:1 [▼ 0 => array:2 [▼ 0 => "fooCollection" 1 => array:1 [▼ 0 => array:2 [▼ 0 => Reference {#59 ▼ -id: "baz" -invalidBehavior: 1 } 1 => null ] ] ] ] // […] #arguments: array:4 [▼ 0 => array:2 [▼ 0 => Reference {#63 ▼ -id: "baz" -invalidBehavior: 1 } 1 => null ] 1 => null 2 => Reference {#56 ▼ -id: "baz" -invalidBehavior: 1 } 3 => null ] }Invalid references are ignored instead of replaced by
nullwhen they are part of a collection argument.If the collection argument is part of a method call and contains an invalid reference, the method call is kept, but the invalid reference removed.
The behavior stays the same as before for non-collection arguments.
🎄