[Console] Fix formatting of SymfonyStyle::comment()

[chalasr]

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

This:

$io->comment('Lorem ipsum dolor sit amet, consectetur adipisicing elit, <comment>sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat </comment>cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum.');

Before outputs:

After:

This moves the lines-cutting logic from block() into a specific createBlock, used from both comment() and block(), sort as comment() can take messages containing nested tags and outputs a correctly formatted block without escaping tags.

[javiereguiluz]

Should we also set the bg to default?

[javiereguiluz]

@chalasr this is really nice! I was afraid that the fix for this would be more complicated.

My only request would be to add some tests for this edge case. Thanks!

[ro0NL]

For me also big 👍 this is really clever!

[ro0NL]

Perhaps its better to determine this in block(), ie. pass $padding as ($padding && $this->isDecorated()). Guess keeping it tight to createBlock is ok, as it's probably the expected behaviour 9/10.

[ro0NL]

Btw, i also discussed this with @chalasr a bit in #19172, but are there any downsides to not support/fix this directly in block()? Ie. why not allow users to mix&match inline styles with the outer $style.

Currently if the user passes a already formatted string to block (like comment did previous) it still can fail from a cutting pov where formatting works just fine.

[chalasr]

@javiereguiluz Sure, I'll set the background to normal and add some tests.

@ro0NL I would like to open a specific discussion about allowing or not nested formatting in block() in order to don't talk about multiple subjects here and keep things clear, wdyt? Then open a PR for, depending on the result of the discussion.

[ro0NL]

This is a UI change for the console. Not sure its intended but i wouldnt change it or just use writeln/block() then.

[chalasr]

This test is part of the FrameworkBundle TextDescriptor that is not responsible of the console output style, so IMO we should not test the output style of a comment here, I written a test for that on the corresponding class (SymfonyStyle). Plus, this test needs to be adapted each time we made a change on the SymfonyStyle::comment() method, that is first confusing because not in the same component.
Not intendeed! The goal is to stop decorated output on the corresponding unit test

[ro0NL]

Thing is, this changes the state of the output for subsequent descriptions. Ie currently its expected to be always decorated. (https://github.com/chalasr/symfony/blob/bd8530572154cbab67a239e597501519d37fc588/src/Symfony/Bundle/FrameworkBundle/Tests/Fixtures/Descriptor/array_parameter.txt)

[chalasr]

I made the change, added a test and adapted the FrameworkBundle ones when needed.

My guess is that the 2 failed travis builds are due to that the console dependency of the FrameworkBundle doesn't contain the fix provided by this PR and so cannot pass because the expected output is the old one (I looked at the output of the failed assertion, it is the old one). After merge on the branches corresponding to the required versions, it should pass as for appveyor, other php versions on travis, and my local.

[ro0NL]

@chalasr can this be moved to the foreach below?

[chalasr]

@ro0NL $type will not change so there is no reason to repeat this check for each line. Do I misunderstood your comment?

[ro0NL]

If $type is not null, $lines will be foreached within the outer foreach ($messages), ie. the same lines are iterated multiple times (hence the check to add $lineIndentation if needed). Seems to make sense to prepare $lines in the second foreach once, ie https://github.com/chalasr/symfony/blob/e06d30b229169945ba340e7998d0938a5d99d478/src/Symfony/Component/Console/Style/SymfonyStyle.php#L437

To clearify,

foreach messages {
  modify lines
  foreach lines {}
}

foreach lines {}

The first foreach on $lines seems inefficient. However at first sight, im not sure how and if this affects

if (count($messages) > 1 && $key < count($messages) - 1) {
    $lines[] = '';
}

somehow.

[chalasr]

I understand now. I'm not sure, but I remember that what is done in this check really needs to be done at this point, when iterating over $messages (first loop). I'll give it a try to see if I can bring the two in the last loop ($lines). Thank's for pointing it out @ro0NL

[chalasr]

@ro0NL I tried to remove the check here and make it in the last loop e.g:

foreach ($lines as $i => &$line) {
    if ('' !== $line && 0 !== $i && null !== $type) {
        $line = $lineIndentation === substr($line, 0, $indentLength) ? $line : $lineIndentation.$line;
    }
    $line = sprintf('%s%s', $prefix, $line);
    // ...
}

unfortunately it breaks the line format.

[ro0NL]

As far as i can see it c/should be just
if ($type !== null) { $line = $lineIndentation.$line; }

I.e. there's no need for $lineIndentation === substr($line, 0, $indentLength) ? $line, as we'er iterating all lines once now. But maybe im overlooking something ;-) i'll try to play around with it tonight, but i can only imagine its breaking indeed.

