Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[DI] Fixes aliases visibility with and without defaults#21219
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
ogizanagi commentedJan 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 |
| $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()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Duplicate below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Oops. Thanks.
| } | ||
| }else { | ||
| $defaults =array(); | ||
| $defaults =array('public' =>true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Case above that triggers a deprecation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 updatehttps://github.com/symfony/symfony/blob/master/src/Symfony/Component/DependencyInjection/Loader/YamlFileLoader.php#L274 too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Previous fix was not enough in case_defaults: { public: true } was set anyway... So updated.
| -name:baz | ||
| with_defaults_aliased: | ||
| alias:'with_defaults' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I would remove single quotes here, in order to harmonize it with thedocumentation. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
ah it actually doesn't matter :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
That's only fixtures, so we don't care much IMHO. But ok. :)
| calls: | ||
| -[ setBar, [ foo, '@foo', [true, false] ] ] | ||
| alias_for_foo:'@foo' | ||
| another_third_alias_for_foo: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This alias is beforeanother_alias_for_foo because of an issue with theYamlDumper. 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/AWithout 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
fabpot commentedJan 9, 2017
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/ACommits-------f1cc090 [DI] Fixes aliases visibility with and without defaults