Skip to content

[FrameworkBundle] Always display service arguments & deprecate --show-arguments option for debug:container #59225

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
Merged
nicolas-grekas merged 1 commit into from
Jan 7, 2025
Merged

[FrameworkBundle] Always display service arguments & deprecate --show-arguments option for debug:container #59225

nicolas-grekas merged 1 commit into from
Jan 7, 2025

Conversation

@Florian-Merle
Copy link
Contributor

@Florian-Merle Florian-Merle commented Dec 16, 2024 •
edited
Loading

Q A
Branch? 7.3
Bug fix? no
New feature? no
Deprecations? yes
Issues Fix #59187
License MIT

This PR does the following:

  • Always shows service arguments
  • Deprecates --show-arguments option when a service name is provided and makes it a no-op

The next step will be to remove --show-arguments in 8.0.

@Florian-Merle Florian-Merle force-pushed the debug-container-deprecate-not-passing-show-arguments branch from 0c415df to 50ffbae Compare December 16, 2024 14:29
@carsonbot carsonbot added this to the 7.3 milestone Dec 16, 2024
@alexislefebvre
Copy link
Contributor

alexislefebvre commented Dec 16, 2024

Deprecate not passing the --show-arguments option when a service name is provided. The next step will be to deprecate --show-arguments in 8.0.

So with Symfony 7.3 and 7.4, I would see a deprecation if I don’t use --show-arguments, that would encourage me to use this option.

Then in Symfony 8.0, using the --show-arguments option would show a deprecation message?

That sounds counter-intuitive.

@OskarStark OskarStark changed the title [FrameworkBundle] debug:container deprecate not passing --show-arguments [FrameworkBundle] deprecate not passing --show-arguments for debug:container command Dec 16, 2024
GromNaN
GromNaN reviewed Dec 16, 2024
View reviewed changes
Copy link
Member

@GromNaN GromNaN left a comment

Choose a reason for hiding this comment

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

Then in Symfony 8.0, using the --show-arguments option would show a deprecation message?

I agree that it's not a great DX.
We can add the option --hide-arguments. In 8.0, change the default behavior but keep both options.

@chalasr
Copy link
Member

chalasr commented Dec 17, 2024

Or we could add --hide-arguments now in 7.3 and deprecate not setting one or the other. But do we really want to have that hide-arguments ultimately? Not sure we need it.

Asking to opt-in the behavior we want to be the only possible one, before making it the default one and removing the capacity to opt-in/out is what we usually do I think.
So apart from the fact that the option should be deprecated in 8.1 instead of 8.0, which makes a difference, the proposed plan looks good to me.

@fabpot
Copy link
Member

fabpot commented Dec 18, 2024

I would keep things simple here:

  • Always show arguments
  • Deprecate --show-arguments and make it a noop in 7.3
  • Remove --show-arguments in 8.0

@GromNaN
Copy link
Member

GromNaN commented Dec 18, 2024

Indeed, command output is not covered by our BC promise. If someone parses the output, they should opt for --format=json.

@chalasr
Copy link
Member

chalasr commented Dec 18, 2024

The json output will be impacted by this change but that’s fine for me 🙌

@fabpot
Copy link
Member

fabpot commented Dec 18, 2024

The json output will be impacted by this change but that’s fine for me 🙌

Not really, the only difference is that we'll always have the arguments.

@chalasr
Copy link
Member

chalasr commented Dec 18, 2024

Yes that’s what I mean, the output will now have an arguments key by default so a 1:1 assertion won’t pass anymore.

@Florian-Merle Florian-Merle force-pushed the debug-container-deprecate-not-passing-show-arguments branch from 50ffbae to c090137 Compare December 18, 2024 13:49
@Florian-Merle
Copy link
Contributor Author

Florian-Merle commented Dec 18, 2024

I updated the code to always show arguments.
Some unit tests are failing but I believe it's not because of my changes.

GromNaN
GromNaN reviewed Dec 18, 2024
View reviewed changes
Copy link
Member

@GromNaN GromNaN left a comment

Choose a reason for hiding this comment

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

