Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DI] Added support for deprecating aliases #29968

Merged
merged 2 commits into from Jan 25, 2019

Conversation

Projects
None yet
7 participants
@renanbr
Copy link

commented Jan 23, 2019

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 TBD

This PR is a continuity of #24707

@nicolas-grekas nicolas-grekas added this to the next milestone Jan 23, 2019

@stof
Copy link
Member

left a comment

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)

throw new InvalidArgumentException(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;

This comment has been minimized.

Copy link
@stof

stof Jan 23, 2019

Member

this continue is dead code

* Whether this alias is deprecated, that means it should not be called
* anymore.
*
* @param bool $status Defaults to true

This comment has been minimized.

Copy link
@OskarStark

OskarStark Jan 24, 2019

Contributor

the description should describe what it does

@nicolas-grekas
Copy link
Member

left a comment

I think we're good thank you!
Here are some minor comments.
Please squash all your commits also keep only two.

@@ -78,6 +85,58 @@ public function isPrivate()
return $this->private;
}
/**
* Whether this alias is deprecated, that means it should not be called

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jan 24, 2019

Member

s/called/referenced

/**
* Returns whether this alias is deprecated.
*
* @return bool

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jan 24, 2019

Member

the whole docblock can be removed and replaced by a real return type

*
* @return string
*/
public function getDeprecationMessage($id)

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jan 24, 2019

Member

should use real argument+return type + remove docblock

@@ -667,8 +671,12 @@ private function validateAlias(\DOMElement $alias, $file)
}
}
$allowedTags = ['deprecated'];

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jan 24, 2019

Member

no need for the temporary variable, isn't it? better remove it.

