Terminal Color Mode refactoring: Adding a way to force color mode by the dev. user (with a new method), fewergetenv() calls (cache value), simpler tests (use the a new method and a new constant).

For example, it can be useful in an environment where none of the expected environment variables are available (Docker container...) , but where the support of a specific mode is imperative.

A future evolution could be to add an optional default option to all commands to force a particular mode by the final user.

julien-boudry requested a review fromchalasr as acode owner

August 27, 2022 11:22

carsonbot added Status: Needs Review Console Feature labels

Aug 27, 2022

carsonbot added this to the6.2 milestone

Aug 27, 2022

julien-boudry force-pushed thearcenciel2 branch from3065dbf to6b86489Compare

August 27, 2022 11:26

stof reviewed

Aug 27, 2022

View reviewed changes

src/Symfony/Component/Console/Terminal.php OutdatedShow resolvedHide resolved

julien-boudry force-pushed thearcenciel2 branch 2 times, most recently frome851fd9 tod430f59Compare

August 27, 2022 14:17

Copy link

ContributorAuthor

julien-boudry commentedAug 27, 2022•
edited
Loading

Do you prefer as actually:

Terminal::getTermColorMode();Terminal::setTermColorMode();(private) Terminal::$termColorMode;

Or?

Terminal::getColorMode();Terminal::setColorMode();(private) Terminal::$colorMode;

Copy link

Contributor

alamirault commentedAug 29, 2022

Do you prefer as actually:

Terminal::getTermColorMode();Terminal::setTermColorMode();(private) Terminal::$termColorMode;

Or?

Terminal::getColorMode();Terminal::setColorMode();(private) Terminal::$colorMode;

I preferTerminal::getColorMode();

[Console] Terminal Color Mode refactoring: Adding a way to force colo…

4cb8384

…r mode by user, fewer getenv() calls, simpler tests.It can be useful for example in the case of an environment where none of the expected environment variables are available (Docker container...) , but where the support of a specific mode is imperative.

julien-boudry force-pushed thearcenciel2 branch fromd430f59 to4cb8384Compare

September 7, 2022 11:47

Copy link

ContributorAuthor

julien-boudry commentedSep 7, 2022

I agree. I just push a new version with just theses naming changes.
For me, it's ready to merge.

stof approved these changes

Sep 7, 2022

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels

Sep 7, 2022

Copy link

ContributorAuthor

julien-boudry commentedSep 7, 2022•
edited
Loading

About Psalm, which triggers some erros here. They are all false positives, because it is in fact impossible. The case is handled upstream in the getColorMode method, but Psalm is not smart enough for that.

There is - I think - only one way to deal with it:

Add a static method like resetColorMode(): void {self::$colorMode = null;}
Disallow null values as arguments toTerminal::setColorMode(AnsiColorMode $colorMode) instead ofTerminal::setColorMode(?AnsiColorMode $colorMode).

However, this is an unnecessary burden on the public API to please psalm, when the code looks clean and logical to me.

Original Psalm log, actually:

Error: src/Symfony/Component/Console/Terminal.php:29:44: InvalidNullableReturnType: The declaredreturntype'Symfony\Component\Console\Output\AnsiColorMode'for Symfony\Component\Console\Terminal::getColorMode is not nullable, but'Symfony\Component\Console\Output\AnsiColorMode|null' contains null (see https://psalm.dev/144)Error: src/Symfony/Component/Console/Terminal.php:43:24: NullableReturnStatement: The declaredreturntype'Symfony\Component\Console\Output\AnsiColorMode'for Symfony\Component\Console\Terminal::getColorMode is not nullable, but thefunctionreturns'null' (see https://psalm.dev/139)Error: src/Symfony/Component/Console/Terminal.php:49:24: NullableReturnStatement: The declaredreturntype'Symfony\Component\Console\Output\AnsiColorMode'for Symfony\Component\Console\Terminal::getColorMode is not nullable, but thefunctionreturns'null' (see https://psalm.dev/139)Error: src/Symfony/Component/Console/Terminal.php:60:24: NullableReturnStatement: The declaredreturntype'Symfony\Component\Console\Output\AnsiColorMode'for Symfony\Component\Console\Terminal::getColorMode is not nullable, but thefunctionreturns'null' (see https://psalm.dev/139)Error: src/Symfony/Component/Console/Terminal.php:66:24: NullableReturnStatement: The declaredreturntype'Symfony\Component\Console\Output\AnsiColorMode'for Symfony\Component\Console\Terminal::getColorMode is not nullable, but thefunctionreturns'null' (see https://psalm.dev/139)Error: src/Symfony/Component/Console/Terminal.php:72:16: NullableReturnStatement: The declaredreturntype'Symfony\Component\Console\Output\AnsiColorMode'for Symfony\Component\Console\Terminal::getColorMode is not nullable, but thefunctionreturns'null' (see https://psalm.dev/139)Error: Process completed withexit code 2.