[Console] Console output formatter improvement #45318

fchris82 · 2022-02-04T17:38:40Z

Q	A
Branch?	6.1
Bug fix?	yes
New feature?	yes
Deprecations?	no
Tickets	Fix #30920
License	MIT
Doc PR	symfony/symfony-docs#16476

Current SymfonyStyle can cause some problems:

can cut format tags
cut/split URLs, the user can't click on them in the terminal

I fixed the problem and added or modified some tests. This is a simple, fast, and working solution with a regular expression that won't wrap in the middle of a formatting tag or a link. The last one is optional, see the doc change.

carsonbot · 2022-02-05T13:44:05Z

Hey!

I think @boesing has recently worked with this code. Maybe they can help review this?

Cheers!

Carsonbot

theofidry

A giant 👍 for this. Badly needed!

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

theofidry · 2022-02-05T18:13:17Z

src/Symfony/Component/Console/Helper/OutputWrapper.php

+
+/**
+ * Simple output wrapper for "tagged outputs" instead of wordwrap(). This solution is based on a StackOverflow
+ * answer: https://stackoverflow.com/a/20434776/1476819 from SLN.


I started to do this 2 years ago, SLN was the original name of the user557597.

theofidry · 2022-02-05T18:15:48Z

src/Symfony/Component/Console/Helper/OutputWrapperInterface.php

+
+interface OutputWrapperInterface
+{
+    public const TAG_OPEN_REGEX = '[a-z](?:[^\\\\<>]*+ | \\\\.)*';


Suggested change

public const TAG_OPEN_REGEX = '[a-z](?:[^\\\\<>]*+ | \\\\.)*';

public const TAG_OPEN_REGEX_SEGMENT = '[a-z](?:[^\\\\<>]*+ | \\\\.)*';

? I am not sure it's the best terminology either. I'm just a bit concerned that "regex" here feels wrong as, as a user, I would expect a regex to give a valid regex, not a part of it.

theofidry · 2022-02-05T18:19:13Z

src/Symfony/Component/Console/Helper/OutputWrapper.php

+ */
+class OutputWrapper implements OutputWrapperInterface
+{
+    public const URL_PATTERN = 'https?://\S+';


Might be better to make it private.

theofidry · 2022-02-05T18:20:14Z

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

@@ -453,7 +469,7 @@ private function createBlock(iterable $messages, string $type = null, string $st

        if (null !== $type) {
            $type = sprintf('[%s] ', $type);
-            $indentLength = \strlen($type);
+            $indentLength = Helper::width($type);


It doesn't look directly related to your change no? Asking this in case as it feels like there is a missing test for it and I don't know if your new tests covers it or not

One of my new tests covers it: https://github.com/symfony/symfony/pull/45318/files#diff-6ef90609877201be87f303aa59301e30a147c322dffba8f96f3e27655a5bcf0fR11

theofidry · 2022-02-05T18:20:52Z

src/Symfony/Component/Console/Tests/Fixtures/Style/SymfonyStyle/command/command_13.php

@@ -9,6 +9,6 @@
    $output->setDecorated(true);
    $output = new SymfonyStyle($input, $output);
    $output->comment(
-        'Lorem ipsum dolor sit <comment>amet, consectetur adipisicing elit, 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.</comment> Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum'
+        'Árvíztűrőtükörfúrógép Lorem ipsum dolor sit <comment>amet, consectetur adipisicing elit, 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.</comment> Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum'


maybe a little emoji in the mix? 😄

I added. Good to know, this is a Hungarian 'artificial word' that contains all of the special characters in the language. In free translation: 'floodproof-mirror-driller'. :D

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

theofidry · 2022-02-05T18:24:40Z

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

+ornare</error> efficitur.
+EOS;
+        $result = $wrapper->wrap($text, 20);
+        $this->assertEquals($baseExpected, $result);


might I suggest a provider pattern instead?

/** * @dataProvider textProvider */ public function testBasicWrap(string $text, int $width, bool $allowCutUrls, string $expected): void { $wrapper = new OutputWrapper(); $wrapper->setAllowCutUrls($allowCutUrls); $actual = $wrapper->wrap($text, $width); self::assertSame($expected, $actual); } public static function textProvider(): iterable { yield 'title' => [...]; }

Co-authored-by: Théo FIDRY <theo.fidry@gmail.com>

fchris82 · 2022-02-15T19:38:18Z

@theofidry Hi, could you review and close the fixed issues?

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

theofidry · 2022-02-15T21:19:47Z

For some reasons I cannot mark them as resolved. LGTM though 👍

Co-authored-by: Théo FIDRY <theo.fidry@gmail.com>

fchris82 · 2022-02-15T21:56:10Z

For some reasons I cannot mark them as resolved. LGTM though +1

None of them? 🤔

nicolas-grekas

Here are some random notes :)

.editorconfig

nicolas-grekas · 2022-02-21T14:15:15Z

src/Symfony/Component/Console/CHANGELOG.md

@@ -5,6 +5,9 @@ CHANGELOG
 ---

 * Add method `__toString()` to `InputInterface`
+ * Added `OutputWrapperInterface` and `OutputWrapper` to allow modifying your
+   wrapping strategy in `SymfonyStyle` or in other `OutputStyle`. Eg: you can
+   switch off to wrap URLs.


I'm not convinced we need to open for extensibility here. Fight me if you want to keep it :)

I added this because the behavior you need can depend on your console, your language, and other things. I added a really basic solution here, but people may want different behavior, or they want to split the long URL-s on their way. Eg: originally I just wanted to solve the text wrapping but it turned out, URL wrapping is another issue.

I agree with Nicolas, we should not make it extensible.

nicolas-grekas · 2022-02-21T14:15:57Z

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

@@ -157,7 +158,7 @@ public function formatAndWrap(?string $message, int $width)
            $offset = $pos + \strlen($text);

            // opening tag?
-            if ($open = '/' != $text[1]) {
+            if ($open = ('/' !== $text[1])) {


no need for the brackets

I added it because PHPStorm recommended this change, and I agree, it is more readable what is happening, even if you are totally right, the original version worked. If you want, I can 'undo' this, up to you. But now you know why I did it.

Let's revert as it's not needed.

nicolas-grekas · 2022-02-21T14:16:15Z

src/Symfony/Component/Console/Helper/OutputWrapper.php

+
+    public function wrap(string $text, int $width, string $break = "\n"): string
+    {
+        if (0 === $width) {


if (!$width) {

fabpot · 2022-07-21T12:29:47Z

src/Symfony/Component/Console/CHANGELOG.md

@@ -5,6 +5,9 @@ CHANGELOG
 ---

 * Add method `__toString()` to `InputInterface`
+ * Added `OutputWrapperInterface` and `OutputWrapper` to allow modifying your
+   wrapping strategy in `SymfonyStyle` or in other `OutputStyle`. Eg: you can
+   switch off to wrap URLs.


I agree with Nicolas, we should not make it extensible.

fabpot · 2022-07-21T12:30:10Z

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

@@ -157,7 +158,7 @@ public function formatAndWrap(?string $message, int $width)
            $offset = $pos + \strlen($text);

            // opening tag?
-            if ($open = '/' != $text[1]) {
+            if ($open = ('/' !== $text[1])) {


Let's revert as it's not needed.

fabpot · 2022-07-21T12:30:49Z

src/Symfony/Component/Console/Helper/OutputWrapperInterface.php

+
+namespace Symfony\Component\Console\Helper;
+
+interface OutputWrapperInterface


Let's remove this interface.

fabpot · 2022-07-21T12:31:09Z

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


-    public function __construct(InputInterface $input, OutputInterface $output)
+    public function __construct(InputInterface $input, OutputInterface $output, OutputWrapperInterface $outputWrapper = null)


Let's remove this argument.

If the argument and the setters are removed, the property OutputWrapper::$allowCutUrls cannot be updated and becomes useless.

fabpot · 2022-07-21T12:31:23Z

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

+        $this->outputWrapper = $outputWrapper;
+
+        return $this;
+    }


We can also remove these gettter/setter methods.

GromNaN · 2022-07-26T07:57:58Z

Hello @fchris82, I created a new PR to rebase your work and apply reviews.
Feel free to review #47062

fabpot · 2022-07-26T08:03:41Z

Closing in favor of #47062 then.

… (fchris82, GromNaN) This PR was merged into the 6.2 branch. Discussion ---------- [Console] Don't cut Urls wrapped in SymfonyStyle block | Q | A | ------------- | --- | Branch? | 6.2 | Bug fix? | no | New feature? | yes | Deprecations? | no | Tickets | #30920 | License | MIT | Doc PR | symfony/symfony-docs#16476 Continuation of #45318 by fchris82 Commits ------- b317f06 Remove OutputWrapperInterface 2bc2571 Console output formatter improvement

…utput wrapper (fchris82) This PR was submitted for the 6.1 branch but it was merged into the 6.2 branch instead. Discussion ---------- [Console] Add note about URL cutting of default output wrapper Add note about the new output wrapper function. Related MR: symfony/symfony#45318 Commits ------- 0bfa8ac [Console] Add note about URL cutting of default output wrapper

fchris82 requested a review from chalasr as a code owner February 4, 2022 17:38

carsonbot added Status: Needs Review Bug Console Feature labels Feb 4, 2022

carsonbot added this to the 6.1 milestone Feb 4, 2022

fchris82 mentioned this pull request Feb 4, 2022

[Console] Add note about URL cutting of default output wrapper symfony/symfony-docs#16476

Merged

theofidry reviewed Feb 5, 2022

View reviewed changes

fchris82 force-pushed the console-output-formatter-improvement branch from fc32637 to 525e092 Compare February 12, 2022 20:32

fchris82 and others added 13 commits February 12, 2022 20:32

Editor config: add *.txt because of output test files.

b6f9290

[Console] Add OutputWrapper and fix multibyte characters

d8b9fd2

Add changes to CHANGELOG.md

9c25f32

Standard fixes

3d9eb48

Rename url cutting setting

b67a95e

[Console] Wrapper test fixes

c4a4138

[Console] Code standard fix

9b799dd

[Console] Fix URL pattern in OutputFormatter

e1470fd

Update src/Symfony/Component/Console/Tests/Helper/OutputWrapperTest.php

7b5906d

Co-authored-by: Théo FIDRY <theo.fidry@gmail.com>

Update src/Symfony/Component/Console/Helper/OutputWrapperInterface.php

cd0a65f

Co-authored-by: Théo FIDRY <theo.fidry@gmail.com>

[Console] Review fixes

663330f

[Console] CS fix

4616f5c

[Console] CS fix

8e06868

fchris82 force-pushed the console-output-formatter-improvement branch from 525e092 to 8e06868 Compare February 12, 2022 20:33

theofidry approved these changes Feb 15, 2022

View reviewed changes

src/Symfony/Component/Console/Formatter/OutputFormatter.php Outdated Show resolved Hide resolved

carsonbot added Status: Reviewed and removed Status: Needs Review labels Feb 15, 2022

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

7a1ec1c

Co-authored-by: Théo FIDRY <theo.fidry@gmail.com>

nicolas-grekas reviewed Feb 21, 2022

View reviewed changes

[Console] Requested changes

d6de585

fabpot modified the milestones: 6.1, 6.2 May 20, 2022

fabpot requested changes Jul 21, 2022

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Reviewed labels Jul 21, 2022

GromNaN mentioned this pull request Jul 26, 2022

[Console] Don't cut Urls wrapped in SymfonyStyle block #47062

Merged

fabpot closed this Jul 26, 2022

greg0ire mentioned this pull request Nov 30, 2022

Bump Psalm to 5.0.0 and fix errors for Symfony 6.2 doctrine/orm#10261

Merged

	public const TAG_OPEN_REGEX = '[a-z](?:[^\\\\<>]+ \| \\\\.)';
	public const TAG_OPEN_REGEX_SEGMENT = '[a-z](?:[^\\\\<>]+ \| \\\\.)';


		namespace Symfony\Component\Console\Helper;

		interface OutputWrapperInterface


		public function __construct(InputInterface $input, OutputInterface $output)
		public function __construct(InputInterface $input, OutputInterface $output, OutputWrapperInterface $outputWrapper = null)

Uh oh!

[Console] Console output formatter improvement #45318

[Console] Console output formatter improvement #45318

Uh oh!

Conversation

fchris82 commented Feb 4, 2022

Uh oh!

carsonbot commented Feb 5, 2022

Uh oh!

theofidry left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

theofidry Feb 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fchris82 commented Feb 15, 2022

Uh oh!

Uh oh!

theofidry commented Feb 15, 2022

Uh oh!

fchris82 commented Feb 15, 2022

Uh oh!

nicolas-grekas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GromNaN commented Jul 26, 2022

Uh oh!

fabpot commented Jul 26, 2022

Uh oh!

Uh oh!

theofidry Feb 5, 2022 •

edited

Loading