wp-cli/php-cli-tools — pull request #60 by tollmanz (2014-08-03 18:32:49+00:00)
Title: Colors::length returns incorrect lengths for colorized strings

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
setHeaders( array( 'Test Header' ) );
$table->setRows( array( array( $success )) );
$table->display();
}
}

WP_CLI::add_command( 'failwhale', 'ReducedTestCase' );
```

1. The output produced is:

```bash
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.

1. The expected output is:

```bash
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:

```php
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
1. 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.