Skip to content
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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: using multiple CLI::color() in CLI::write() outputs strings with wrong color #5893

Merged
merged 10 commits into from
Apr 18, 2022
Merged

fix: using multiple CLI::color() in CLI::write() outputs strings with wrong color #5893

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
61999b0
fix: multiple CLI::color() inside CLI::write() change color of string…
kenjis Apr 13, 2022
f406f4a
refactor: vendor/bin/rector process
kenjis Apr 13, 2022
8995da9
test: add line breaks
kenjis Apr 13, 2022
1b26129
test: add test
kenjis Apr 13, 2022
ae56d6c
docs: remove out of dated comment
kenjis Apr 17, 2022
6320733
refactor: combine if statements
kenjis Apr 17, 2022
9dcb39c
refacter: remove unneeded parentheses
kenjis Apr 17, 2022
2aa9348
docs: fix heading underlines
kenjis Apr 17, 2022
8567043
docs: change method names to headings
kenjis Apr 17, 2022
c0ca99f
docs: add changelog
kenjis Apr 17, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 48 additions & 25 deletions system/CLI/CLI.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -473,6 +473,10 @@ public static function color(string $text, string $foreground, ?string $backgrou
return $text;
}

if ($text === '') {
return $text;
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can combine this with the first if?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

if (! array_key_exists($foreground, static::$foreground_colors)) {
throw CLIException::forInvalidColor('foreground', $foreground);
}
Expand All @@ -481,6 +485,49 @@ public static function color(string $text, string $foreground, ?string $backgrou
throw CLIException::forInvalidColor('background', $background);
}

// Reset text
$newText = '';

// Detect if color method was already in use with this text
if (strpos($text, "\033[0m") !== false) {
$pattern = '/\\033\\[0;.+?\\033\\[0m/u';

preg_match_all($pattern, $text, $matches);
$coloredStrings = $matches[0];

// No colored string found. Invalid strings with no `\033[0;??`.
if ($coloredStrings === []) {
return $newText . self::getColoredText($text, $foreground, $background, $format);
}

$nonColoredText = preg_replace(
$pattern,
'<<__colored_string__>>',
$text
);
$nonColoredChunks = preg_split(
'/<<__colored_string__>>/u',
$nonColoredText
);

foreach ($nonColoredChunks as $i => $chunk) {
if ($chunk !== '') {
$newText .= self::getColoredText($chunk, $foreground, $background, $format);
}

if (isset($coloredStrings[$i])) {
$newText .= $coloredStrings[$i];
}
}
} else {
$newText .= self::getColoredText($text, $foreground, $background, $format);
}

return $newText;
}

