Skip to content

[DependencyInjection] Enable deprecating parameters #47719

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.

Sign up for GitHub

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

Jump to bottom
Closed
HeahDude wants to merge 1 commit into from
Closed

[DependencyInjection] Enable deprecating parameters #47719

HeahDude wants to merge 1 commit into from

Conversation

@HeahDude
Copy link
Contributor

@HeahDude HeahDude commented Sep 29, 2022 •
edited
Loading

Q A
Branch? 6.3
Bug fix? no
New feature? yes
Deprecations? no
Tickets ~
License MIT
Doc PR TODO

Following #47680, this PR allows deprecating parameters either to move to build parameters, or to rename them or just to be able to remove them in a BC way.

// with ContainerBuilder:
$container->setParameter('old_param', 'xxx');
$container->setParameter('new_param', 'yyy');
$container->deprecateParameter(
    'old_param',
    'symfony/xxx',
    '6.2',
    # Fourth arg is optional, default: 'The parameter "%s" is deprecated.'
    'The param "%s" is deprecated use "%new_param%" instead.'
);

TODO:

  • Waiting for @nicolas-grekas (and others) to challenge the implementation
  • Update CHANGELOG
  • Add tests

@carsonbot carsonbot added this to the 6.2 milestone Sep 29, 2022
@carsonbot carsonbot changed the title [DI] [POC] Enable deprecating parameters [DependencyInjection] [POC] Enable deprecating parameters Sep 29, 2022
stof
stof requested changes Sep 29, 2022
View reviewed changes
Copy link
Member

@stof stof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This misses the update of the ContainerBuilder merging to copy the deprecations.

@HeahDude HeahDude force-pushed the feat/deprecate-parameters branch 2 times, most recently from 750ef59 to 02f5339 Compare September 29, 2022 19:38
@HeahDude
Copy link
Contributor Author

HeahDude commented Sep 29, 2022

This misses the update of the ContainerBuilder merging to copy the deprecations.

Thanks for the heads-up :), fixed!

nicolas-grekas
nicolas-grekas reviewed Oct 19, 2022
View reviewed changes
@HeahDude HeahDude force-pushed the feat/deprecate-parameters branch from c662cd1 to 62455c9 Compare October 22, 2022 10:30
@HeahDude HeahDude changed the title [DependencyInjection] [POC] Enable deprecating parameters [DependencyInjection] Enable deprecating parameters Oct 22, 2022
@HeahDude
Copy link
Contributor Author

HeahDude commented Oct 22, 2022

The new implementation does not trigger deprecations in the API itself, so the label can be removed.
Thanks for the reviews.

@fabpot fabpot modified the milestones: 6.2, 6.3 Nov 1, 2022
@HeahDude HeahDude force-pushed the feat/deprecate-parameters branch from fcb85cd to a22b22c Compare December 5, 2022 11:52
@HeahDude HeahDude requested a review from stof December 5, 2022 11:52
@HeahDude HeahDude force-pushed the feat/deprecate-parameters branch from 8172f78 to e47d070 Compare December 6, 2022 08:02
@HeahDude HeahDude requested review from nicolas-grekas and stof and removed request for nicolas-grekas and stof December 6, 2022 13:01
@nicolas-grekas nicolas-grekas force-pushed the feat/deprecate-parameters branch from 8a2634a to d3ee161 Compare December 12, 2022 14:28
nicolas-grekas
nicolas-grekas approved these changes Dec 12, 2022
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just squashed+added a commit with some tweaks.
LGTM thanks!

@HeahDude
Copy link
Contributor Author

HeahDude commented Dec 17, 2022

While trying the feature I realized that the FrozenParameterBag fails to trigger a deprecation due to property visibility in parent::get().
I've added a test that covers this and fixed it by using protected, the property remains immutable anyway thanks to the exception in deprecate() (now covered as well).

@nicolas-grekas nicolas-grekas force-pushed the feat/deprecate-parameters branch from 63d39b0 to bb6432b Compare December 17, 2022 17:56
@HeahDude HeahDude force-pushed the feat/deprecate-parameters branch from bb6432b to c806be8 Compare January 7, 2023 08:47
@HeahDude HeahDude requested a review from stof January 7, 2023 08:49
nicolas-grekas
nicolas-grekas reviewed Jan 9, 2023
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still 👍 on my side after the last changes
/cc @symfony/mergers any other comments?

@chalasr
Copy link
Member

chalasr commented Jan 11, 2023

Thank you @HeahDude.

@chalasr chalasr force-pushed the feat/deprecate-parameters branch from c806be8 to 0f0a2fb Compare January 11, 2023 14:39
@chalasr
Copy link
Member

chalasr commented Jan 11, 2023

gh went wrong due to some race condition, merged in be5fbce

@chalasr chalasr closed this Jan 11, 2023
@HeahDude HeahDude deleted the feat/deprecate-parameters branch January 11, 2023 14:43
@OskarStark
Copy link
Contributor

OskarStark commented Jan 11, 2023

Looks like there was no doc issue opened 🤔

@HeahDude HeahDude mentioned this pull request Jan 12, 2023
Closed
@HeahDude HeahDude mentioned this pull request Jul 18, 2023
Merged
fabpot added a commit that referenced this pull request Oct 11, 2023
@fabpot
feature #51011 [FrameworkBundle] Add parameters deprecations to the o…
4e16f7b 
…utput of `debug:container` command (HeahDude)

This PR was squashed before being merged into the 6.4 branch.

Discussion
----------

[FrameworkBundle] Add parameters deprecations to the output of `debug:container` command

| Q             | A
| ------------- | ---
| Branch?       | 6.4
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Tickets       | ~
| License       | MIT
| Doc PR        | ~

Since #47719 parameters can be deprecated but one needs to read the deprecation logs carefully.
It would be convenient to have the info when dumping them with debug commands.

Here's a glimpse of text format (the fixtures in tests can do the rest):

<img width="1126" alt="Screenshot 2023-07-18 at 12 50 49 PM" src="https://github.com/symfony/symfony/assets/10107633/6a2ea20b-be3c-4428-bb5d-aa97f3b38803">

I don't know if we really want to support all formats since it may break BC somehow if parsers are used to read the output.
I still tried to adapt them all in this PR for consistency.
But JSON required an object to display both the value and the deprecation, another way could be to add a specific entry for one or all deprecations.

Commits
-------

7963e9d [FrameworkBundle] Add parameters deprecations to the output of `debug:container` command
@franmomu franmomu mentioned this pull request Dec 14, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@chalasr chalasr chalasr left review comments

@stof stof Awaiting requested review from stof

Assignees

No one assigned

Labels

DependencyInjection Deprecation Feature Status: Needs Review

Projects

None yet

Milestone

6.3

Development

Successfully merging this pull request may close these issues.

7 participants

@HeahDude @chalasr @OskarStark @nicolas-grekas @stof @fabpot @carsonbot