Symfony Console Style tweaks #15964

javiereguiluz · 2015-09-28T11:18:12Z

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	-
License	MIT
Doc PR	-

This PR will contain some minor tweaks found while updating all the Symfony commands. Don't merge yet. Thanks!

javiereguiluz · 2015-09-28T11:30:55Z

The first change is about adding a new method called comment() which is used to display secondary information. This text is prepended with // (like a code comment), which is what the original text() helper displayed. Now the text() helper just displays regular text.

javiereguiluz · 2015-09-28T11:33:18Z

The second change is about fixing the calculation of the underline length. We don't have to take into accoun the style tags length. E.g. <info>ACME</info> is a 4 char length message, not a 16 char length message.

Before:

After:

stof · 2015-09-28T13:11:43Z

src/Symfony/Component/Console/Style/SymfonyStyle.php

@@ -120,10 +120,14 @@ public function title($message)
     */
    public function section($message)
    {
+        // don't take into account the style tags to compute the message length
+        // otherwise, the underline displayed after the section title is too long
+        $messageLength = strlen(preg_replace('/<.*>/U', '', $message));


this is the wrong way to compute it. there is many cases where it will be computed wrong:

<foo>bar<baz> will get a length of 0 here

if your string contains tags which are not registered as applying styles, they will be stripped while they should not (as they are part of the output)

if your string contains escaped tags, they will be stripped too.

We already have the implementation of computed the length of a string without counting the decoration. Please just reuse it instead of trying to reimplement it in a broken way.

Indeed, you can actually use Helper::strlenWithoutDecoration

Done this change and for the title() too. Thanks.

ogizanagi · 2015-09-28T17:56:47Z

Shouldn't the underline length calculation be applied to title too ?

ogizanagi · 2015-09-28T18:07:38Z

src/Symfony/Component/Console/Style/SymfonyStyle.php

+    {
+        $this->autoPrependText();
+
+        if (!is_array($message)) {


I think it could be simplified to something like:

$this->autoPrependText(); $messages = is_array($messages) ? array_values($messages) : array($messages); foreach ($messages as $message) { $this->writeln(sprintf(' // %s', $message)); }

Unless you really need the recursion to do something like:

$sfStyle->comment([ array('lorem', array('ipsum', 'dolor')), 'Sit', 'amet', ])

Which would be kind of confusing and should be reflected to other similar methods.

I like this proposal. If @kbond (the original author of this code) doesn't say anything against it, I'll do these changes. Thanks!

Oh ! Actually I didn't see it was the same implementation as text... It would be a minor BC break again if someone rely on this particular behavior.
Would string[]|string be more accurate in the docblock ?

I don't think the recursion is necessary. Simplify away :)

Changes made. Thanks.

ogizanagi · 2015-09-28T18:09:51Z

src/Symfony/Component/Console/Style/StyleInterface.php

+     *
+     * @param string|array $message
+     */
+    public function comment($message);


Isn't it a BC break ? I think the StyleInterface is already implemented by some third-party libraries. (But I like the comment method over the text one, it makes much more sense !) Anyway it's a really minor one.

Yeah, StyleInterface already was present on 2.7

OK, I've removed the new method from the interface.

fabpot · 2015-09-28T20:01:01Z

Tests are broken

xabbuh · 2015-09-28T20:18:34Z

src/Symfony/Component/Console/Style/SymfonyStyle.php

@@ -110,7 +110,7 @@ public function title($message)
        $this->autoPrependBlock();
        $this->writeln(array(
            sprintf('<comment>%s</>', $message),
-            sprintf('<comment>%s</>', str_repeat('=', strlen($message))),
+            sprintf('<comment>%s</>', str_repeat('=', Helper::strlenWithoutDecoration($message))),


You should pass the return value of $this->getFormatter() as the first argument to Helper::strlenWithoutDecoration() (same below).

Fixed. Thanks.

javiereguiluz · 2015-09-28T21:01:23Z

This PR is no longer WIP. When tests are green again, this can be merged. Thanks.

fabpot · 2015-09-28T21:04:41Z

Still one broken test for me:

There was 1 failure:

1) Symfony\Component\Console\Tests\Style\SymfonyStyleTest::testOutputs with data set #5 ('/Users/fabien/Code/github/sym..._5.php', '/Users/fabien/Code/github/sym..._5.txt')
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
  consectetur adipiscing elit
-
-Lorem ipsum dolor sit amet
- // Lorem ipsum dolor sit amet
- // consectetur adipiscing elit
 '

/Users/fabien/Code/github/symfony/symfony/src/Symfony/Component/Console/Tests/Style/SymfonyStyleTest.php:48

javiereguiluz · 2015-09-28T21:08:07Z

The failing test should be fixed now.

fabpot · 2015-09-28T21:11:58Z

Thank you @javiereguiluz.

…aviereguiluz) This PR was squashed before being merged into the 2.8 branch (closes #15972). Discussion ---------- [Console] Updated the styles of the server commands | Q | A | ------------- | --- | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - This PR uses `comment()` which hasn't been merged yet. WIP PR at #15964 ![server_comparison_1](https://cloud.githubusercontent.com/assets/73419/10139550/a5dc0d70-6603-11e5-8b4c-30cae7f52232.png) ![server_comparison_2](https://cloud.githubusercontent.com/assets/73419/10139552/a82932f6-6603-11e5-9bf5-7d0944a98327.png) Commits ------- 4e2cc0f [Console] Updated the styles of the server commands

This PR was merged into the 2.8 branch. Discussion ---------- Updated the styles of the cache commands | Q | A | ------------- | --- | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - This PR uses comment() which hasn't been merged yet. WIP PR at #15964 ![cache_1](https://cloud.githubusercontent.com/assets/73419/10141826/f41d5952-660e-11e5-8435-b78aef4130bb.png) ![cache_2](https://cloud.githubusercontent.com/assets/73419/10141828/f6da7e40-660e-11e5-80a9-3546e912bc0f.png) ![cache_3](https://cloud.githubusercontent.com/assets/73419/10141831/f9dea92c-660e-11e5-9c1f-3be42a263696.png) Commits ------- 44c5416 Updated the styles for the "cache:warmup" command 08b2959 Updated the style for the "cache:clear" command

Tobion · 2015-09-28T21:37:02Z

src/Symfony/Component/Console/Style/SymfonyStyle.php

+    /**
+     * {@inheritdoc}
+     */
+    public function comment($message)


This method is not defined in the parent class. So inheritdoc makes no sense. Also I guess this method should be part of StyleInterface. But I don't really see the point of this interface either.

I'm very glad that I'm not the only one to wonder about why one insists on having interfaces for everything. Here again, I'm 👍 for removing this interface.

By the way, the interface is useless as we always create concrete instances instead of injecting them.

I can image a user-case: Having a centralized place where the style is created. And all commands would just work with StyleInterface. Thus one can easily replace the style evewhere. But that is not how commands currently work.

I am +1 for removing the interface as well. There was a reason for it that was discussed during the initial PR but I can't find it. Laravel is using this feature but they extend SymfonyStyle.

This PR was squashed before being merged into the 2.8 branch (closes #15980). Discussion ---------- Updated the styles of the config parameters | Q | A | ------------- | --- | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - This PR uses comment() which hasn't been merged yet. WIP PR at #15964 **I have a question**: is it mandatory that the output of this command is a valid YAML or XML file? If not, we can keep the changes in the title of the command. Otherwise, I'll revert those changes. ![config_1](https://cloud.githubusercontent.com/assets/73419/10144197/9018a138-661c-11e5-8c9d-dc1b721533b8.png) ![config_2](https://cloud.githubusercontent.com/assets/73419/10144199/925e5604-661c-11e5-8597-8e01013c2e86.png) Commits ------- fdaa513 Updated the styles of the config parameters

…luz) This PR was squashed before being merged into the 2.8 branch (closes #15983). Discussion ---------- Updated the styles of the container commands | Q | A | ------------- | --- | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - This PR uses comment() which hasn't been merged yet. WIP PR at #15964 This command defines a lot of options, so I don't know if I've updated all the possible outputs: ![container_params](https://cloud.githubusercontent.com/assets/73419/10148240/a4265ed4-6632-11e5-97c6-56d9d5fc496b.png) ![container_tags](https://cloud.githubusercontent.com/assets/73419/10148244/a68f392a-6632-11e5-925d-60eec7d659af.png) ![container_one_service](https://cloud.githubusercontent.com/assets/73419/10148224/8708c7b0-6632-11e5-9a4e-efd3c8206bc1.png) ![container_select_service](https://cloud.githubusercontent.com/assets/73419/10148225/88e88d9a-6632-11e5-969a-57a1f6efc17f.png) ![container_one_parameter](https://cloud.githubusercontent.com/assets/73419/10148227/8c92d342-6632-11e5-9d95-53bb71b6f67d.png) ![container_tagged_services](https://cloud.githubusercontent.com/assets/73419/10148229/9070a002-6632-11e5-8af9-e1d0579d539e.png) Commits ------- d209a4e Updated the styles of the container commands

Added a "comment()" helper to show unimportant text

162d047

carsonbot added the Status: Needs Review label Sep 28, 2015

Fix the calculation of the section underline length

8fc9db7

stof reviewed Sep 28, 2015
View reviewed changes

This was referenced Sep 28, 2015

[Console] Updated the styles of the server commands #15972

Closed

[WCM] Updated the styles of the assets:install command #15975

Closed

Updated the styles of the cache commands #15978

Merged

ogizanagi reviewed Sep 28, 2015
View reviewed changes

javiereguiluz mentioned this pull request Sep 28, 2015

Updated the styles of the config parameters #15980

Closed

ogizanagi reviewed Sep 28, 2015
View reviewed changes

javiereguiluz added 4 commits September 28, 2015 20:14

Calculate the length of the strings using the Helper methods

bbcbddb

Removed recursion

e4cbfae

Fixed test fixtures

9f03c00

Removed the method from the interface

176d25f

xabbuh reviewed Sep 28, 2015
View reviewed changes

javiereguiluz mentioned this pull request Sep 28, 2015

Updated the styles of the container commands #15983

Closed

Fixed tests

48efdd9

javiereguiluz changed the title ~~[WIP] Symfony Console Style tweaks~~ Symfony Console Style tweaks Sep 28, 2015

Fixed a failing test

c65c1d5

fabpot closed this in 5322cbb Sep 28, 2015

Tobion reviewed Sep 28, 2015
View reviewed changes

fabpot mentioned this pull request Nov 16, 2015

Release v2.8.0-BETA1 #16550

Merged

ogizanagi mentioned this pull request May 26, 2016

[Console] StyleInterface: Add missing comment() #18888

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Symfony Console Style tweaks #15964

Symfony Console Style tweaks #15964

javiereguiluz commented Sep 28, 2015

javiereguiluz commented Sep 28, 2015

javiereguiluz commented Sep 28, 2015

stof Sep 28, 2015

ogizanagi Sep 28, 2015

javiereguiluz Sep 28, 2015

ogizanagi commented Sep 28, 2015

ogizanagi Sep 28, 2015

javiereguiluz Sep 28, 2015

ogizanagi Sep 28, 2015

kbond Sep 28, 2015

javiereguiluz Sep 28, 2015

ogizanagi Sep 28, 2015

keradus Sep 28, 2015

javiereguiluz Sep 28, 2015

fabpot commented Sep 28, 2015

xabbuh Sep 28, 2015

javiereguiluz Sep 28, 2015

javiereguiluz commented Sep 28, 2015

fabpot commented Sep 28, 2015

javiereguiluz commented Sep 28, 2015

fabpot commented Sep 28, 2015

Tobion Sep 28, 2015

fabpot Sep 29, 2015

fabpot Sep 29, 2015

Tobion Sep 29, 2015

kbond Sep 29, 2015

Symfony Console Style tweaks #15964

Symfony Console Style tweaks #15964

Conversation

javiereguiluz commented Sep 28, 2015

javiereguiluz commented Sep 28, 2015

javiereguiluz commented Sep 28, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ogizanagi commented Sep 28, 2015

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

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

fabpot commented Sep 28, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javiereguiluz commented Sep 28, 2015

fabpot commented Sep 28, 2015

javiereguiluz commented Sep 28, 2015

fabpot commented Sep 28, 2015

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

Choose a reason for hiding this comment