Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[Console] Fix formatting of SymfonyStyle::comment()#19189
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| $this->block($messages, null, null, ' // '); | ||
| $this->autoPrependBlock(); | ||
| $this->writeln($this->createBlock($messages, null, null, '<fg=default> // </>')); |
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.
Should we also set thebg todefault?
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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()).
ro0NL commentedJun 26, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 to03c1c23Comparechalasr commentedJun 26, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@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 to336b086Compare| @@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is a UI change for the console. Not sure its intended but i wouldnt change it or just usewriteln/block() then.
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.
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 theSymfonyStyle::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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 toe06d30bCompareI 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@chalasr can this be moved to theforeach below?
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$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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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, iehttps://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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@ro0NL I tried to remove the check here and make it in the last loop e.g:
foreach ($linesas$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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
As far as i can see it c/should be justif ($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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
- Symfony\Component\Console\Tests\Style\SymfonyStyleTest::testOutputs with data setAdd 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
@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.phpindex 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) { |
@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. |
Remove decoration from frameworkbundle test (avoid testing the Console behaviour)Set background to defaultTest outputAdapt test for FrameworkBundleUse Helper::strlenWithoutDecoration rather than Helper::strlen(strip_tags(..))Improve logic for align all lines to the first in block()Tests more block() possible outputsAvoid calling Helper::strlenWithoutDecoration in loop for prefix, assign it instead
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/aThis:```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: |
ro0NL commentedJun 30, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 anyThis 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 annotationsConflicts:CHANGELOG-2.7.mdCHANGELOG-3.0.mdsrc/Symfony/Bundle/FrameworkBundle/composer.jsonsrc/Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/MongoDbSessionHandlerTest.phpsrc/Symfony/Component/HttpKernel/Kernel.phpsrc/Symfony/Component/HttpKernel/composer.jsonsrc/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 annotationsConflicts:src/Symfony/Component/HttpKernel/Kernel.phpsrc/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.