{
$container = new ContainerBuilder();
$container->register('foo', '\stdClass');

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jan 24, 2019

Member

no need for the \ (same below)

$aliasDeprecated = new Alias('foo');
$aliasDeprecated->setDeprecated(true);
$container->setAlias('foo_aliased', $aliasDeprecated);

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jan 24, 2019

Member

maybe name it deprecated_foo_alias instead of foo_aliased?

$dumper = new PhpDumper($container);
$this->assertStringEqualsFile(self::$fixturesPath.'/php/container_alias_deprecation.php', $dumper->dump());
eval('?>'.$dumper->dump(['class' => 'Symfony_DI_PhpDumper_Test_Aliases_Deprecation']));

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jan 24, 2019

Member

better dump only once (see previous line)

/**
* Gets the public 'alias_for_foo_deprecated' alias.
*
* @return object An instance returned by "getFooService()"

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jan 24, 2019

Member

Maybe this message should be The "foo" service.?

@nicolas-grekas nicolas-grekas changed the title Added support for deprecating aliases [DI] Added support for deprecating aliases Jan 24, 2019

@renanbr renanbr force-pushed the renanbr:deprecate-aliases branch from 0eabd0b to 6711e59 Jan 24, 2019

@renanbr

This comment has been minimized.

Copy link
Author

commented Jan 24, 2019

changes applied, commits squashed
@stof, @OskarStark, @nicolas-grekas

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Jan 24, 2019

Fabbot complains :)

@renanbr renanbr force-pushed the renanbr:deprecate-aliases branch from 6711e59 to 6c571ad Jan 25, 2019

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Jan 25, 2019

Thank you @renanbr.

@nicolas-grekas nicolas-grekas merged commit 6c571ad into symfony:master Jan 25, 2019

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

nicolas-grekas added a commit that referenced this pull request Jan 25, 2019

feature #29968 [DI] Added support for deprecating aliases (j92, Renan)
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        | TBD

This PR is a continuity of #24707

Commits
-------

6c571ad Added support for deprecating aliases (runtime+dumper)
0eb071b Added support for deprecating an alias
@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Jan 25, 2019

And thank you @j92 !

private $deprecated;
private $deprecationTemplate;
private static $defaultDeprecationTemplate = 'The "%service_id%" service alias is deprecated. You should stop using it, as it will soon be removed.';

This comment has been minimized.

Copy link
@javiereguiluz

javiereguiluz Jan 28, 2019

Member

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.

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jan 28, 2019

Member

updated in #29995, please review

nicolas-grekas added a commit that referenced this pull request Jan 28, 2019

bug #29995 [DI] add id of referencing service when a deprecated alias…
… 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

javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request Jan 29, 2019

minor #10937 Documented the deprecation of service aliases (javieregu…
…iluz)

This PR was squashed before being merged into the master branch (closes #10937).

Discussion
----------

Documented the deprecation of service aliases

It documents symfony/symfony#29968 but it contains the changes of the pending PR symfony/symfony#29995

Commits
-------

d5d87e6 Documented the deprecation of service aliases

XuruDragon pushed a commit to XuruDragon/symfony that referenced this pull request Feb 4, 2019

Anthony MARTIN
[DependencyInjection] Added information about deprecated aliases in d…
…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/a

Fix and improves a bit PR symfony#29968 and symfony#29995

XuruDragon pushed a commit to XuruDragon/symfony that referenced this pull request Feb 4, 2019

Anthony MARTIN
[DependencyInjection] Added information about deprecated aliases in d…
…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/a

Fix and improves a bit PR symfony#29968 and symfony#29995

XuruDragon pushed a commit to XuruDragon/symfony that referenced this pull request Feb 4, 2019

Anthony MARTIN
[DependencyInjection] Added information about deprecated aliases in d…
…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/a

Fix and improves a bit PR symfony#29968 and symfony#29995

XuruDragon pushed a commit to XuruDragon/symfony that referenced this pull request Feb 4, 2019

Anthony MARTIN
[DependencyInjection] Added information about deprecated aliases in d…
…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/a

Fix and improves a bit PR symfony#29968 and symfony#29995

XuruDragon pushed a commit to XuruDragon/symfony that referenced this pull request Feb 5, 2019

Anthony MARTIN
[DependencyInjection] Added information about deprecated aliases in d…
…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/a

Fix and improves a bit PR symfony#29968 and symfony#29995

XuruDragon pushed a commit to XuruDragon/symfony that referenced this pull request Feb 6, 2019

Anthony MARTIN
[DependencyInjection] Added information about deprecated aliases in d…
…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/a

Fix and improves a bit PR symfony#29968 and symfony#29995

XuruDragon pushed a commit to XuruDragon/symfony that referenced this pull request Feb 6, 2019

Anthony MARTIN
[DependencyInjection] Added information about deprecated aliases in d…
…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/a

Fix and improves a bit PR symfony#29968 and symfony#29995

XuruDragon pushed a commit to XuruDragon/symfony that referenced this pull request Feb 6, 2019

Anthony MARTIN
[DependencyInjection] Added information about deprecated aliases in d…
…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/a

Fix and improves a bit PR symfony#29968 and symfony#29995

XuruDragon pushed a commit to XuruDragon/symfony that referenced this pull request Feb 6, 2019

Anthony MARTIN
[DependencyInjection] Added information about deprecated aliases in d…
…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/a

Fix and improves a bit PR symfony#29968 and symfony#29995

XuruDragon pushed a commit to XuruDragon/symfony that referenced this pull request Feb 7, 2019

Anthony MARTIN
[DependencyInjection] Added information about deprecated aliases in d…
…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/a

Fix and improves a bit PR symfony#29968 and symfony#29995

nicolas-grekas added a commit that referenced this pull request Feb 8, 2019

feature #30075 [DependencyInjection] Added information about deprecat…
…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/a

Fix and improves a bit PR #29968 and #29995

![Screenshot](https://i.imgur.com/GDj2NIY.png)

Commits
-------

3d2378d [DependencyInjection] Added information about deprecated aliases in debug:autowiring

@nicolas-grekas nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019

@fabpot fabpot referenced this pull request May 9, 2019

Merged

Release v4.3.0-BETA1 #31435

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.