private static function getColoredText(string $text, string $foreground, ?string $background, ?string $format): string
{
$string = "\033[" . static::$foreground_colors[$foreground] . 'm';

if ($background !== null) {
Expand All @@ -491,31 +538,7 @@ public static function color(string $text, string $foreground, ?string $backgrou
$string .= "\033[4m";
}

// Detect if color method was already in use with this text
if (strpos($text, "\033[0m") !== false) {
// Split the text into parts so that we can see
// if any part missing the color definition
$chunks = mb_split('\\033\\[0m', $text);
// Reset text
$text = '';

foreach ($chunks as $chunk) {
if ($chunk === '') {
continue;
}

// If chunk doesn't have colors defined we need to add them
if (strpos($chunk, "\033[") === false) {
$chunk = static::color($chunk, $foreground, $background, $format);
// Add color reset before chunk and clear end of the string
$text .= rtrim("\033[0m" . $chunk, "\033[0m");
} else {
$text .= $chunk;
}
}
}

return $string . $text . "\033[0m";
return $string . ($text . "\033[0m");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the need for parenthesis?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems rector added.
Removed.

}

/**
Expand Down
81 changes: 70 additions & 11 deletions tests/system/CLI/CLITest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,18 +39,21 @@ protected function tearDown(): void
public function testNew()
{
$actual = new CLI();

$this->assertInstanceOf(CLI::class, $actual);
}

public function testBeep()
{
$this->expectOutputString("\x07");

CLI::beep();
}

public function testBeep4()
{
$this->expectOutputString("\x07\x07\x07\x07");

CLI::beep(4);
}

Expand Down Expand Up @@ -96,6 +99,7 @@ public function testIsWindows()
public function testNewLine()
{
$this->expectOutputString('');

CLI::newLine();
}

Expand All @@ -119,19 +123,21 @@ public function testColorSupportOnNoColor()
{
$nocolor = getenv('NO_COLOR');
putenv('NO_COLOR=1');

CLI::init(); // force re-check on env

$this->assertSame('test', CLI::color('test', 'white', 'green'));

putenv($nocolor ? "NO_COLOR={$nocolor}" : 'NO_COLOR');
}

public function testColorSupportOnHyperTerminals()
{
$termProgram = getenv('TERM_PROGRAM');
putenv('TERM_PROGRAM=Hyper');

CLI::init(); // force re-check on env

$this->assertSame("\033[1;37m\033[42m\033[4mtest\033[0m", CLI::color('test', 'white', 'green', 'underline'));

putenv($termProgram ? "TERM_PROGRAM={$termProgram}" : 'TERM_PROGRAM');
}

Expand All @@ -146,72 +152,105 @@ public function testColor()
// After the tests on NO_COLOR and TERM_PROGRAM above,
// the $isColored variable is rigged. So we reset this.
CLI::init();
$this->assertSame("\033[1;37m\033[42m\033[4mtest\033[0m", CLI::color('test', 'white', 'green', 'underline'));

$this->assertSame(
"\033[1;37m\033[42m\033[4mtest\033[0m",
CLI::color('test', 'white', 'green', 'underline')
);
}

public function testColorEmtpyString()
{
$this->assertSame(
'',
CLI::color('', 'white', 'green', 'underline')
);
}

public function testPrint()
{
CLI::print('test');
$expected = 'test';

$expected = 'test';
$this->assertSame($expected, CITestStreamFilter::$buffer);
}

public function testPrintForeground()
{
CLI::print('test', 'red');
$expected = "\033[0;31mtest\033[0m";

$expected = "\033[0;31mtest\033[0m";
$this->assertSame($expected, CITestStreamFilter::$buffer);
}

public function testPrintBackground()
{
CLI::print('test', 'red', 'green');
$expected = "\033[0;31m\033[42mtest\033[0m";

$expected = "\033[0;31m\033[42mtest\033[0m";
$this->assertSame($expected, CITestStreamFilter::$buffer);
}

public function testWrite()
{
CLI::write('test');

$expected = PHP_EOL . 'test' . PHP_EOL;
$this->assertSame($expected, CITestStreamFilter::$buffer);
}

public function testWriteForeground()
{
CLI::write('test', 'red');

$expected = "\033[0;31mtest\033[0m" . PHP_EOL;
$this->assertSame($expected, CITestStreamFilter::$buffer);
}

public function testWriteForegroundWithColorBefore()
{
CLI::write(CLI::color('green', 'green') . ' red', 'red');
$expected = "\033[0;31m\033[0;32mgreen\033[0m\033[0;31m red\033[0m" . PHP_EOL;

$expected = "\033[0;32mgreen\033[0m\033[0;31m red\033[0m" . PHP_EOL;
$this->assertSame($expected, CITestStreamFilter::$buffer);
}

public function testWriteForegroundWithColorAfter()
{
CLI::write('red ' . CLI::color('green', 'green'), 'red');
$expected = "\033[0;31mred \033[0;32mgreen\033[0m" . PHP_EOL;

$expected = "\033[0;31mred \033[0m\033[0;32mgreen\033[0m" . PHP_EOL;
$this->assertSame($expected, CITestStreamFilter::$buffer);
}

/**
* @see https://github.com/codeigniter4/CodeIgniter4/issues/5892
*/
public function testWriteForegroundWithColorTwice()
{
CLI::write(
CLI::color('green', 'green') . ' red ' . CLI::color('green', 'green'),
'red'
);

$expected = "\033[0;32mgreen\033[0m\033[0;31m red \033[0m\033[0;32mgreen\033[0m" . PHP_EOL;
$this->assertSame($expected, CITestStreamFilter::$buffer);
}

public function testWriteBackground()
{
CLI::write('test', 'red', 'green');

$expected = "\033[0;31m\033[42mtest\033[0m" . PHP_EOL;
$this->assertSame($expected, CITestStreamFilter::$buffer);
}

public function testError()
{
$this->stream_filter = stream_filter_append(STDERR, 'CITestStreamFilter');

CLI::error('test');

// red expected cuz stderr
$expected = "\033[1;31mtest\033[0m" . PHP_EOL;
$this->assertSame($expected, CITestStreamFilter::$buffer);
Expand All @@ -220,15 +259,19 @@ public function testError()
public function testErrorForeground()
{
$this->stream_filter = stream_filter_append(STDERR, 'CITestStreamFilter');

CLI::error('test', 'purple');

$expected = "\033[0;35mtest\033[0m" . PHP_EOL;
$this->assertSame($expected, CITestStreamFilter::$buffer);
}

public function testErrorBackground()
{
$this->stream_filter = stream_filter_append(STDERR, 'CITestStreamFilter');

CLI::error('test', 'purple', 'green');

$expected = "\033[0;35m\033[42mtest\033[0m" . PHP_EOL;
$this->assertSame($expected, CITestStreamFilter::$buffer);
}
Expand Down Expand Up @@ -273,9 +316,18 @@ public function testShowProgressWithoutBar()
public function testWrap()
{
$this->assertSame('', CLI::wrap(''));
$this->assertSame('1234' . PHP_EOL . ' 5678' . PHP_EOL . ' 90' . PHP_EOL . ' abc' . PHP_EOL . ' de' . PHP_EOL . ' fghij' . PHP_EOL . ' 0987654321', CLI::wrap('1234 5678 90' . PHP_EOL . 'abc de fghij' . PHP_EOL . '0987654321', 5, 1));
$this->assertSame('1234 5678 90' . PHP_EOL . ' abc de fghij' . PHP_EOL . ' 0987654321', CLI::wrap('1234 5678 90' . PHP_EOL . 'abc de fghij' . PHP_EOL . '0987654321', 999, 2));
$this->assertSame('1234 5678 90' . PHP_EOL . 'abc de fghij' . PHP_EOL . '0987654321', CLI::wrap('1234 5678 90' . PHP_EOL . 'abc de fghij' . PHP_EOL . '0987654321'));
$this->assertSame(
'1234' . PHP_EOL . ' 5678' . PHP_EOL . ' 90' . PHP_EOL . ' abc' . PHP_EOL . ' de' . PHP_EOL . ' fghij' . PHP_EOL . ' 0987654321',
CLI::wrap('1234 5678 90' . PHP_EOL . 'abc de fghij' . PHP_EOL . '0987654321', 5, 1)
);
$this->assertSame(
'1234 5678 90' . PHP_EOL . ' abc de fghij' . PHP_EOL . ' 0987654321',
CLI::wrap('1234 5678 90' . PHP_EOL . 'abc de fghij' . PHP_EOL . '0987654321', 999, 2)
);
$this->assertSame(
'1234 5678 90' . PHP_EOL . 'abc de fghij' . PHP_EOL . '0987654321',
CLI::wrap('1234 5678 90' . PHP_EOL . 'abc de fghij' . PHP_EOL . '0987654321')
);
}

public function testParseCommand()
Expand All @@ -287,6 +339,7 @@ public function testParseCommand()
];
$_SERVER['argc'] = 3;
CLI::init();

$this->assertNull(CLI::getSegment(3));
$this->assertSame('b', CLI::getSegment(1));
$this->assertSame('c', CLI::getSegment(2));
Expand All @@ -312,6 +365,7 @@ public function testParseCommandMixed()
'sure',
];
CLI::init();

