[Console] Do not leak hidden console commands #33412

m-vo · 2019-09-01T18:02:42Z

Q	A
Branch?	4.4 (updated)
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#33398
License	MIT

This PR attempts to fix hidden console commands to be leaked when interacting with the console.

These are the changes:

Hidden commands won't be shown anymore in the list of commands in a namespace as well as the list of suggestions ("Did you mean...") for invalid or ambiguous commands.
Hidden commands therefore now need to be always entered with their full name.
If an abbreviated command is entered that was previously ambiguous with (only) hidden commands, it's now executed directly (not ambiguous anymore).

Side note: When implementing the tests & changes I realized that Application->get() isn't side effect free (when redirecting to the help command) and behaves differently when called multiple times. It therefore must not be used from inside find(). Maybe we should change this? Here are the relevant bits:

symfony/src/Symfony/Component/Console/Application.php

Lines 495 to 502 in f71f74b

    
           if ($this->wantHelps) { 
        
               $this->wantHelps = false; 
        
               $helpCommand = $this->get('help'); 
        
               $helpCommand->setCommand($command); 
        
               return $helpCommand; 
        
           }

nicolas-grekas · 2019-09-01T22:09:46Z

Shouldn't we do this on 4.4, to not break scripts that use a short alias into the wild?

m-vo · 2019-09-02T08:16:58Z

Agreed, that's probably a good idea as this issue is rather a cosmetic one.
(I think commands in scripts should never be abbreviated though but who knows... 😄 )

Do you want me to rebase on 4.4?*

*) btw: Should I add return type hints for new functions in this case even though the other functions inside Console/Application don't?

nicolas-grekas · 2019-09-02T11:14:09Z

Please rebase for 4.4 yes, and you can add types for sure.

chalasr · 2019-09-02T11:48:09Z

Hidden commands won't be shown anymore in the list of commands in a namespace as well as the list of suggestions ("Did you mean...") for invalid or ambiguous commands.

This part should be for 3.4.

m-vo · 2019-09-02T12:53:35Z

@chalasr How would that work without the other changes? Assume you're input is ambiguous and you get a suggestion with two commands where one of them is hidden. Now stripping the hidden one would return a single command saying that it is ambiguous. Isn't that worse that leaving it as it is?

m-vo · 2019-09-02T15:48:20Z

rebased for 4.4

fabbot reporting about CS seems unrelated to my changes (it's about unsorted use statements).

src/Symfony/Component/Console/Application.php

nicolas-grekas · 2019-09-02T16:04:03Z

fabbot patch applied in 61dad16

src/Symfony/Component/Console/Application.php

m-vo · 2019-09-20T10:22:28Z

I accidentally rebased against the wrong branch first and then against 4.4 again. ~~Not sure what went wrong there because github is showing lots of commits now.~~ Of course I know what happened. I am silly. I'm going to fix this (hopefully tomorrow). Shouldn't do things in passing... 🤦‍♂️

m-vo · 2019-09-20T10:29:03Z

Done.

nicolas-grekas · 2019-09-26T21:47:34Z

@chalasr we need your help to unlock here (you can also approve as is :) )

nicolas-grekas · 2019-09-26T21:48:24Z

(rebase needed btw, to see tests green)

chalasr · 2019-09-28T14:51:01Z

PR rebased + added 1 commit to turn the CommandNotFoundException into a deprecation notice in case one uses an abbreviation for finding an hidden command.
Bugfix part moved to its own PR for 3.4: #33748

…rnatives (m-vo) This PR was merged into the 3.4 branch. Discussion ---------- [Console] Do not include hidden commands in suggested alternatives | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | Fix #33398 | License | MIT | Doc PR | n/a Partially backports #33412 on 3.4, avoids leaking the names of hidden commands in suggested alternatives on command not found. Commits ------- 8a9d173 Do not include hidden commands in suggested alternatives

…on->find()

chalasr · 2019-09-28T15:03:53Z

Thank you @m-vo.

This PR was merged into the 4.4 branch. Discussion ---------- [Console] Do not leak hidden console commands | Q | A | ------------- | --- | Branch? | 4.4 (updated) | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #33398 | License | MIT This PR attempts to fix hidden console commands to be leaked when interacting with the console. These are the changes: * Hidden commands won't be shown anymore in the list of commands in a namespace as well as the list of suggestions ("Did you mean...") for invalid or ambiguous commands. * Hidden commands therefore now need to be always entered with their full name. * If an abbreviated command is entered that was previously ambiguous with (only) hidden commands, it's now executed directly (not ambiguous anymore). Side note: When implementing the tests & changes I realized that `Application->get()` isn't side effect free (when redirecting to the help command) and behaves differently when called multiple times. It therefore must not be used from inside `find()`. Maybe we should change this? Here are the relevant bits: https://github.com/symfony/symfony/blob/f71f74b36a80227d3e68f1b65b1f1d9b42fa9952/src/Symfony/Component/Console/Application.php#L495-L502 Commits ------- f340633 [Console] Deprecate abbreviating hidden command names using Application->find()

