Color support detectiong change in 6.3.11 introduced a regression

[mxr576]

Symfony version(s) affected

6.3.11

Description

See steps the reproduce.

How to reproduce

I have a custom KISS e2e test script for testing composer audit command outputs.

It looks like the color support detection change that was introduced in 6.3.11 introduced a regression and the output of composer audit --format=json become colorized that makes the JSON invalid.

$audit_output = $argv[1] ?? (stream_get_contents(STDIN) ?: null);
if (null === $audit_output) {
    throw new \LogicException('Missing "composer audit" command output.');
}


fwrite(STDERR, $audit_output);


try {
    $audit_result = json_decode($audit_output, true, flags: JSON_THROW_ON_ERROR);
} catch (JsonException $e) {
    throw new \LogicException(sprintf('Malformed JSON input: "%s". %s', base64_encode(gzdeflate($audit_output, 9)), $e->getMessage()), 0, $e);
}

(Source: https://github.com/mxr576/ddqg-composer-audit/blob/1.0.0-rc4/tests/fixtures/e2e/test.php#L18-L29)

tests/fixtures/e2e/test.php "$(DDQG_COMPOSER_AUDIT_TEST_ENV=1 ./vendor/bin/composer -d tests/fixtures/e2e audit --format=json)"

(Source: https://github.com/mxr576/ddqg-composer-audit/blob/1.0.0-rc4/.github/workflows/run-e2e-test.yml#L51)

Possible Solution

No response

Additional Context

No response

[mxr576]

Running the command with NO_COLOR=1 mitigates the problem on my end.

tests/fixtures/e2e/test.php "$(NO_COLOR=1 DDQG_COMPOSER_AUDIT_TEST_ENV=1 ./vendor/bin/composer -d tests/fixtures/e2e audit --format=json)"

[cs278]

Noticed the same thing upon upgrading to 5.4.34, the output is being decorated even when not attached to a tty.

$ git checkout v5.4.33
HEAD is now at 6548b56ecd Merge pull request #52857 from fabpot/release-5.4.33
$ php -r 'require "vendor/autoload.php"; var_dump((new Symfony\Component\Console\Output\StreamOutput(STDOUT))->isDecorated());' | cat
bool(false)
$ git checkout v5.4.34
Previous HEAD position was 6548b56ecd Merge pull request #52857 from fabpot/release-5.4.33
HEAD is now at e35eb6e132 Merge pull request #53312 from fabpot/release-5.4.34
$ php -r 'require "vendor/autoload.php"; var_dump((new Symfony\Component\Console\Output\StreamOutput(STDOUT))->isDecorated());' | cat
bool(true)

[stof]

The issue with env-based detection is indeed that this applies even when redirecting the output to something else.

I think #52940 should be reverted /cc @theofidry

[theofidry]

@stof what would be the best way to fix? If you only revert, you are adding back the bug fixed too, and that means the bug described in this ticket already exists if you have a Hyper TERM_PROGRAM.

In the case of composer, isn't it Composer's responsibility to disable the decorated output for this format?

Noticed the same thing upon upgrading to 5.4.34, the output is being decorated even when not attached to a tty.

Could be due to an environment variable from the session in which case it is expected?

[cs278]

Could be due to an environment variable from the session in which case it is expected?

Yes it's picking up that I have TERM=xterm-256color set in the environment, but it's not checking the output is actually attached to the terminal.

[mxr576]

In the case of composer, isn't it Composer's responsibility to disable the decorated output for this format?

Hm, indeed, it could handle that.

tests/fixtures/e2e/test.php "$(DDQG_COMPOSER_AUDIT_TEST_ENV=1 ./vendor/bin/composer --no-ansi -d tests/fixtures/e2e audit --format=json)" also works.

--no-ansi is the change

[theofidry]

If this is too much of a change in the ecosystem it might be better to revert it and introduce it in a new version, although technically it is really a fix.

IMO the case of Composer was a "happy accident" that it was working: it outputs a result in a specific format, so disabling output decoration should be done there (and it indicates it is broken in some terminals).

Yes it's picking up that I have TERM=xterm-256color set in the environment, but it's not checking the output is actually attached to the terminal.

If you check #52940 you can see that is how it worked already before, it just failed to capture the xterm-256color in a non Windows environment.

WDYM by attaching the output to the terminal?

[stof]

@theofidry checking whether the output stream is a tty (instead of being a file where it is redirected for instance) is exactly what stream_isatty is doing (on Unix system). Your change means that it ignores this check when there is an env variable that tells about the term.

[cs278]

WDYM by attaching the output to the terminal?

If the output is redirected or piped the output isn't attached to a terminal e.g. bin/console list | grep something or bin/console list > commands.txt.

Basically every command on Linux works by detecting if the output is going directly to a terminal when deciding to apply output formatting, as an example note the first ls output where I've forced colour on which results in some funkiness.

[lyrixx]

For the record, a recent update of symfony/console also break our application (our test suite, don't worry) and it also broke castor's test suite

[theofidry]

Will have a look at it tomorrow

[theofidry]

Based on your feedback it looks to me that stream_isatty() should then be enough on OSX. The question there is why is it necessary for HyperX? (see #27794).

Also note that this should affect the Symfony\Component\VarDumper\Dumper\CliDumper, which also goes further with the ::isWindowsTrueColor() and there is probably a reference too in the PHPUnitBridge.

It also looks like, looking at https://github.com/composer/xdebug-handler/pull/123/files, that there could be an additional fallback or that the case stream_isatty not being defined could be handled.

It's a bit out of scope of the change so I guess for now it would be best to revert this with an explanation. I'll submit a PR during the week