[Console] Add of hidden and deprecation option flags #54439

flkasper · 2024-03-29T18:29:05Z

Q	A
Branch?	7.1
Bug fix?	no
New feature?	yes
Deprecations?	no
Issues	resolves #54206
License	MIT

Add new InputOption flags HIDDEN and DEPRECATED.

Hidden options are not printed to the console output (or other descriptors) unless the hidden option --show-hidden-options is used.

If a deprecated flagged option is used, a deprecation message is output before execution (execute method).
Deprecated flagged options are coloured red in the help output.

Example:

#[AsCommand(name: 'test', description: 'description')]
class TestCommand extends Command
{
    protected function configure()
    {
        $this->addOption('test-hidden', null, InputOption::VALUE_NONE | InputOption::HIDDEN, 'HIDDEN option');
        $this->addOption('test-deprecated', null, InputOption::VALUE_NONE | InputOption::DEPRECATED, 'DEPRECATED option');
    }

    protected function execute(InputInterface $input, OutputInterface $output): int
    {
        // ...
        return self::SUCCESS;
    }
}

Example usage:

carsonbot · 2024-03-29T18:29:12Z

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has a contribution guide which I suggest you to read.

In short:

Always add tests
Keep backward compatibility (see https://symfony.com/bc).
Bug fixes must be submitted against the lowest maintained branch where they apply (see https://symfony.com/releases)
Features and deprecations must be submitted against the 7.1 branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

94noni · 2024-03-30T09:21:23Z

Waw very nice :)
Passing by, i would just split the PR commits into 2 separated PRs
Feature addition and cs fix on the other

flkasper · 2024-03-30T16:58:32Z

@94noni I actually only wanted to fix my commit via ecs and had therefore limited ecs to the console bundle only.
Who would have thought that the code would differ in so many places :)

94noni · 2024-03-30T19:47:02Z

Got it :)
Lets wait other reviews 👌🏼

smnandre · 2024-03-30T21:20:31Z

Love the idea... but i feel really agressed by the red, and i feel it makes the option an unwanted point of attention.

Did you try with other colors ? Something muted maybe to get the opposite effect ?

I think we should not rely only on colors (for accessibility reasons but also technical ones),

What about an automatic [deprecated] prefix in the description ?

Options:
    -test-deprecated            [deprecated] Lorem ipsum ...

With [deprecated] not having to be set in the option description

flkasper · 2024-03-31T13:26:30Z

Love the idea... but i feel really agressed by the red, and i feel it makes the option an unwanted point of attention.
Did you try with other colors ? Something muted maybe to get the opposite effect ?

I had originally thought of yellow (warning), but this is already used in the headings such as "Options:".

I have now simply output in every available style and colour for comparison.
Personally, I think that yellow, gray and white would suit both in light and dark mode.

flkasper · 2024-03-31T13:29:30Z

What about an automatic [deprecated] prefix in the description ?

I had also thought of this and it is actually a nice idea, but it has the disadvantage that you cannot simply customise this prefix.
See screenshots for the colours.

You would then have to add something like this (either to set the prefix or the message):

private array $optionDeprecationPrefix = [];

public function setOptionDeprecationPrefix(string $optionName, string $deprecationPrefix) {
    $this->optionDeprecationPrefix[$optionName] = $deprecationPrefix;
}

// On output:
$deprecationPrefix =  $this->optionDeprecationPrefix[$optionName] ??  'deprecated';
"[$deprecationPrefix] $optionDescription"

smnandre · 2024-03-31T18:00:00Z

[...] grey [...]

Would be my personal choices, but colors are subjective, so let's see what others think about it ? :)

the disadvantage that you cannot simply customise this prefix.

That was not (in my mind) something configurable so i did not add the constraint.

But if you have commands from your app and bundles, that would be better to not allow a different style per command, no ?

I guess i'd do something like

sf list 

lorem:list            [deprecated] List all lorem from ipsum


sf list -v

lorem:list            [deprecated since 2.0] List all lorem from ipsum


sf list -vv

