[Console] Remove deprecations across the component #50613
Conversation
903ba6a to
171965f
Compare
171965f to
740e684
Compare
| "symfony/console": "^6.4|^7.0", | ||
| "symfony/dotenv": "^6.4|^7.0", | ||
| "symfony/finder": "^6.4", |
There was a problem hiding this comment.
This is necessary because composer/class-map-generator doesn't support Symfony ^7.0 yet. The highest dep for the Finder must be set to ^6.4 because of this for now.
There was a problem hiding this comment.
please send a PR to class-map-generator instead
There was a problem hiding this comment.
I was going to update when I saw you updated the file. Thank you! 🙂
1a9a9f0 to
a1854be
Compare
| @@ -82,8 +71,7 @@ protected function initialize(InputInterface $input, OutputInterface $output): v | |||
| protected function execute(InputInterface $input, OutputInterface $output): int | |||
| { | |||
| try { | |||
| // "symfony" must be kept for compat with the shell scripts generated by Symfony Console 5.4 - 6.1 | |||
| $version = $input->getOption('symfony') ? '1' : $input->getOption('api-version'); | |||
| $version = $input->getOption('api-version'); | |||
There was a problem hiding this comment.
What about this one @wouterj? should we keep support for the deprecated flag to provide a smooth transition?
Also, we're missing a deprecation notice if this should be removed in 7.0
There was a problem hiding this comment.
This was modified by #46901.
We need to keep the symfony option if we want developers to be able use to bash completion for different projects with different Symfony versions on the same computer. The completion script in symfony/console from 5.4.x to 6.1.x sets the option -S but not -a. If the option is removed, the _complete command will fail.
We can fallback to api-version=1 when not specified.
| $version = $input->getOption('api-version'); | |
| $version = $input->getOption('api-version', 1); |
It would be better to make this change in a dedicated PR.
There was a problem hiding this comment.
If I understand correctly, here is what you suggest:
- Create a PR targeting 6.4 as a new feature, making
api-versionfallback to1 - Update
symfonyoption deprecation to be removed in 8.0, when 5.4 is EOLed.
I'll take care of this if it is what you meant
There was a problem hiding this comment.
I think we should revert this change completely and leave it as-is. Notice the comment says "kept for compat" without deprecating the old one, which is intentional.
Shell completion scripts are global and there is no reason to make life extremely difficult for developers that work on several Symfony versions, some of which are older than 7.0, for the gain of removing these 2 LOC :)
There was a problem hiding this comment.
This suits me fine. As you said, these are just 2 lines of code 😄 I reverted this change
There was a problem hiding this comment.
Do I have something to do with this?
| $this->assertSame('This is a command I wrote all by myself', $command->getDescription()); | ||
| } | ||
|
|
||
| public function testAttributeOverridesProperty() |
There was a problem hiding this comment.
this one is not legacy 🤔
There was a problem hiding this comment.
That's right, but now that $defaultName and $defaultDescription are removed, it feels irrelevant to me to have this test. I guess this should have been marked as legacy as well when the properties were deprecated? Or maybe am I missing something?
However, I may update this test with a command without the attribute and checking that the getDefaultName() and getDefaultDescription() are null
There was a problem hiding this comment.
We should still keep it to ensure no regressions on the topic. (Removing depreciation annotations on the fixture)
There was a problem hiding this comment.
That's reverted and updated 👍
36591f1 to
41d1515
Compare
41d1515 to
6c89c12 to
7e81c91
Compare
|
Thank you @alexandre-daubois. |
In short: preparing the component for 7.0 🙂