NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.7k

[Console] Remove deprecations across the component#50613

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 ourterms of service andprivacy 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 intosymfony:7.0fromalexandre-daubois:console-deprecations

Jun 30, 2023

Merged

[Console] Remove deprecations across the component#50613

nicolas-grekas merged 1 commit intosymfony:7.0fromalexandre-daubois:console-deprecations

Jun 30, 2023

Conversation

Copy link

Member

alexandre-daubois commentedJun 9, 2023•
edited
Loading

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

In short: preparing the component for 7.0 🙂

alexandre-daubois requested review fromchalasr,xabbuh andyceruto ascode owners

June 9, 2023 11:37

carsonbot added Status: Needs Review Console Feature labels

Jun 9, 2023

carsonbot added this to the7.0 milestone

Jun 9, 2023

alexandre-daubois force-pushed theconsole-deprecations branch 2 times, most recently from903ba6a to171965fCompare

June 9, 2023 12:16

nicolas-grekas reviewed

Jun 9, 2023

View reviewed changes

Copy link

Member

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.

let's finish existing PRs before opening new ones ;)

src/Symfony/Bridge/Monolog/Command/ServerLogCommand.php OutdatedShow resolvedHide resolved

alexandre-daubois force-pushed theconsole-deprecations branch from171965f to740e684Compare

June 13, 2023 15:29

alexandre-daubois changed the title~~[Console] Remove deprecations and add types across the component~~[Console] Remove deprecations across the component

Jun 13, 2023

alexandre-daubois commented

Jun 13, 2023

View reviewed changes

src/Symfony/Component/Runtime/composer.json Outdated

		"composer/composer":"^2.6",
		"symfony/console":"^6.4\|^7.0",
		"symfony/dotenv":"^6.4\|^7.0",
		"symfony/finder":"^6.4",

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

This is necessary becausecomposer/class-map-generator doesn't supportSymfony ^7.0 yet. The highest dep for the Finder must be set to^6.4 because of this for now.

Copy link

Member

Choose a reason for hiding this comment

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

please send a PR to class-map-generator instead

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

It has been dealt by@stloyd here, thanks!

I'll revert the change once it's merged.

Copy link

Member

Choose a reason for hiding this comment

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

Unlocked!

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

I was going to update when I saw you updated the file. Thank you! 🙂

alexandre-daubois commented

Jun 13, 2023

View reviewed changes

src/Symfony/Component/Runtime/composer.jsonShow resolvedHide resolved

alexandre-daubois force-pushed theconsole-deprecations branch 7 times, most recently from1a9a9f0 toa1854beCompare

June 13, 2023 15:52

nicolas-grekas reviewed

Jun 29, 2023

View reviewed changes

src/Symfony/Component/Console/Command/CompleteCommand.php Outdated

		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');

Copy link

Member

Choose a reason for hiding this comment

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

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

Copy link

Member

GromNaNJun 29, 2023•
edited
Loading

Choose a reason for hiding this comment

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

This was modified by#46901.

We need to keep thesymfony 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 insymfony/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.

symfony/src/Symfony/Component/Console/Resources/completion.bash

Line 28 ina6e5b75

local completecmd=("$sf_cmd""_complete""--no-interaction""-sbash""-c$cword""-S{{ VERSION }}")

We can fallback toapi-version=1 when not specified.

Suggested change

	$version =$input->getOption('api-version');
	$version =$input->getOption('api-version',1);

It would be better to make this change in a dedicated PR.

Copy link

MemberAuthor

alexandre-dauboisJun 29, 2023

Choose a reason for hiding this comment

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

If I understand correctly, here is what you suggest:

Create a PR targeting 6.4 as a new feature, makingapi-version fallback to1
Updatesymfony option deprecation to be removed in 8.0, when 5.4 is EOLed.

I'll take care of this if it is what you meant

Copy link

Member

wouterjJun 29, 2023

Choose a reason for hiding this comment

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

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 :)

Copy link

MemberAuthor

alexandre-dauboisJun 30, 2023

Choose a reason for hiding this comment

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

This suits me fine. As you said, these are just 2 lines of code 😄 I reverted this change

Copy link

GWilson69Jul 16, 2023

Choose a reason for hiding this comment

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

Do I have something to do with this?

nicolas-grekas reviewed

Jun 29, 2023

View reviewed changes

src/Symfony/Component/Console/Tests/Command/CommandTest.php

		$this->assertSame('This is a command I wrote all by myself',$command->getDescription());
		}

		publicfunctiontestAttributeOverridesProperty()

Copy link

Member

nicolas-grekasJun 29, 2023•
edited
Loading

Choose a reason for hiding this comment

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

this one is not legacy 🤔

Copy link

MemberAuthor

alexandre-dauboisJun 29, 2023•
edited
Loading

Choose a reason for hiding this comment

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

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 thegetDefaultName() andgetDefaultDescription() are null