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] Added support for deprecating aliases#29968
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
stof left a comment
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 also seems to miss warning (at compile-time) when a service is referencing a deprecated alias (before resolving them, otherwise we won't have any such reference)
| if (!\in_array($key, ['alias','public'])) { | ||
| if (!\in_array($key, ['alias','public','deprecated'])) { | ||
| thrownewInvalidArgumentException(sprintf('The configuration key "%s" is unsupported for the service "%s" which is defined as an alias in "%s". Allowed configuration keys for service aliases are "alias" and "public".',$key,$id,$file)); | ||
| continue; |
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.
thiscontinue is dead code
| * Whether this alias is deprecated, that means it should not be called | ||
| * anymore. | ||
| * | ||
| * @param bool $status Defaults to 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.
the description should describe what it does
nicolas-grekas left a comment
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 think we're good thank you!
Here are some minor comments.
Please squash all your commits also keep only two.
| } | ||
| /** | ||
| * Whether this alias is deprecated, that means it should not be called |
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.
s/called/referenced
| /** | ||
| * Returns whether this alias is deprecated. | ||
| * | ||
| * @return bool |
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.
the whole docblock can be removed and replaced by a real return type
| * | ||
| * @return string | ||
| */ | ||
| publicfunctiongetDeprecationMessage($id) |
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.
should use real argument+return type + remove docblock
| } | ||
| } | ||
| $allowedTags = ['deprecated']; |
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.
no need for the temporary variable, isn't it? better remove it.
| { | ||
| $container =newContainerBuilder(); | ||
| $container->register('foo','\stdClass'); |
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.
no need for the\ (same below)
| $aliasDeprecated =newAlias('foo'); | ||
| $aliasDeprecated->setDeprecated(true); | ||
| $container->setAlias('foo_aliased',$aliasDeprecated); |
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.
maybe name it deprecated_foo_alias instead of foo_aliased?
| $dumper =newPhpDumper($container); | ||
| $this->assertStringEqualsFile(self::$fixturesPath.'/php/container_alias_deprecation.php',$dumper->dump()); | ||
| eval('?>'.$dumper->dump(['class' =>'Symfony_DI_PhpDumper_Test_Aliases_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.
better dump only once (see previous line)
| /** | ||
| * Gets the public 'alias_for_foo_deprecated' alias. | ||
| * | ||
| * @return object An instance returned by "getFooService()" |
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.
Maybe this message should beThe "foo" service.?
renanbr commentedJan 24, 2019
changes applied, commits squashed |
nicolas-grekas commentedJan 24, 2019
Fabbot complains :) |
nicolas-grekas commentedJan 25, 2019
Thank you@renanbr. |
This PR was merged into the 4.3-dev branch.Discussion----------[DI] Added support for deprecating aliases| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? |no| Tests pass? | yes| Fixed tickets |#24507| License | MIT| Doc PR | TBDThis PR is a continuity of#24707Commits-------6c571ad Added support for deprecating aliases (runtime+dumper)0eb071b Added support for deprecating an alias
nicolas-grekas commentedJan 25, 2019
And thank you@j92 ! |
| private$deprecated; | ||
| private$deprecationTemplate; | ||
| privatestatic$defaultDeprecationTemplate ='The "%service_id%" service alias is deprecated. You should stop using it, as it will soon be removed.'; |
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 know this PR is merged, but I've just reviewed its code and I wondered if the%service_id% placeholder name is confusing to other people too. To me, this placeholder would be easier to understand if it was called%alias_id%.
Also, this ->... as it will soon be removed. could be ->... as it will be removed in the future.
Thanks.
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.
updated in#29995, please review
… is found (nicolas-grekas)This PR was merged into the 4.3-dev branch.Discussion----------[DI] add id of referencing service when a deprecated alias is found| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | -| License | MIT| Doc PR | -Improves#29968 a bit for DX.Commits-------b124fb7 [DI] add id of referencing service when a deprecated alias is found
…iluz)This PR was squashed before being merged into the master branch (closes#10937).Discussion----------Documented the deprecation of service aliasesIt documentssymfony/symfony#29968 but it contains the changes of the pending PRsymfony/symfony#29995Commits-------d5d87e6 Documented the deprecation of service aliases
…ebug:autowiring| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | none| License | MIT| Doc PR | n/aFix and improves a bit PRsymfony#29968 andsymfony#29995
…ebug:autowiring| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | none| License | MIT| Doc PR | n/aFix and improves a bit PRsymfony#29968 andsymfony#29995
…ebug:autowiring| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | none| License | MIT| Doc PR | n/aFix and improves a bit PRsymfony#29968 andsymfony#29995
…ebug:autowiring| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | none| License | MIT| Doc PR | n/aFix and improves a bit PRsymfony#29968 andsymfony#29995
…ebug:autowiring| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | none| License | MIT| Doc PR | n/aFix and improves a bit PRsymfony#29968 andsymfony#29995
…ebug:autowiring| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | none| License | MIT| Doc PR | n/aFix and improves a bit PRsymfony#29968 andsymfony#29995
…ebug:autowiring| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | none| License | MIT| Doc PR | n/aFix and improves a bit PRsymfony#29968 andsymfony#29995
…ebug:autowiring| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | none| License | MIT| Doc PR | n/aFix and improves a bit PRsymfony#29968 andsymfony#29995
…ebug:autowiring| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | none| License | MIT| Doc PR | n/aFix and improves a bit PRsymfony#29968 andsymfony#29995
…ebug:autowiring| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | none| License | MIT| Doc PR | n/aFix and improves a bit PRsymfony#29968 andsymfony#29995
…ed aliases in debug:autowiring (XuruDragon)This PR was merged into the 4.3-dev branch.Discussion----------[DependencyInjection] Added information about deprecated aliases in debug:autowiring| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | none| License | MIT| Doc PR | n/aFix and improves a bit PR#29968 and#29995Commits-------3d2378d [DependencyInjection] Added information about deprecated aliases in debug:autowiring
This PR is a continuity of#24707