Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

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

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[Console] Do not leak hidden console commands#33412

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

Merged
chalasr merged 1 commit intosymfony:4.4fromm-vo:ticket_33398
Sep 28, 2019

Conversation

@m-vo
Copy link
Contributor

@m-vom-vo commentedSep 1, 2019•
edited
Loading

QA
Branch?4.4 (updated)
Bug fix?yes
New feature?no
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets#33398
LicenseMIT

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 thatApplication->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 insidefind(). Maybe we should change this? Here are the relevant bits:

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

@nicolas-grekas
Copy link
Member

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

@m-vo
Copy link
ContributorAuthor

m-vo commentedSep 2, 2019

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 insideConsole/Application don't?

@nicolas-grekas
Copy link
Member

nicolas-grekas commentedSep 2, 2019•
edited
Loading

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

m-vo reacted with thumbs up emoji

@chalasr
Copy link
Member

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
Copy link
ContributorAuthor

m-vo commentedSep 2, 2019

@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-vom-vo changed the base branch from3.4 to4.4September 2, 2019 15:45
@m-vo
Copy link
ContributorAuthor

m-vo commentedSep 2, 2019•
edited
Loading

rebased for 4.4

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

@nicolas-grekas
Copy link
Member

fabbot patch applied in61dad16

@m-vo
Copy link
ContributorAuthor

m-vo commentedSep 20, 2019•
edited
Loading

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
Copy link
ContributorAuthor

Done.

@nicolas-grekas
Copy link
Member

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

@nicolas-grekas
Copy link
Member

(rebase needed btw, to see tests green)

@chalasr
Copy link
Member

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

m-vo reacted with thumbs up emoji

nicolas-grekas added a commit that referenced this pull requestSep 28, 2019
…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/aPartially 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
@chalasrchalasrforce-pushed theticket_33398 branch 2 times, most recently from38e67fd to92b5a29CompareSeptember 28, 2019 15:00
@chalasr
Copy link
Member

Thank you@m-vo.

m-vo reacted with hooray emoji

chalasr added a commit that referenced this pull requestSep 28, 2019
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       | MITThis 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-L502Commits-------f340633 [Console] Deprecate abbreviating hidden command names using  Application->find()
@chalasrchalasr merged commitf340633 intosymfony:4.4Sep 28, 2019
@nicolas-grekasnicolas-grekas modified the milestones:next,4.4Oct 27, 2019
This was referencedNov 12, 2019
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@fabpotfabpotfabpot requested changes

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@chalasrchalasrchalasr approved these changes

@dunglasdunglasAwaiting requested review from dunglas

@lyrixxlyrixxAwaiting requested review from lyrixx

@srozesrozeAwaiting requested review from sroze

@xabbuhxabbuhAwaiting requested review from xabbuh

@ycerutoycerutoAwaiting requested review from yceruto

Assignees

No one assigned

Projects

None yet

Milestone

4.4

Development

Successfully merging this pull request may close these issues.

5 participants

@m-vo@nicolas-grekas@chalasr@fabpot@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp