[Console] Fix question formatting using SymfonyStyle::ask() #20970

chalasr · 2016-12-17T11:46:52Z

Q	A
Branch?	2.7
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#20964
License	MIT
Doc PR	n/a

Given

$io = new SymfonyStyle($input, $output);
$io->ask('Do you want to use Foo\\Bar <comment>or</comment> Foo\\Baz\\?', 'Foo\\Bar');

Before output

After output

ogizanagi · 2016-12-17T12:07:13Z

👍

Status: Reviewed

ogizanagi · 2016-12-17T12:19:48Z

src/Symfony/Component/Console/Tests/Helper/SymfonyQuestionHelperTest.php

@@ -101,6 +103,15 @@ public function testAskEscapeLabel()
        $this->assertOutputContains('Do you want a \?', $output);


This test does not longer look relevant, and anyway I think it didn't covered well the initial issue 😕

Changing it for the following:

public function testAskEscapeLabel() { $helper = new SymfonyQuestionHelper(); $helper->setInputStream($this->getInputStream('sure')); $helper->ask($this->createInputInterfaceMock(), $output = $this->createOutputInterface(), new Question('Add a \\')); $this->assertOutputContains('Add a \\', $output); }

will result into the following failure:

1) Symfony\Component\Console\Tests\Helper\SymfonyQuestionHelperTest::testAskEscapeLabel Failed asserting that ' Add a </info>: > ' contains "Add a \".

Looks edgy to have a simple backslash at the end, but it actually was the initial issue. It seems there is no need to escape anything else 😕

The initial issue was about having a backslash as default value which has been fixed, not really about a question ending with a \, which is indeed a very edge case to me. I think this is a good consensus, don't you?

As I said, your fix is legitimate, but there is still another issue elsewhere.

That said, the test mentioned does not actually cover anything I think and should be removed ^^'

Edit: This issue cannot be solved in the output formatter directly, because a \ before a tag already means escaping the tag and is the expected behavior.
If you want to use a trailing slash somewhere, an know it'll be decorated, simply escape it, or use <<:

public function testDecoratedTrailingBackslash() { $formatter = new OutputFormatter(true); - $this->assertSame("\033[32mstring ending with \\\e[39m", $formatter->format('<info>string ending with \</info>')); + $this->assertSame("\033[32mstring ending with \\\e[39m", $formatter->format('<info>string ending with <<</info>')); }

Thanks for looking at it. The useless test has been removed

sstok · 2016-12-17T15:30:54Z

So to be clear, if you have a value with backslashes like a namespace you need to replace these before usage?

ogizanagi · 2016-12-17T15:34:37Z

@sstok: You'll at least need to escape the last character if it's a backslash using OutputFormatter::escape(), or replace it by <<. If backslashes are not at the end (which means right before a </info> tag for instance here), it should not be an issue.

Maybe we can do this check before wrapping any user text into tags, and replace it automatically, but that probably means searching and updating a lot more places.

fabpot · 2016-12-17T17:02:41Z

src/Symfony/Component/Console/Tests/Helper/SymfonyQuestionHelperTest.php

-        $question->setValidator(function ($value) { return $value; });
+        $question->setValidator(function ($value) {
+            return $value;
+        });


should be reverted. The new PHP-CS-Fixer is not configured the right way, sorry about that.

good to know, reverted

ogizanagi · 2016-12-17T20:43:26Z

After all, I think there is only a few places that would require this special check, so it might be worth it and can be fixed within this very same PR IMHO.

@chalasr You can apply 2.7...ogizanagi:fix/2.7/console/trailing_backslashes if you think it makes sense, or I'll submit another PR once this one is merged.

chalasr · 2016-12-18T11:38:28Z

@ogizanagi It makes good sense to me, thank you for the patch, applied.

xabbuh · 2016-12-19T05:48:47Z

src/Symfony/Component/Console/Formatter/OutputFormatter.php

+     *
+     * @return string Escaped text
+     */
+    public static function escapeTrailingBackslash($text)


Shouldn't the method be marked as internal?

Not sure. I guess it can be used by anyone creating it's own helpers in userland.

I agree that it should be marked as internal.

chalasr · 2016-12-22T12:02:26Z

This is ready to go

fabpot · 2016-12-22T12:20:20Z

Thank you @chalasr.