lorem:list            [deprecated since 2.0] List all lorem from ipsum  [Use foo:bar instead]

GromNaN · 2024-03-31T20:22:08Z

I like the grey too. We want this options to be less visible in the list, not highlight them.

And I don't see any reason for customizing the description prefix either. Consistency across commands is better.

GromNaN

I really like this feature. I did a few tests on an application. It works very well, congratulations for your work.

There's a comment about the depreciation message display. It may take a deeper work to detect if an option has been passed. This could be a feature in its own.

The large number of style changes unrelated to the PR make review more difficult. These changes should be made in dedicated PRs with arguments. They can have an impact on maintenance (and upmerges from old branches to new ones).

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

src/Symfony/Component/Console/Completion/CompletionInput.php

src/Symfony/Component/Console/Descriptor/Descriptor.php

src/Symfony/Component/Console/.editorconfig

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

src/Symfony/Component/Console/Helper/ProgressIndicator.php

src/Symfony/Component/Console/Input/InputOption.php

src/Symfony/Component/Console/Tests/Fixtures/Style/SymfonyStyle/command/command_4.php

GromNaN · 2024-04-07T16:18:03Z

src/Symfony/Component/Console/Tests/Fixtures/input_option_hidden.json

@@ -0,0 +1 @@
+NULL


Why is there a NULL char here?

I have extended the existing test providers with corresponding test cases for hidden options.

NULL is a valid json (string) and unfortunately necessary, as otherwise JsonDescriptorTest::testDescribeInputOption would fail.

src/Symfony/Component/Console/Tests/Helper/TableTest.php

- Included option shortcut in deprecation message - Applied phpdoc suggestion - Applied remove of console .editorconfig

src/Symfony/Component/Console/Input/InputOption.php

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

GromNaN · 2024-04-11T14:23:57Z

src/Symfony/Component/Console/Tests/Fixtures/Foo1Command.php

+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */


Is it php-cs-fixer that caused this license block to be added? That can't be done in this PR because it makes it look like there are a lot more modified files than actually necessary for this feature.
Please revert all formatting changes not related to the PR.

the config of php-cs-fixer should be ignoring all files in Fixtures folder, so I doubt it is the case (unless a custom config was used, which would be a no-go)

Is it php-cs-fixer that caused this license block to be added?

Yes, this is due to the php-cs-fixer.

I have used the config .php-cs-fixer.dist.php from the root.
Rule header_comment is being configured there.
No idea why the fixtures are also changed.

#Executed command: php tools/php-cs-fixer/vendor/bin/php-cs-fixer fix --config=.php-cs-fixer.dist.php <folder>

I'll look over it again and remove all unrelated changes to the PR.

When you pass a directory, you need to use --path-mode=intersection (from memory, maybe some typo there) or phpcs ignores the previously defined paths .. and exclusions.

@smnandre phpcs is a different tool. I think you mean php-cs-fixer.

Indeed sorry *

php php-cs-fixer.phar fix --path-mode=intersection /path/to/dir

https://cs.symfony.com/doc/usage.html#the-fix-command

(* my local alias for php-cs-fixer is... phpcs 😶 )

stof · 2024-04-11T14:36:29Z

I haven't checked the current state of the PR yet. But for sure, we cannot rely on color only:

color blind people won't see them, causing accessibility issues
not all terminals support colors

… TextDescriptor.php

flkasper · 2024-04-11T23:19:22Z

I have added @smnandre's suggestion for "[deprecated] prefix to the option description in the TextDescriptor.

GromNaN

LGTM, with a last suggestion.

src/Symfony/Component/Console/Input/InputOption.php

flkasper requested a review from chalasr as a code owner March 29, 2024 18:29

carsonbot added Status: Needs Review Console Feature labels Mar 29, 2024

carsonbot added this to the 7.1 milestone Mar 29, 2024

flkasper added 2 commits March 29, 2024 19:45

[Console] Add of hidden and deprecation option flags

332b4ef

Fix code style

6639321

flkasper force-pushed the feature/7.1-hidden-and-deprecated-options branch from bfd9bfc to 6639321 Compare March 29, 2024 18:45