[chalasr]

7. Symfony\Component\Console\Tests\Style\SymfonyStyleTest::testOutputs with data set Add a "routing.generator.base.class" to the routing service definition #13 ('/Volumes/HD/Sites/perso/contr..._9.php', '/Volumes/HD/Sites/perso/contr..._9.txt')
   Failed asserting that two strings are equal.
   --- Expected
   +++ Actual
   @@ @@
   '
   -X [CUSTOM] Custom block
   +X [CUSTOM] ock
   X
   X Second custom block line

Not all lines need to be processed (the reason of $lineIndentation === substr($line, 0, $indentLength)).

ATM this code part (related to the correct alignment/prefixing of blocks) looks good enough to me as it works as expected (get it stable was not so easy, see #18879). I wouldn't like to break the existing while trying to slightly optimize it "btw" in this PR.

I let you try by yourself, please keep me informed

[ro0NL]

@chalasr can you try this? Makes sense to me.. =/

diff --git a/src/Symfony/Component/Console/Style/SymfonyStyle.php b/src/Symfony/Component/Console/Style/SymfonyStyle.php
index de03aa8..abc5df5 100644
--- a/src/Symfony/Component/Console/Style/SymfonyStyle.php
+++ b/src/Symfony/Component/Console/Style/SymfonyStyle.php
@@ -406,29 +406,25 @@ class SymfonyStyle extends OutputStyle

             $lines = array_merge($lines, explode(PHP_EOL, wordwrap($message, $this->lineLength - Helper::strlenWithoutDecoration($this->getFormatter(), $prefix) - $indentLength, PHP_EOL, true)));

-            // prefix each line with a number of spaces equivalent to the type length
-            if (null !== $type) {
-                foreach ($lines as &$line) {
-                    $line = $lineIndentation === substr($line, 0, $indentLength) ? $line : $lineIndentation.$line;
-                }
-            }
-
             if (count($messages) > 1 && $key < count($messages) - 1) {
                 $lines[] = '';
             }
         }

-        if (null !== $type) {
-            $lines[0] = substr_replace($lines[0], $typePrefix, 0, $indentLength);
-        }
-
+        $firstLineKey = 0;
         if ($padding && $this->isDecorated()) {
             array_unshift($lines, '');
             $lines[] = '';
+            $firstLineKey = 1;
         }

-        foreach ($lines as &$line) {
-            $line = sprintf('%s%s', $prefix, $line);
+        foreach ($lines as $key => &$line) {
+            if (null !== $type) {
+                // prefix first line with the type, otherwise a number of spaces equivalent to the type length
+                $line = $firstLineKey === $key ? $prefix.$typePrefix.$line : $prefix.$lineIndentation.$line;
+            } else {
+                $line = $prefix.$line;
+            }
             $line .= str_repeat(' ', $this->lineLength - Helper::strlenWithoutDecoration($this->getFormatter(), $line));

             if ($style) {

[chalasr]

@ro0NL I took inspiration from your diff, one loop avoided! Thank you.

I added some additional tests to prevent regressions due to the changes added here.
Tests pass.
(The failure on appveyor is unrelated, the travis build fails because the FrameworkBundle's Console dependency isn't in sync with this PR).

[fabpot]

Thank you @chalasr.

[kbond]

I believe this PR causes this issue:

Before:

After:

[ro0NL]

@chalasr i guess i can confirm.. this should be moved back up and check for the "first" line being index 0 or 1.

Oops?

[chalasr]

That's it, my bad...

See #19253 for the fix.

[chalasr]

Thank you for pointing it out so quickly @kbond