[Console] Fix formatting of SymfonyStyle::comment() #19189
Conversation
|
|
||
| $this->block($messages, null, null, ' // '); | ||
| $this->autoPrependBlock(); | ||
| $this->writeln($this->createBlock($messages, null, null, '<fg=default> // </>')); |
There was a problem hiding this comment.
Should we also set the bg to default?
|
@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! |
|
For me also big 👍 this is really clever! |
| $lines[0] = substr_replace($lines[0], $typePrefix, 0, $indentLength); | ||
| } | ||
|
|
||
| if ($padding && $this->isDecorated()) { |
There was a problem hiding this comment.
Perhaps its better to determine this in Guess keeping it tight to createBlock is ok, as it's probably the expected behaviour 9/10.block(), ie. pass $padding as ($padding && $this->isDecorated()).
|
Btw, i also discussed this with @chalasr a bit in #19172, but are there any downsides to not support/fix this directly in 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. |
7e5ef80 to
03c1c23
Compare
|
@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 |
32ad392 to
336b086
Compare
| @@ -339,6 +339,7 @@ protected function describeContainerDefinition(Definition $definition, array $op | ||
| */ | ||
| protected function describeContainerAlias(Alias $alias, array $options = array()) | ||
| { | ||
| $options['output']->setDecorated(false); |
There was a problem hiding this comment.
This is a UI change for the console. Not sure its intended but i wouldnt change it or just use writeln/block() then.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)
bd85305 to
e06d30b
Compare
|
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. |
| $lines = array_merge($lines, explode(PHP_EOL, wordwrap($message, $this->lineLength - Helper::strlen(strip_tags($prefix)) - $indentLength, PHP_EOL, true))); | ||
|
|
||
| // prefix each line with a number of spaces equivalent to the type length | ||
| if (null !== $type) { |
There was a problem hiding this comment.
@chalasr can this be moved to the foreach below?
There was a problem hiding this comment.
@ro0NL $type will not change so there is no reason to repeat this check for each line. Do I misunderstood your comment?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
- 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
e06d30b to
8efe6fc
Compare
|
@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) { |
8efe6fc to
8cb6f93
Compare
|
@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. |
8cb6f93 to
d1b3467
Compare
Remove decoration from frameworkbundle test (avoid testing the Console behaviour) Set background to default Test output Adapt test for FrameworkBundle Use Helper::strlenWithoutDecoration rather than Helper::strlen(strip_tags(..)) Improve logic for align all lines to the first in block() Tests more block() possible outputs Avoid calling Helper::strlenWithoutDecoration in loop for prefix, assign it instead
d1b3467 to
0a53e1d
Compare
|
Thank you @chalasr. |
This PR was merged into the 2.8 branch. Discussion ---------- [Console] Fix formatting of SymfonyStyle::comment() | 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: ```php $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. Commits ------- 0a53e1d [Console] Fix formatting of SymfonyStyle::comment()
|
I believe this PR causes this issue: Before: After: |
|
That's it, my bad... See #19253 for the fix. |
Prevent future regression
|
Thank you for pointing it out so quickly @kbond |
…lasr) This PR was merged into the 2.8 branch. Discussion ---------- [Console] Fix block() padding formatting after #19189 | Q | A | ------------- | --- | Branch? | 2.8 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #19189 (comment) | License | MIT | Doc PR | reference to the documentation PR, if any This fixes the unformatted padding of `block()` output after #19189. Commits ------- dc130be [Console] Fix for block() padding formatting after #19189
* 2.8: [travis] Fix deps=low/high builds fixed CS skip test with current phpunit bridge Fix for #19183 to add support for new PHP MongoDB extension in sessions. [Console] Fix for block() padding formatting after #19189 [Security][Guard] check if session exist before using it bumped Symfony version to 2.8.9 updated VERSION for 2.8.8 updated CHANGELOG for 2.8.8 bumped Symfony version to 2.7.16 updated VERSION for 2.7.15 update CONTRIBUTORS for 2.7.15 updated CHANGELOG for 2.7.15 Fix some lowest deps Fixed typos in the expectedException annotations Conflicts: CHANGELOG-2.7.md CHANGELOG-3.0.md src/Symfony/Bundle/FrameworkBundle/composer.json src/Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/MongoDbSessionHandlerTest.php src/Symfony/Component/HttpKernel/Kernel.php src/Symfony/Component/HttpKernel/composer.json src/Symfony/Component/Yaml/Tests/ParserTest.php
* 3.0: [travis] Fix deps=low/high builds fixed CS skip test with current phpunit bridge Fix for #19183 to add support for new PHP MongoDB extension in sessions. [Console] Fix for block() padding formatting after #19189 [Security][Guard] check if session exist before using it bumped Symfony version to 3.0.9 updated VERSION for 3.0.8 updated CHANGELOG for 3.0.8 bumped Symfony version to 2.8.9 updated VERSION for 2.8.8 updated CHANGELOG for 2.8.8 bumped Symfony version to 2.7.16 updated VERSION for 2.7.15 update CONTRIBUTORS for 2.7.15 updated CHANGELOG for 2.7.15 Fix some lowest deps Fixed typos in the expectedException annotations Conflicts: src/Symfony/Component/HttpKernel/Kernel.php src/Symfony/Component/Security/Guard/Authenticator/AbstractFormLoginAuthenticator.php
* 3.1: (22 commits) [travis] Fix deps=low/high builds [Form] Fix depreciation triggers fixed CS skip test with current phpunit bridge Fix for #19183 to add support for new PHP MongoDB extension in sessions. [Console] Fix for block() padding formatting after #19189 [Security][Guard] check if session exist before using it bumped Symfony version to 3.1.3 updated VERSION for 3.1.2 updated CHANGELOG for 3.1.2 bumped Symfony version to 3.0.9 updated VERSION for 3.0.8 updated CHANGELOG for 3.0.8 bumped Symfony version to 2.8.9 updated VERSION for 2.8.8 updated CHANGELOG for 2.8.8 bumped Symfony version to 2.7.16 updated VERSION for 2.7.15 update CONTRIBUTORS for 2.7.15 updated CHANGELOG for 2.7.15 ... Conflicts: src/Symfony/Component/HttpKernel/Kernel.php
This:
Before outputs:
After:
This moves the lines-cutting logic from
block()into a specificcreateBlock, used from bothcomment()andblock(), sort ascomment()can take messages containing nested tags and outputs a correctly formatted block without escaping tags.