Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
Escape trailing \ in QuestionHelper autocompletion#24660
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
Escape trailing \ in QuestionHelper autocompletion#24660
Uh oh!
There was an error while loading.Please reload this page.
Conversation
kamazee commentedOct 22, 2017
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. |
kamazee commentedOct 22, 2017
Looks like CI consoles enable formatting. I'll do more cleanup for irrelevant sequences and update PR. |
e5015fa to1e66c9eCompareFixessymfony#24652Trailing backslash, being unescaped, used to escape closing formattingtag and, thus, formatting tag appeared in autocompletion
1e66c9e to0c0f1daComparekamazee commentedOct 22, 2017
Ready for review. |
| $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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
cant we useescape() here? (it includesescapeTrailingBackslash also) Not sure about leaking out this detail =/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
As noted in the PR description, I think this is a BC break asescape 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@ro0NL , yeah.
I'd be glad to create another one for master with justescape 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
unless we qualify that a bugfix for 2.7 ;-) impact should be minimal. More or less relates to escaping exceptions#22142
alsoh1 leaks in as a valid style after calling... perhaps inline it using background/foreground options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I would keep it as is until someone asks for it, escaping trailing backslashes only
nicolas-grekas commentedOct 24, 2017
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 | MITFixes#24652Trailing backslash, being unescaped, used to escape closing formattingtag and, thus, formatting tag appeared in autocompletionOutput of the added test without the fix:```./phpunit --filter testAutocompleteWithTrailingBackslash src/Symfony/Component/Console/Tests/Helper/QuestionHelperTest.php#!/usr/bin/env phpPHPUnit 6.0.13 by Sebastian Bergmann and contributors.Testing Symfony\Component\Console\Tests\Helper\QuestionHelperTestF 1 / 1 (100%)Time: 4.28 seconds, Memory: 6.00MBThere was 1 failure:1) Symfony\Component\Console\Tests\Helper\QuestionHelperTest::testAutocompleteWithTrailingBackslashFailed 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
Uh oh!
There was an error while loading.Please reload this page.
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).