Skip to content

[Console] Command simplification and deprecations #59564

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
wants to merge 1 commit into from
Closed

[Console] Command simplification and deprecations #59564

wants to merge 1 commit into from

Conversation

yceruto
Copy link
Member

@yceruto yceruto commented Jan 20, 2025 •
edited
Loading

Q A
Branch? 7.3
Bug fix? no
New feature? yes
Deprecations? yes
Issues -
License MIT

2 goals in this PR, one feature and one deprecation.

The feature is about delaying the command initialization, making the parent::__construct() call unnecessary in regular/new-invokable commands extending from Command class.

Example:

#[AsCommand(name: 'foo')]
class FooCommand extends Command
{
    public function __construct(private BarInterface $bar)
    {
        parent::__construct(); // <-- unnecessary and can be removed from now on.
    }
}

Additionally, the static methods Command::getDefaultName() and Command::getDefaultDescription() are deprecated (see discussion #59473 (comment)). These methods are now obsolete because AsCommand::name and AsCommand::description are configured directly in the service tag. These methods were mainly intended for framework/DI purposes, making it clearer for end users seeking to set the command name.

@yceruto yceruto requested a review from chalasr as a code owner January 20, 2025 16:20
@carsonbot carsonbot added this to the 7.3 milestone Jan 20, 2025
@yceruto yceruto requested a review from kbond January 20, 2025 16:21
@stof
Copy link
Member

stof commented Jan 20, 2025

Consequently, the constructor method can be removed in Symfony 8.0.

this is not true. Removing the parent constructor would require first deprecating calling it (even without a name), so that the code is ready for the removal. And making the parent constructor call mandatory in 6.4 LTS and deprecated in 7.3 would make it harder for the ecosystem to support both versions at once (which might encourage them to drop support for our LTS version before its EOL, which is bad for our communication about the LTS).
The cost of keeping an empty constructor method is null here, so we should give more time to the ecosystem (deprecating calling it in 8.1 for removal in 9.0, allowing to support 7.4 LTS and 8.x with the same code not calling it).

However, I'm not sure encouraging to not call the parent constructor is the right solution.

@yceruto yceruto force-pushed the command_deprecations branch 3 times, most recently from 61c3d4a to a7024e6 Compare January 20, 2025 16:42
@yceruto yceruto requested a review from welcoMattic as a code owner January 20, 2025 16:42
GromNaN
GromNaN reviewed Jan 20, 2025
View reviewed changes
Copy link
Member

@GromNaN GromNaN 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.

Removing the $name parameter from the constructor seems to be an unnecessary breaking change.

@yceruto
Copy link
Member Author

yceruto commented Jan 20, 2025

Damn! @stof you're right, I removed that sentence from the PR description. It's hard to deprecate since there is no way to know if the constructor was either called or there is no construct at all in the subclass. Let's revert that part, thanks!

@yceruto yceruto force-pushed the command_deprecations branch from a7024e6 to f61c319 Compare January 20, 2025 17:24
@yceruto
Copy link
Member Author

yceruto commented Jan 20, 2025

Update! removed deprecation about passing the command name through the constructor. However, we can update the documentation to remove the parent::__construct() call from all commands.

@yceruto
Copy link
Member Author

yceruto commented Jan 20, 2025

I'll split this PR into two, so we can discuss both topics separately

@ondrejmirtes

This comment was marked as off-topic.

Sign in to view
@GromNaN

This comment was marked as off-topic.

Sign in to view
@GromNaN GromNaN mentioned this pull request Jan 20, 2025
Closed
@yceruto
Copy link
Member Author

yceruto commented Jan 20, 2025

moving the deprecation to a separate PR: #59565, and closing the other proposal for now, as it doesn't seem worthwhile at this point

@yceruto yceruto closed this Jan 20, 2025
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 Awaiting requested review from chalasr chalasr is a code owner

@kbond kbond Awaiting requested review from kbond

@welcoMattic welcoMattic Awaiting requested review from welcoMattic

Assignees
No one assigned
Labels
Console Deprecation Feature Status: Needs Review
Projects
None yet
Milestone
7.3
Development

Successfully merging this pull request may close these issues.
5 participants
@yceruto @stof @ondrejmirtes @GromNaN @carsonbot