Change style of deprecated option to inline formatting

7a42e70

flkasper added 2 commits April 6, 2024 16:50

[Console] Change color of deprecated option to gray

218b9da

[Console] Fixed test after color change

4913d58

GromNaN reviewed Apr 7, 2024

View reviewed changes

flkasper added 2 commits April 8, 2024 10:01

[Console] Revert unrelated (style) changes

eeefd38

[Console] Apply suggestion changes

f686ee3

- Included option shortcut in deprecation message - Applied phpdoc suggestion - Applied remove of console .editorconfig

smnandre reviewed Apr 9, 2024

View reviewed changes

src/Symfony/Component/Console/Input/InputOption.php Outdated Show resolved Hide resolved

[Console] Fixed name of method

0487e76

GromNaN reviewed Apr 11, 2024

View reviewed changes

flkasper added 2 commits April 12, 2024 00:37

[Console] Revert unrelated (style) changes

3f1bdd1

[Console] Applied change suggestion in Command::writeDeprecationMessages

4c4a254

[Console] Added suggested description prefix to deprecated options in…

48ce995

… TextDescriptor.php

GromNaN approved these changes Apr 13, 2024

View reviewed changes

src/Symfony/Component/Console/Input/InputOption.php Outdated Show resolved Hide resolved

carsonbot added Status: Reviewed and removed Status: Needs Review labels Apr 13, 2024

[Console] Apply suggestion & fixed coding standard of changes

8713df0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Console] Add of hidden and deprecation option flags #54439

[Console] Add of hidden and deprecation option flags #54439

flkasper commented Mar 29, 2024 •

edited

carsonbot commented Mar 29, 2024

94noni commented Mar 30, 2024

flkasper commented Mar 30, 2024

94noni commented Mar 30, 2024

smnandre commented Mar 30, 2024

flkasper commented Mar 31, 2024 •

edited

flkasper commented Mar 31, 2024 •

edited

smnandre commented Mar 31, 2024

GromNaN commented Mar 31, 2024

GromNaN left a comment

GromNaN Apr 7, 2024

flkasper Apr 8, 2024

GromNaN Apr 11, 2024

stof Apr 11, 2024

flkasper Apr 11, 2024 •

edited

smnandre Apr 12, 2024

javaDeveloperKid Apr 14, 2024

smnandre Apr 14, 2024

stof commented Apr 11, 2024

flkasper commented Apr 11, 2024

GromNaN left a comment

[Console] Add of hidden and deprecation option flags #54439

Are you sure you want to change the base?

[Console] Add of hidden and deprecation option flags #54439

Conversation

flkasper commented Mar 29, 2024 • edited

carsonbot commented Mar 29, 2024

94noni commented Mar 30, 2024

flkasper commented Mar 30, 2024

94noni commented Mar 30, 2024

smnandre commented Mar 30, 2024

flkasper commented Mar 31, 2024 • edited

flkasper commented Mar 31, 2024 • edited

smnandre commented Mar 31, 2024

GromNaN commented Mar 31, 2024

GromNaN left a comment

Choose a reason for hiding this comment

GromNaN Apr 7, 2024

Choose a reason for hiding this comment

flkasper Apr 8, 2024

Choose a reason for hiding this comment

GromNaN Apr 11, 2024

Choose a reason for hiding this comment

stof Apr 11, 2024

Choose a reason for hiding this comment

flkasper Apr 11, 2024 • edited

Choose a reason for hiding this comment

smnandre Apr 12, 2024

Choose a reason for hiding this comment

javaDeveloperKid Apr 14, 2024

Choose a reason for hiding this comment

smnandre Apr 14, 2024

Choose a reason for hiding this comment

stof commented Apr 11, 2024

flkasper commented Apr 11, 2024

GromNaN left a comment

Choose a reason for hiding this comment

flkasper commented Mar 29, 2024 •

edited

flkasper commented Mar 31, 2024 •

edited

flkasper commented Mar 31, 2024 •

edited

flkasper Apr 11, 2024 •

edited