$this->assertNull(CLI::getSegment(7));
$this->assertSame('b', CLI::getSegment(1));
$this->assertSame('c', CLI::getSegment(2));
Expand All @@ -335,6 +389,7 @@ public function testParseCommandOption()
'd',
];
CLI::init();

$this->assertSame(['parm' => 'pvalue'], CLI::getOptions());
$this->assertSame('pvalue', CLI::getOption('parm'));
$this->assertSame('-parm pvalue ', CLI::getOptionString());
Expand All @@ -359,6 +414,7 @@ public function testParseCommandMultipleOptions()
'value 3',
];
CLI::init();

$this->assertSame(['parm' => 'pvalue', 'p2' => null, 'p3' => 'value 3'], CLI::getOptions());
$this->assertSame('pvalue', CLI::getOption('parm'));
$this->assertSame('-parm pvalue -p2 -p3 "value 3" ', CLI::getOptionString());
Expand All @@ -373,11 +429,13 @@ public function testWindow()
$height = new ReflectionProperty(CLI::class, 'height');
$height->setAccessible(true);
$height->setValue(null);

$this->assertIsInt(CLI::getHeight());

$width = new ReflectionProperty(CLI::class, 'width');
$width->setAccessible(true);
$width->setValue(null);

$this->assertIsInt(CLI::getWidth());
}

Expand All @@ -391,6 +449,7 @@ public function testWindow()
public function testTable($tbody, $thead, $expected)
{
CLI::table($tbody, $thead);

$this->assertSame(CITestStreamFilter::$buffer, $expected);
}

Expand Down