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

flkasper · Mar 29, 2024

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 · Mar 29, 2024

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 · Mar 30, 2024

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 · Mar 30, 2024

@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 · Mar 30, 2024

Got it :)
Lets wait other reviews 👌🏼

smnandre · Mar 30, 2024

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 · Mar 31, 2024

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 · Mar 31, 2024

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 · Mar 31, 2024

[...] 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 · Mar 31, 2024

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 · Apr 7, 2024

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

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

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

GromNaN · Apr 11, 2024

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 · Apr 11, 2024

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

flkasper · Apr 11, 2024

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

nicolas-grekas

LGTM, I just have minor comments. Can you please rebase also?

nicolas-grekas · Aug 22, 2024

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

@@ -273,6 +275,10 @@ public function run(InputInterface $input, OutputInterface $output): int

        $input->validate();

+        if (isset($inputDefinition)) {


why the isset?

Suggested change

if (isset($inputDefinition)) {

if ($inputDefinition) {

$inputDefinition is defined in a try..catch, but should be outside.

As @GromNaN has already explained, $inputDefinition is defined within a try...catch:

try { $inputDefinition = $this->getDefinition(); $input->bind($inputDefinition); } catch (ExceptionInterface $e) { if (!$this->ignoreValidationErrors) { throw $e; } }

Since getDefinition can also throw an exception (if the parent constructor call is missing), moving it outside the try...catch makes little sense.

Therefore, isset must be used to check whether $inputDefinition is defined.

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

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

src/Symfony/Component/Console/Tests/Input/InputOptionTest.php

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

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

nicolas-grekas · Aug 22, 2024

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

+     *
+     * @return bool true if mode is $mode, false otherwise
+     */
+    protected function hasMode(int $mode): bool


I'd rather not add a new method, it's unrelated to the proposed feature

Is an internal function and simplifies and reduces duplicate/similar code

This method is not added as an internal method. You added it as part of the semver API.

If you want to keep the helper, make it private instead.

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

… TextDescriptor.php

Co-authored-by: Nicolas Grekas <nicolas.grekas@gmail.com>

stof · Sep 8, 2024

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

@@ -70,6 +70,7 @@ protected function describeInputOption(InputOption $option, array $options = [])
            .'* Accept value: '.($option->acceptValue() ? 'yes' : 'no')."\n"
            .'* Is value required: '.($option->isValueRequired() ? 'yes' : 'no')."\n"
            .'* Is multiple: '.($option->isArray() ? 'yes' : 'no')."\n"
+            .($option->isDeprecated() ? ('* Is deprecated: yes'."\n") : '')


shouldn't we have a line Is deprecated: no instead ?

stof · Sep 8, 2024

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

            'description' => preg_replace('/\s*[\r\n]\s*/', ' ', $option->getDescription()),
            'default' => \INF === $option->getDefault() ? 'INF' : $option->getDefault(),
        ];
+        if (!$option->isDeprecated()) {
+            unset($data['is_deprecated']);


I would not unset this. The json is easier to use if we don't have conditional keys in it.

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 force-pushed the feature/7.1-hidden-and-deprecated-options branch from bfd9bfc to 6639321 Compare March 29, 2024 18:45

GromNaN reviewed Apr 7, 2024

View reviewed changes

smnandre reviewed Apr 9, 2024

View reviewed changes

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

GromNaN reviewed Apr 11, 2024

View reviewed changes

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

xabbuh modified the milestones: 7.1, 7.2 May 15, 2024

94noni approved these changes Aug 12, 2024

View reviewed changes

nicolas-grekas requested changes Aug 22, 2024

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Reviewed labels Aug 22, 2024

Luc45 mentioned this pull request Sep 6, 2024

Activation refactor backwards compatibility woocommerce/qit-cli#203

Merged

flkasper and others added 13 commits September 7, 2024 22:24

[Console] Add of hidden and deprecation option flags

2cf5da9

Fix code style

d203a4e

Change style of deprecated option to inline formatting

4d86a42

[Console] Change color of deprecated option to gray

69b4bf8

[Console] Fixed test after color change

e2d3f12

[Console] Revert unrelated (style) changes

ece8b7a

[Console] Apply suggestion changes

a08e4d5

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

[Console] Fixed name of method

02f0e0e

[Console] Revert unrelated (style) changes

d4be218

[Console] Applied change suggestion in Command::writeDeprecationMessages

54baa40

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

9a9ab17

… TextDescriptor.php

[Console] Apply suggestion & fixed coding standard of changes

cda876f

Apply suggestions from code review

c615abe

Co-authored-by: Nicolas Grekas <nicolas.grekas@gmail.com>

flkasper requested review from lyrixx, yceruto, welcoMattic, kbond, dunglas, OskarStark, jderusse and xabbuh as code owners September 7, 2024 20:30

carsonbot added Status: Needs Review and removed Status: Needs Work labels Sep 7, 2024

flkasper force-pushed the feature/7.1-hidden-and-deprecated-options branch from 893fdf3 to c615abe Compare September 7, 2024 20:32

flkasper added 2 commits September 7, 2024 22:35

Cleanup after rebase

6602bb4

Apply cs patch

05ad7d4

stof reviewed Sep 8, 2024

View reviewed changes

fabpot modified the milestones: 7.2, 7.3 Nov 20, 2024

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

Loading

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

Loading

flkasper commented Mar 31, 2024 •

edited

Loading

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

Loading

smnandre Apr 12, 2024

ghost Apr 14, 2024

smnandre Apr 14, 2024

stof commented Apr 11, 2024

flkasper commented Apr 11, 2024

GromNaN left a comment

nicolas-grekas left a comment

nicolas-grekas Aug 22, 2024

GromNaN Aug 22, 2024

flkasper Aug 26, 2024 •

edited

Loading

nicolas-grekas Aug 22, 2024

flkasper Aug 26, 2024 •

edited

Loading

stof Sep 8, 2024

stof Sep 8, 2024

stof Sep 8, 2024

Search code, repositories, users, issues, pull requests...

[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 Loading

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 Loading

flkasper commented Mar 31, 2024 • edited Loading

smnandre commented Mar 31, 2024

GromNaN commented Mar 31, 2024

GromNaN left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flkasper Apr 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

nicolas-grekas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flkasper Aug 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flkasper Aug 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flkasper commented Mar 29, 2024 •

edited

Loading

flkasper commented Mar 31, 2024 •

edited

Loading

flkasper commented Mar 31, 2024 •

edited

Loading

flkasper Apr 11, 2024 •

edited

Loading

flkasper Aug 26, 2024 •

edited

Loading

flkasper Aug 26, 2024 •

edited

Loading