[Console] Fix completion #47780

h3xx · 2022-10-03T23:20:49Z

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

Set COMP_WORDBREAKS instead of IFS in the completion script.

More in-depth explanation:

IFS affects other things not related to programmable completion, such as how the shell breaks up argument lists.

For example, by default this splits the unquoted variable as-expected:

OPTION_LIST='foo bar'
for OPT in $OPTION_LIST; do printf '%s\n' "$OPT"; done
> foo
> bar

Changing IFS makes the variable not split as expected, printing all on one line:

IFS=$'\n'
for OPT in $OPTION_LIST; do printf '%s\n' "$OPT"; done
> foo bar

This PR changes it to use $COMP_WORDBREAKS which specifically affects how programmable completion works, instead of affecting and effectively poisoning how the user's shell works.

carsonbot · 2022-10-03T23:20:53Z

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 6.2 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

GromNaN · 2022-10-04T07:31:48Z

Hello @h3xx, thank you for the patch and the details. I worked on this completion script and would like to make some tests.

I'm a beginner in bash scripting, but apparently updating COMP_WORDBREAKS is not more recommended than IFS.

https://stackoverflow.com/a/12495480

Did you hit a specific issue?

h3xx · 2022-10-04T15:53:27Z

I'm a beginner in bash scripting, but apparently updating COMP_WORDBREAKS is not more recommended than IFS. https://stackoverflow.com/a/12495480

Interesting. I didn't know that's what __ltrim_colon_completions did. I'll see if it makes sense to integrate a call to that function instead of modifying COMP_WORDBREAKS. I'm still convinced that modifying IFS isn't the right thing to do -- see issue below.

I worked on this completion script and would like to make some tests.

I'm all in favor of unit tests :-). I'll see what I can come up with, integrating a test for my issue into CI.

Did you hit a specific issue?

Yes, the issue is outlined in the PR's description, but a more concise failure case is thus:

On my system, ls is an alias for /bin/ls $LS_OPTIONS, $LS_OPTIONS is a space-separated list of options:

alias ls='/bin/ls $LS_OPTIONS'
LS_OPTIONS="-F -b -T 0 --color=auto --block-size='1"
ls
CHANGELOG-6.0.md    CONTRIBUTORS.md  UPGRADE-6.1.md  phpunit.xml.dist
CHANGELOG-6.1.md    LICENSE          composer.json   psalm.xml
CODE_OF_CONDUCT.md  README.md        link*           src/
CONTRIBUTING.md     UPGRADE-6.0.md   phpunit*

After using this function the alias would no longer work:

eval "$(sed -e 's,{{ COMMAND_NAME }},foo,g' < src/Symfony/Component/Console/Resources/completion.bash)"
_sf_foo
ls
ls: invalid option -- ' '
Try 'ls --help' for more information.

Changing IFS poisons the shell environment. Use COMP_WORDBREAKS instead.

h3xx · 2022-10-12T23:05:25Z

Okay, I've added a unit test that successfully reproduces my issue. Now, I'm looking into the __ltrim_colon_completions solution you suggested.

GromNaN

Thank you very much for working on a test. I tried on a Mac Intel and in Docker Alpine: it fails (see other comments).

The fix actually works and fixes the reported issues. We can keep only the 1st commit and work on tests in a separate PR.

src/Symfony/Component/Console/Tests/Resources/BashCompletionTest.php

GromNaN · 2022-11-12T19:18:53Z

src/Symfony/Component/Console/Tests/Resources/BashCompletionTest.php