…) (chalasr, ogizanagi) This PR was merged into the 2.7 branch. Discussion ---------- [Console] Fix question formatting using SymfonyStyle::ask() | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #20964 | License | MIT | Doc PR | n/a Given ```php $io = new SymfonyStyle($input, $output); $io->ask('Do you want to use Foo\\Bar <comment>or</comment> Foo\\Baz\\?', 'Foo\\Bar'); ``` Before output ![before](http://image.prntscr.com/image/af3806f866654deda2dec79b50d1ffa2.png) After output ![after](http://image.prntscr.com/image/59c031d9e02949cebeae7a4734c7043f.png) Commits ------- e189183 [Console] SymfonyStyle: Escape trailing backslashes in user texts 9d46712 [Console] Fix question formatting using SymfonyStyle::ask()

carsonbot added Status: Needs Review Console Bug labels Dec 17, 2016

chalasr force-pushed the style/format-question branch from b52d81f to da5d94c Compare December 17, 2016 11:48

chalasr mentioned this pull request Dec 17, 2016

[Console] SymfonyQuestionHelper escapes formatting in question prompts #20964

Closed

chalasr force-pushed the style/format-question branch from da5d94c to 06b0ddd Compare December 17, 2016 12:06

carsonbot added Status: Reviewed and removed Status: Needs Review labels Dec 17, 2016

ogizanagi reviewed Dec 17, 2016

View reviewed changes

[Console] Fix question formatting using SymfonyStyle::ask()

9d46712

chalasr force-pushed the style/format-question branch from 06b0ddd to 9d46712 Compare December 17, 2016 13:05

fabpot reviewed Dec 17, 2016

View reviewed changes

chalasr force-pushed the style/format-question branch from cba6f86 to fd23354 Compare December 18, 2016 11:33

xabbuh reviewed Dec 19, 2016

View reviewed changes

[Console] SymfonyStyle: Escape trailing backslashes in user texts

e189183

chalasr force-pushed the style/format-question branch from fd23354 to e189183 Compare December 19, 2016 10:07

fabpot merged commit e189183 into symfony:2.7 Dec 22, 2016

chalasr deleted the style/format-question branch December 22, 2016 12:51

This was referenced Jan 12, 2017

Release v2.7.23 #21258

Merged

Release v2.8.16 #21259

Merged

Release v3.1.9 #21260

Merged

Release v3.2.2 #21261

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Console] Fix question formatting using SymfonyStyle::ask() #20970

[Console] Fix question formatting using SymfonyStyle::ask() #20970

chalasr commented Dec 17, 2016

ogizanagi commented Dec 17, 2016

ogizanagi Dec 17, 2016 •

edited

Loading

chalasr Dec 17, 2016

ogizanagi Dec 17, 2016 •

edited

Loading

chalasr Dec 17, 2016

sstok commented Dec 17, 2016

ogizanagi commented Dec 17, 2016 •

edited

Loading

fabpot Dec 17, 2016

chalasr Dec 18, 2016

ogizanagi commented Dec 17, 2016 •

edited

Loading

chalasr commented Dec 18, 2016 •

edited

Loading

xabbuh Dec 19, 2016

ogizanagi Dec 19, 2016

fabpot Dec 19, 2016

chalasr Dec 19, 2016

chalasr commented Dec 22, 2016

fabpot commented Dec 22, 2016

		@@ -101,6 +103,15 @@ public function testAskEscapeLabel()
		$this->assertOutputContains('Do you want a \?', $output);

[Console] Fix question formatting using SymfonyStyle::ask() #20970

[Console] Fix question formatting using SymfonyStyle::ask() #20970

Conversation

chalasr commented Dec 17, 2016

ogizanagi commented Dec 17, 2016

ogizanagi Dec 17, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ogizanagi Dec 17, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sstok commented Dec 17, 2016

ogizanagi commented Dec 17, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ogizanagi commented Dec 17, 2016 • edited Loading

chalasr commented Dec 18, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chalasr commented Dec 22, 2016

fabpot commented Dec 22, 2016

ogizanagi Dec 17, 2016 •

edited

Loading

ogizanagi Dec 17, 2016 •

edited

Loading

ogizanagi commented Dec 17, 2016 •

edited

Loading

ogizanagi commented Dec 17, 2016 •

edited

Loading

chalasr commented Dec 18, 2016 •

edited

Loading