I'm happy to see the code simplified.

@chalasr
Copy link
Member

chalasr commented Dec 18, 2024

Please also add a note in the framework-bundle's CHANGELOG and UPGRADE-7.3 about the deprecation.

@vudaltsov

This comment was marked as off-topic.

Sign in to view
@vudaltsov

This comment was marked as off-topic.

Sign in to view
@chalasr

This comment was marked as off-topic.

Sign in to view
@vudaltsov

This comment was marked as off-topic.

Sign in to view
@Florian-Merle

This comment was marked as off-topic.

Sign in to view
@Florian-Merle Florian-Merle force-pushed the debug-container-deprecate-not-passing-show-arguments branch from c090137 to 18d47f8 Compare December 19, 2024 12:59
@vudaltsov
Copy link
Contributor

vudaltsov commented Dec 20, 2024

@Florian-Merle, thank you!

Good luck with this PR, looking forward to better DX with debug:container in 7.3!

chalasr
chalasr requested changes Dec 20, 2024
View reviewed changes
chalasr
chalasr requested changes Dec 20, 2024
View reviewed changes
@Florian-Merle Florian-Merle force-pushed the debug-container-deprecate-not-passing-show-arguments branch from 18d47f8 to 377ec8f Compare December 20, 2024 16:23
@chalasr
Copy link
Member

chalasr commented Dec 21, 2024

The PR title doesn't match anymore with what the PR does.

@Florian-Merle Florian-Merle changed the title [FrameworkBundle] deprecate not passing --show-arguments for debug:container command [FrameworkBundle] Always Display Service Arguments & Deprecate --show-arguments Option for debug:container Dec 23, 2024
@Florian-Merle Florian-Merle changed the title [FrameworkBundle] Always Display Service Arguments & Deprecate --show-arguments Option for debug:container [FrameworkBundle] Always Display Service Arguments & Deprecate --show-arguments Option for debug:container Dec 23, 2024
@nicolas-grekas nicolas-grekas changed the title [FrameworkBundle] Always Display Service Arguments & Deprecate --show-arguments Option for debug:container [FrameworkBundle] Always display service arguments & deprecate --show-arguments option for debug:container Jan 7, 2025
@nicolas-grekas nicolas-grekas force-pushed the debug-container-deprecate-not-passing-show-arguments branch from 377ec8f to 2641778 Compare January 7, 2025 14:20
@nicolas-grekas
Copy link
Member

nicolas-grekas commented Jan 7, 2025

Thank you @Florian-Merle.

@nicolas-grekas nicolas-grekas merged commit d240a6d into symfony:7.3 Jan 7, 2025
5 of 10 checks passed
@nicolas-grekas nicolas-grekas mentioned this pull request Jan 7, 2025
Closed
@fabpot fabpot mentioned this pull request May 2, 2025
Merged
@PierreCapel PierreCapel mentioned this pull request Sep 29, 2025
Merged
nicolas-grekas added a commit that referenced this pull request Sep 29, 2025
@nicolas-grekas
minor #61888 [FrameworkBundle] remove deprecated --show-arguments fro…
1c3f572 
…m debug:container command (PierreCapel)

This PR was merged into the 8.0 branch.

Discussion
----------

[FrameworkBundle] remove deprecated --show-arguments from debug:container command

| Q             | A
| ------------- | ---
| Branch?       | 8.0
| Bug fix?      | no
| New feature?  | no
| Deprecations? | yes
| License       | MIT

Removed at 8.0 per this discussion: #59225 (comment)

Commits
-------

1d4f37e remove deprecated --show-arguments from debug:container command
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@GromNaN GromNaN GromNaN left review comments

@chalasr chalasr chalasr requested changes

Assignees

No one assigned

Labels

Deprecation FrameworkBundle Status: Needs Work

Projects

None yet

Milestone

7.3

Development

Successfully merging this pull request may close these issues.

[FrameworkBundle] Enable --show-arguments in debug:container by default

8 participants

@Florian-Merle @alexislefebvre @chalasr @fabpot @GromNaN @vudaltsov @nicolas-grekas @carsonbot