[DependencyInjection] Yaml: check if $tags is an array before using it #21348
Conversation
dunglas
commented
Jan 19, 2017
| Q | A |
|---|---|
| Branch? | master |
| Bug fix? | yes |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | n/a |
| License | MIT |
| Doc PR | n/a |
| @@ -331,7 +331,15 @@ private function parseDefinition($id, $service, $file, array $defaults) | |||
| } | |||
| } | |||
|
|
|||
| $tags = isset($service['tags']) ? $service['tags'] : array(); | |||
There was a problem hiding this comment.
I think this line was fine, just add if (!is_array($tags)) { on the next line
There was a problem hiding this comment.
shouldn't this PR be applied on a lower branch btw?
There was a problem hiding this comment.
Why not but it adds 1 useless check when $service['tags'] is not set.
There was a problem hiding this comment.
No the branch is correct (this "bug" was introduced by _defaults in #21071).
There was a problem hiding this comment.
it adds 1 useless check
yes, but we don't care, readibility is more important here (thinking about the complexity of nested if/else blocks) IMHO
There was a problem hiding this comment.
I usually prefer if/else than the ternary operator but it's probably because I need glasses to read on a screen.
I'll change that.
|
We should add a test then to avoid future regressions. |
|
@xabbuh test added |
|
Tests fail :) |
|
Fixed. |
|
|
||
| Foo\Bar: | ||
| tags: invalid | ||
| inherit_tags: true |
There was a problem hiding this comment.
I find this test confusing. Intuitively, I thought that tags were inherited and then validated, so the Foo\Bar service has the valid tags: ['foo'] tags instead of tags: invalid
There was a problem hiding this comment.
But this way (using a string instead of an array) to specify tags already wasn't supported before. Why should we do that now?
|
Thank you @dunglas. |
…efore using it (dunglas) This PR was squashed before being merged into the 3.3-dev branch (closes #21348). Discussion ---------- [DependencyInjection] Yaml: check if $tags is an array before using it | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | n/a Commits ------- 1c9b5c9 [DependencyInjection] Yaml: check if is an array before using it