+        $possibles = [
+            // Paths to where required shell functions reside
+            '/etc/bash_completion',
+            '/usr/share/bash-completion/bash_completion',


On Mac Intel, none of this files exists. There is /usr/local/Cellar/bash-completion/1.3_3/etc/bash_completion and /usr/local/Cellar/bash-completion@2/2.11/share/bash-completion/bash_completion.

On Mac Intel, is bash completion even supported outside of Homebrew?

The real question is, does CI need to run/pass on a Mac? Are any of the CI images used OSX-based?

Yes, tests must run on all supported environments. They could be skipped if some preconditions are not meet (bash completion not installed).

src/Symfony/Component/Console/Tests/Resources/BashCompletionTest.php

Code review suggestion. Co-authored-by: Jérôme Tamarelle <jerome@tamarelle.net>

nicolas-grekas · 2022-12-13T17:13:37Z

What's the status here?

h3xx · 2022-12-13T20:38:49Z

What's the status here?

@nicolas-grekas It's "on my list of things to do." Life is pretty hectic atm.

If it lessens your mental load, feel free to close this PR and I will re-open when I get around it.

nicolas-grekas · 2022-12-14T15:24:41Z

No worries, I was just wondering. When you can!

chalasr · 2025-03-29T15:26:54Z

Closing as there is no more activity. Please feel free to reopen

h3xx requested a review from chalasr as a code owner October 3, 2022 23:20

carsonbot added Status: Needs Review Bug Console labels Oct 3, 2022

carsonbot added this to the 6.1 milestone Oct 3, 2022

h3xx force-pushed the fix-completion branch 2 times, most recently from 7efccdd to 5ec1459 Compare October 3, 2022 23:48

h3xx requested review from xabbuh, lyrixx, dunglas, yceruto, wouterj, OskarStark and jderusse as code owners October 3, 2022 23:48

h3xx force-pushed the fix-completion branch from 5ec1459 to 4263602 Compare October 3, 2022 23:49

[Console] Fix completion script affecting $IFS

bd20291

Changing IFS poisons the shell environment. Use COMP_WORDBREAKS instead.

h3xx force-pushed the fix-completion branch 7 times, most recently from 960f815 to fa259f5 Compare October 12, 2022 22:48

GromNaN requested changes Nov 12, 2022

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Nov 12, 2022

[Console] Add unit test for Bash completion

0420a7e

h3xx force-pushed the fix-completion branch from fa259f5 to 0420a7e Compare November 13, 2022 02:14

h3xx mentioned this pull request Nov 29, 2022

Fix completion script affecting $IFS symfony/console#82

Closed

Change assertion type

dd04865

Code review suggestion. Co-authored-by: Jérôme Tamarelle <jerome@tamarelle.net>

nicolas-grekas modified the milestones: 6.1, 6.2 Jan 25, 2023

fabpot modified the milestones: 6.2, 6.3 Jul 30, 2023

nicolas-grekas modified the milestones: 6.3, 6.4 Feb 1, 2024

chalasr closed this Mar 29, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

[Console] Fix completion #47780

[Console] Fix completion #47780

Uh oh!

h3xx commented Oct 3, 2022

Uh oh!

carsonbot commented Oct 3, 2022

Uh oh!

GromNaN commented Oct 4, 2022

Uh oh!

h3xx commented Oct 4, 2022 •

edited

Loading

Uh oh!

h3xx commented Oct 12, 2022

Uh oh!

GromNaN left a comment •

edited

Loading

Uh oh!

Uh oh!

GromNaN Nov 12, 2022

Uh oh!

h3xx Nov 13, 2022

Uh oh!

h3xx Nov 13, 2022

Uh oh!

GromNaN Nov 13, 2022

Uh oh!

Uh oh!

nicolas-grekas commented Dec 13, 2022

Uh oh!

h3xx commented Dec 13, 2022

Uh oh!

nicolas-grekas commented Dec 14, 2022

Uh oh!

chalasr commented Mar 29, 2025

Uh oh!

Uh oh!

Uh oh!

[Console] Fix completion #47780

[Console] Fix completion #47780

Uh oh!

Conversation

h3xx commented Oct 3, 2022

Uh oh!

carsonbot commented Oct 3, 2022

Uh oh!

GromNaN commented Oct 4, 2022

Uh oh!

h3xx commented Oct 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

h3xx commented Oct 12, 2022

Uh oh!

GromNaN left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

GromNaN Nov 12, 2022

Choose a reason for hiding this comment

Uh oh!

h3xx Nov 13, 2022

Choose a reason for hiding this comment

Uh oh!

h3xx Nov 13, 2022

Choose a reason for hiding this comment

Uh oh!

GromNaN Nov 13, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nicolas-grekas commented Dec 13, 2022

Uh oh!

h3xx commented Dec 13, 2022

Uh oh!

nicolas-grekas commented Dec 14, 2022

Uh oh!

chalasr commented Mar 29, 2025

Uh oh!

Uh oh!

h3xx commented Oct 4, 2022 •

edited

Loading

GromNaN left a comment •

edited

Loading