Colors::length returns incorrect lengths for colorized strings

[tollmanz]

When you pass a string to Colors::length that has already been colorized with Colors::colorized, the wrong result is given. This issue resulted in incorrectly formatted tables using \cli\Table::display().

To reproduce:

1. Add the following WP CLI command:

   <?php
   
   class ReducedTestCase extends WP_CLI_Command {
     function test() {
         $success = \cli\Colors::colorize( "%Gx%n" );
   
         // Display results
         $table = new \cli\Table();
         $table->setHeaders( array( 'Test Header' ) );
         $table->setRows( array( array( $success )) );
         $table->display();
     }
   }
   
   WP_CLI::add_command( 'failwhale', 'ReducedTestCase' );

2. The output produced is:

   vagrant@vvv:/srv/www/wordpress-develop/src$ wp failwhale test
   +--------------+
   | Test Header  |
   +--------------+
   | x |
   +--------------+

   Note that the output is colorized even though I cannot indicate that in the code above.

3. The expected output is:

   vagrant@vvv:/srv/www/wordpress-develop/src$ wp failwhale test
   +--------------+
   | Test Header  |
   +--------------+
   | x            |
   +--------------+

This problem is occurring because before the content is passed to the table methods, it is colorized. Colorizing the string turns it from:

%Gx%n to something like \033[32;1mx\033[0m

The string length counts are thusly messed up. In fact, I'd argue that the string lengths, for any version of colorized values have always been messed up. The first value above would return a length of 5 and the second a value of 18 when really, the string length should be 1 as the only non-color value is x.

I was able to implement a really sloppy proof of concept fix for the issue:

static public function length($string) {
    // Convert the string back to it's original state
    $conversions = array(
        '%y' => array('color' => 'yellow'),
        '%g' => array('color' => 'green'),
        '%b' => array('color' => 'blue'),
        '%r' => array('color' => 'red'),
        '%p' => array('color' => 'magenta'),
        '%m' => array('color' => 'magenta'),
        '%c' => array('color' => 'cyan'),
        '%w' => array('color' => 'grey'),
        '%k' => array('color' => 'black'),
        '%n' => array('color' => 'reset'),
        '%Y' => array('color' => 'yellow', 'style' => 'bright'),
        '%G' => array('color' => 'green', 'style' => 'bright'),
        '%B' => array('color' => 'blue', 'style' => 'bright'),
        '%R' => array('color' => 'red', 'style' => 'bright'),
        '%P' => array('color' => 'magenta', 'style' => 'bright'),
        '%M' => array('color' => 'magenta', 'style' => 'bright'),
        '%C' => array('color' => 'cyan', 'style' => 'bright'),
        '%W' => array('color' => 'grey', 'style' => 'bright'),
        '%K' => array('color' => 'black', 'style' => 'bright'),
        '%N' => array('color' => 'reset', 'style' => 'bright'),
        '%3' => array('background' => 'yellow'),
        '%2' => array('background' => 'green'),
        '%4' => array('background' => 'blue'),
        '%1' => array('background' => 'red'),
        '%5' => array('background' => 'magenta'),
        '%6' => array('background' => 'cyan'),
        '%7' => array('background' => 'grey'),
        '%0' => array('background' => 'black'),
        '%F' => array('style' => 'blink'),
        '%U' => array('style' => 'underline'),
        '%8' => array('style' => 'inverse'),
        '%9' => array('style' => 'bright'),
        '%_' => array('style' => 'bright')
    );

    foreach ($conversions as $key => $value) {
        $string = str_replace(self::color($value), '', self::colorize($string, true));
    }

    return safe_strlen(self::colorize($string, true));
}

As a proof of concept, this simply converts the colorized string to the original string without any color information. This gets the true string length value. Furthermore, this gives the expected results in the table display. This is a sloppy fix and it produced noticeable performance issues.

I think there are a lot of different approaches to fixing this issue. An approach that I think could work would be too:

1. Whenever a new value is passed to Colors::colorize, store that value in memory and specifically track:
   • String without any color information
   • The original string value
   • The colorized value
2. Change the Colors::length method to do a simple strlen call on the string without any color information.

I think this method would be backwards compatible, and provide a nice performance boost by removing redundant color conversions.

Additionally, I think some unit testing should be added to show how broken these methods are.

[danielbachhuber]

Thanks Zack. This is very similar to #53.

I'm reasonably confident Colors::length() should only ever be used to calculate the width of output in TTY mode.

Unit tests to cover the changes would be great.

[tollmanz]

Closing this in favor of PR #60

[arm4b]

Formatting colored strings is broken in latest php-cli-tools.
Could be reproduced by instructions in initial message.

However php-cli-tools version where @tollmanz fixed the issue works good:

composer.json:

{
    "require": {
        "wp-cli/php-cli-tools": "dev-master#c3a01e7b7f8e8bc2fe6c5ae7f198a185e9256c6f"
    }
}

[danielbachhuber]

Formatting colored strings is broken in latest php-cli-tools.

Zack's pull request was merged, so not sure how it could be broken in latest but fixed in his version. Which version of php-cli-tools are you using? Can you verify that it's 0.10.1?

[arm4b]

I'm using dev-master with latest changes.
Just checked v0.10.1 - it has that bug too.

I'll dig later into the source code, maybe could have fun with it by finding where it was introduced again :)