Update Formatter\show_table to use Runner->in_color rather than shouldColorize

[Dan-Q]

Fixes #5744.

Problem

When a WP-CLI command outputs a table, the table formatter:

1. Checks whether colorization is enabled. If so, disables it.
2. Outputs the table.
3. If colorization was previously enabled, re-enables it.

The problem is that we're using Colors::shouldColorize() to determine whether colorization is enabled. Colors::shouldColorize() returns true if (a) the terminal is capable of colors, AND (b) no output has yet been produced by the colorizer.

This means that if WP-CLI outputs a table before it outputs any colorized messages, it will always switch to color mode after the table has bveen printed.

You can demonstrate this for yourself by adding the line WP_CLI::success( 'nothing here!' ); to the top of function show_table. This makes the problem go away (but prints out "Success: nothing here!" at the top of every table, which is undesirable).

Solution

This PR switches the table formatter to use WP_CLI::get_runner()->in_color() to determine whether or not colorization is enabled. This returns false if --no-color was passed, true otherwise, and so the test below works properly.

Testing

1. Check out this branch
2. Set up a copy of WordPress with an outdated plugin, e.g. bin/wp plugin install classic-editor --version=1.6.2 --path=/your/wp
3. Request that the plugin is updated, e.g. bin/wp plugin update --all --path=/your/wp, and see the usual output
4. Set up a copy of WordPress with an outdated plugin, e.g. bin/wp plugin install classic-editor --version=1.6.2 --path=/your/wp
5. Request that the plugin is updated without colorized output, e.g. bin/wp plugin update --all --no-color --path=/your/wp
6. Please test around to ensure that , e.g. quiet mode (--quiet) or using a terminal that isn't capable of color output

[danielbachhuber]

I don't love adding a dependency on WP_CLI::get_runner() inside of this Formatter class.

My first thought was to add a new class-level $in_color variable that can be passed when instantiated:

diff --git a/php/WP_CLI/Formatter.php b/php/WP_CLI/Formatter.php
index 8dd354f5..74fd714c 100644
--- a/php/WP_CLI/Formatter.php
+++ b/php/WP_CLI/Formatter.php
@@ -33,7 +33,7 @@ class Formatter {
 	 * @param string|bool $prefix Check if fields have a standard prefix.
 	 * False indicates empty prefix.
 	 */
-	public function __construct( &$assoc_args, $fields = null, $prefix = false ) {
+	public function __construct( &$assoc_args, $fields = null, $prefix = false, $in_color = true ) {
 		$format_args = [
 			'format' => 'table',
 			'fields' => $fields,
@@ -53,8 +53,9 @@ class Formatter {
 
 		$format_args['fields'] = array_map( 'trim', $format_args['fields'] );
 
-		$this->args   = $format_args;
-		$this->prefix = $prefix;
+		$this->args     = $format_args;
+		$this->prefix   = $prefix;
+		$this->in_color = $in_color
 	}
 
 	/**

However, this would just put the odd dependency elsewhere:

wp-cli/php/utils.php

Lines 362 to 369 in 9239cff

	function format_items( $format, $items, $fields ) {
	$assoc_args = [
	'format' => $format,
	'fields' => $fields,
	];
	$formatter = new Formatter( $assoc_args );
	$formatter->display_items( $items );
	}

Because Formatter already calls WP_CLI::print_value(), Utils\write_csv(), etc., I think it's fine to call out to WP_CLI:: or a utility function. We could add an in_color() method like this:

diff --git a/php/class-wp-cli.php b/php/class-wp-cli.php
index c775bbc3..67d29c7a 100644
--- a/php/class-wp-cli.php
+++ b/php/class-wp-cli.php
@@ -728,6 +728,20 @@ class WP_CLI {
 		unset( self::$deferred_additions[ $name ] );
 	}
 
+	/**
+	 * Whether or not WP-CLI is currently in color.
+	 *
+	 * @param bool|null $set Whether or not to set the status.
+	 * @return bool
+	 */
+	public static function in_color( $set = null ) {
+		static $in_color;
+		if ( ! is_null( $set ) ) {
+			$in_color = (bool) $set;
+		}
+		return $in_color;
+	}
+
 	/**
 	 * Display informational message without prefix, and ignore `--quiet`.
 	 *

We'd then use it like this:

diff --git a/php/WP_CLI/Formatter.php b/php/WP_CLI/Formatter.php
index 8dd354f5..7f2500f5 100644
--- a/php/WP_CLI/Formatter.php
+++ b/php/WP_CLI/Formatter.php
@@ -301,7 +301,7 @@ class Formatter {
 	private static function show_table( $items, $fields, $ascii_pre_colorized = false ) {
 		$table = new Table();
 
-		$enabled = Colors::shouldColorize();
+		$enabled = WP_CLI::in_color();
 		if ( $enabled ) {
 			Colors::disable( true );
 		}
diff --git a/php/WP_CLI/Runner.php b/php/WP_CLI/Runner.php
index 9e7ceb27..f4d7948c 100644
--- a/php/WP_CLI/Runner.php
+++ b/php/WP_CLI/Runner.php
@@ -938,6 +938,7 @@ class Runner {
 		} else {
 			$this->colorize = $this->config['color'];
 		}
+		WP_CLI::in_color( $this->colorize );
 	}
 
 	public function init_logger() {

This is all somewhat spaghetti logic, but I think what I propose is marginally better than calling WP_CLI::get_runner() directly.

Thoughts?

[schlessera]

@danielbachhuber I agree that this dependency is not great. However, this has been one of the longstanding architectural issues that cannot easily be redressed, and references to the WP_CLI::get_runner() are distributed all across the codebase. Therefore, I think it doesn't make sense to add additional spaghetti for this one instance when we cannot at the same time fix the bigger picture. I'd opt for keeping with the WP_CLI::get_runner() for now until we decide for a bigger rewrite.

[danielbachhuber]

@schlessera Ok, sounds good.

[schlessera]

I had trouble coming up with an automated test for this, as Behat has its own say of whether color is supported or not.

However, I was able to verify the behavior with manual testing, so I'm confident we can merge this.

[schlessera]

Thanks, @Dan-Q!