[DI] Fixes aliases visibility with and without defaults #21219
Conversation
ogizanagi
commented
Jan 9, 2017
| Q | A |
|---|---|
| Branch? | master |
| Bug fix? | yes |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | #21071 (comment) |
| License | MIT |
| Doc PR | N/A |
| @@ -148,9 +148,13 @@ public function testLoadServices() | |||
| $this->assertTrue(isset($aliases['alias_for_foo']), '->load() parses aliases'); | |||
| $this->assertEquals('foo', (string) $aliases['alias_for_foo'], '->load() parses aliases'); | |||
| $this->assertTrue($aliases['alias_for_foo']->isPublic()); | |||
| $this->assertTrue($aliases['another_third_alias_for_foo']->isPublic()); | |||
| @@ -194,7 +194,7 @@ private function parseDefinitions($content, $file) | |||
| } | |||
| } | |||
| } else { | |||
| $defaults = array(); | |||
| $defaults = array('public' => true); | |||
There was a problem hiding this comment.
Case above that triggers a deprecation?
There was a problem hiding this comment.
Actually, would you prefer using the same way as you did for classic definitions, i.e:
$public = isset($service['public']) ? $service['public'] : (isset($defaults['public']) ? $defaults['public'] : true);rather than adding this in defaults?
To me it makes sense to define canonical defaults, but then I should probably update https://github.com/symfony/symfony/blob/master/src/Symfony/Component/DependencyInjection/Loader/YamlFileLoader.php#L274 too
There was a problem hiding this comment.
Previous fix was not enough in case _defaults: { public: true } was set anyway... So updated.
| @@ -34,3 +34,8 @@ services: | |||
| inherit_tags: true | |||
| tags: | |||
| - name: baz | |||
|
|
|||
| with_defaults_aliased: | |||
| alias: 'with_defaults' | |||
There was a problem hiding this comment.
I would remove single quotes here, in order to harmonize it with the documentation. What do you think?
There was a problem hiding this comment.
ah it actually doesn't matter :)
There was a problem hiding this comment.
That's only fixtures, so we don't care much IMHO. But ok. :)
| @@ -18,6 +18,8 @@ services: | |||
| calls: | |||
| - [ setBar, [ foo, '@foo', [true, false] ] ] | |||
| alias_for_foo: '@foo' | |||
| another_third_alias_for_foo: | |||
There was a problem hiding this comment.
This alias is before another_alias_for_foo because of an issue with the YamlDumper. See #21220
This PR was merged into the 2.7 branch. Discussion ---------- [DI] Fix missing new line after private alias | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #21219 (comment) | License | MIT | Doc PR | N/A Without this change, the output is: ```yml # [...] alias_for_foo: '@foo' another_alias_for_foo: alias: foo public: false another_third_alias_for_foo: '@foo' # <--- missing new line after `public: false` ``` (this is tested by the `CrossCheckTest` but there is no fixture file to update (automatically dumped and removed by the test case)) Commits ------- 101a165 [DI] Fix missing new line after private alias
|
Thank you @ogizanagi. |
…gizanagi) This PR was merged into the 3.3-dev branch. Discussion ---------- [DI] Fixes aliases visibility with and without defaults | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #21071 (comment) | License | MIT | Doc PR | N/A Commits ------- f1cc090 [DI] Fixes aliases visibility with and without defaults