Escape trailing \ in QuestionHelper autocompletion #24660
Conversation
|
Oops, adding "BC Break" label must be a mistake (that's either me leaving BC Break row with "yes/no" when submitting PR or just mentioning BC break in PR description text). I'd appreciate it being removed. |
|
Looks like CI consoles enable formatting. I'll do more cleanup for irrelevant sequences and update PR. |
e5015fa to
1e66c9e
Compare
Fixes symfony#24652 Trailing backslash, being unescaped, used to escape closing formatting tag and, thus, formatting tag appeared in autocompletion
1e66c9e to
0c0f1da
Compare
|
Ready for review. |
| @@ -303,7 +304,7 @@ private function autocomplete(OutputInterface $output, Question $question, $inpu | |||
| // Save cursor position | |||
| $output->write("\0337"); | |||
| // Write highlighted text | |||
| $output->write('<hl>'.substr($matches[$ofs], $i).'</hl>'); | |||
| $output->write('<hl>'.OutputFormatter::escapeTrailingBackslash(substr($matches[$ofs], $i)).'</hl>'); | |||
There was a problem hiding this comment.
cant we use escape() here? (it includes escapeTrailingBackslash also) Not sure about leaking out this detail =/
There was a problem hiding this comment.
As noted in the PR description, I think this is a BC break as escape would effectively turn off interpreting tags in autocompletion options (they'll be always shown as is); while the impact should not be terrible, this doesn't seem to be the right thing to do for anything but master where it could be acceptable.
There was a problem hiding this comment.
Right!
they'll be always shown as is
Which is in fact the actual value.. no? But 👍 for keeping as is; thus escapeTrailingBackslash only.
There was a problem hiding this comment.
@ro0NL , yeah.
I'd be glad to create another one for master with just escape as it indeed seems to be the right thing in general (interpretation of tags is not guaranteed there anyway, only <hl> accidentally works).
There was a problem hiding this comment.
unless we qualify that a bugfix for 2.7 ;-) impact should be minimal. More or less relates to escaping exceptions #22142
also h1 leaks in as a valid style after calling... perhaps inline it using background/foreground options.
There was a problem hiding this comment.
I would keep it as is until someone asks for it, escaping trailing backslashes only
|
Thank you @kamazee. |
This PR was merged into the 2.7 branch. Discussion ---------- Escape trailing \ in QuestionHelper autocompletion | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #24652 | License | MIT Fixes #24652 Trailing backslash, being unescaped, used to escape closing formatting tag and, thus, formatting tag appeared in autocompletion Output of the added test without the fix: ``` ./phpunit --filter testAutocompleteWithTrailingBackslash src/Symfony/Component/Console/Tests/Helper/QuestionHelperTest.php #!/usr/bin/env php PHPUnit 6.0.13 by Sebastian Bergmann and contributors. Testing Symfony\Component\Console\Tests\Helper\QuestionHelperTest F 1 / 1 (100%) Time: 4.28 seconds, Memory: 6.00MB There was 1 failure: 1) Symfony\Component\Console\Tests\Helper\QuestionHelperTest::testAutocompleteWithTrailingBackslash Failed asserting that two strings are equal. --- Expected +++ Actual @@ @@ -'ExampleNamespace\' +'ExampleNamespace</hl>' ``` `OutputFormatter::escapeTrailingBackslash` is marked as `@internal`; however, this seems to be a valid use without exposing outside of the component. `OutputFormatter::escape`, which is not internal, doesn't fit here because escaping tags in autocompletion options is a deeper topic: there might be a valid use for tags in autocompletion and even if not, taking out the possibility to use them might be considered a BC break (even though it hasn't been advertised anywhere). Commits ------- 0c0f1da Escape trailing \ in QuestionHelper autocompletion
Fixes #24652
Trailing backslash, being unescaped, used to escape closing formatting
tag and, thus, formatting tag appeared in autocompletion
Output of the added test without the fix:
OutputFormatter::escapeTrailingBackslashis marked as@internal; however, this seems to be a valid use without exposing outside of the component.OutputFormatter::escape, which is not internal, doesn't fit here because escaping tags in autocompletion options is a deeper topic: there might be a valid use for tags in autocompletion and even if not, taking out the possibility to use them might be considered a BC break (even though it hasn't been advertised anywhere).