carsonbot added Status: Needs Review Console Bug labels Sep 1, 2019

m-vo force-pushed the ticket_33398 branch from f71f74b to 2bb40fe Compare September 1, 2019 19:13

nicolas-grekas modified the milestones: next, 3.4 Sep 1, 2019

m-vo force-pushed the ticket_33398 branch from 2bb40fe to afcdc6e Compare September 2, 2019 15:44

m-vo requested review from dunglas, lyrixx, sroze and xabbuh as code owners September 2, 2019 15:44

m-vo changed the base branch from 3.4 to 4.4 September 2, 2019 15:45

nicolas-grekas reviewed Sep 2, 2019

View reviewed changes

src/Symfony/Component/Console/Application.php Outdated Show resolved Hide resolved

nicolas-grekas reviewed Sep 2, 2019

View reviewed changes

src/Symfony/Component/Console/Application.php Outdated Show resolved Hide resolved

nicolas-grekas approved these changes Sep 2, 2019

View reviewed changes

src/Symfony/Component/Console/Application.php Outdated Show resolved Hide resolved

carsonbot added Status: Reviewed and removed Status: Needs Review labels Sep 2, 2019

fabpot requested changes Sep 3, 2019

View reviewed changes

src/Symfony/Component/Console/Application.php Outdated Show resolved Hide resolved

carsonbot added Status: Needs Work and removed Status: Reviewed labels Sep 3, 2019

nicolas-grekas modified the milestones: 3.4, next Sep 6, 2019

m-vo force-pushed the ticket_33398 branch from 1d65b04 to 2f4ba66 Compare September 20, 2019 10:28

chalasr force-pushed the ticket_33398 branch 5 times, most recently from 8573af8 to 24a98d7 Compare September 28, 2019 14:37

chalasr mentioned this pull request Sep 28, 2019

[Console] Do not include hidden commands in suggested alternatives #33748

Merged

chalasr force-pushed the ticket_33398 branch 2 times, most recently from 38e67fd to 92b5a29 Compare September 28, 2019 15:00

[Console] Deprecate abbreviating hidden command names using Applicati…

f340633

…on->find()

chalasr force-pushed the ticket_33398 branch from 92b5a29 to f340633 Compare September 28, 2019 15:01

chalasr approved these changes Sep 28, 2019

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Sep 28, 2019

chalasr merged commit f340633 into symfony:4.4 Sep 28, 2019

chalasr added Feature Deprecation and removed Bug labels Sep 28, 2019

nicolas-grekas modified the milestones: next, 4.4 Oct 27, 2019

This was referenced Nov 12, 2019

Release v4.4.0-BETA1 #34338

Merged

Release v5.0.0-BETA1 #34339

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Console] Do not leak hidden console commands #33412

[Console] Do not leak hidden console commands #33412

m-vo commented Sep 1, 2019 •

edited

Loading

nicolas-grekas commented Sep 1, 2019

m-vo commented Sep 2, 2019

nicolas-grekas commented Sep 2, 2019 •

edited

Loading

chalasr commented Sep 2, 2019

m-vo commented Sep 2, 2019

m-vo commented Sep 2, 2019 •

edited

Loading

nicolas-grekas commented Sep 2, 2019

m-vo commented Sep 20, 2019 •

edited

Loading

m-vo commented Sep 20, 2019

nicolas-grekas commented Sep 26, 2019

nicolas-grekas commented Sep 26, 2019

chalasr commented Sep 28, 2019

chalasr commented Sep 28, 2019

	if ($this->wantHelps) {
	$this->wantHelps = false;

	$helpCommand = $this->get('help');
	$helpCommand->setCommand($command);

	return $helpCommand;
	}

[Console] Do not leak hidden console commands #33412

[Console] Do not leak hidden console commands #33412

Conversation

m-vo commented Sep 1, 2019 • edited Loading

nicolas-grekas commented Sep 1, 2019

m-vo commented Sep 2, 2019

nicolas-grekas commented Sep 2, 2019 • edited Loading

chalasr commented Sep 2, 2019

m-vo commented Sep 2, 2019

m-vo commented Sep 2, 2019 • edited Loading

nicolas-grekas commented Sep 2, 2019

m-vo commented Sep 20, 2019 • edited Loading

m-vo commented Sep 20, 2019

nicolas-grekas commented Sep 26, 2019

nicolas-grekas commented Sep 26, 2019

chalasr commented Sep 28, 2019

chalasr commented Sep 28, 2019

m-vo commented Sep 1, 2019 •

edited

Loading

nicolas-grekas commented Sep 2, 2019 •

edited

Loading

m-vo commented Sep 2, 2019 •

edited

Loading

m-vo commented Sep 20, 2019 •

edited

Loading