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.

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

Bug: Automatic no CLI colors for Windows terminals #2849

Closed
paulbalandan opened this issue Apr 19, 2020 · 6 comments
Closed

Bug: Automatic no CLI colors for Windows terminals #2849

paulbalandan opened this issue Apr 19, 2020 · 6 comments
Labels
bug Verified issues on the current code behavior or pull requests that will fix them

Comments

@paulbalandan
Copy link
Member

Describe the bug
CLI::color() outright returns an unformatted text when running on Windows. It explicitly requires that $_SERVER['ANSICON'] be set to return a formatted text.

Modern Windows machines now natively supports VT100 Sequences by default starting with Win 10. As such, I believe Windows terminal are deprived to display colors.

I am thinking of adding checks for php's sapi_windows_vt100_support() or stream_isatty() to check the stream first. The only problem I see is that it is currently not testable with the current setup of the unit test since it is using filtered streams and those two methods don't support that. Also, the streams (STDOUT, STDERR) to test are tightly coupled in the code.

CodeIgniter 4 version
4.0.2
4.0-develop

Affected module(s)
CLI::color()

Expected behavior, and steps to reproduce if appropriate
In a Windows environment, run any CLI::color() with provided foreground and/or background. Example:

CLI::color('test', 'white', 'red');

Context

  • OS: Windows 10 version 1909
  • Web server: Apache 2.4.43
  • PHP version 7.4.5
@paulbalandan paulbalandan added the bug Verified issues on the current code behavior or pull requests that will fix them label Apr 19, 2020
@lonnieezell
Copy link
Member

You're probably right here. Windows didn't have all of the new fancy CLI stuff when this lib was developed/ported from FuelPHP.

I don't currently have a Win10 setup easy to get to. Any chance you'd be willing to look into this and submit a PR?

@paulbalandan
Copy link
Member Author

I tried changing the library yesterday but had many failing tests due to the use of filtered streams.

So I was thinking of moving all output methods to a new dedicated class, passing a stream resource to its constructor. This is made so that in unit tests we can try other PHP streams available. Is this okay?

@lonnieezell
Copy link
Member

I'm confused about the streams issue. We are currently testing the CLI codes, right? How are the streams behaving differently on Win10 than in Mac/Linux?

@paulbalandan
Copy link
Member Author

I am sorry for the confusion. There is no different behavior of the streams. My issue is how we can run tests on the color support (assuming use of sapi_windows_vt100_support() or stream_isatty()) without using the stream_filter in the current unit test, which gives this error: Warning: stream_isatty(): cannot cast a filtered stream on this system or Warning: sapi_windows_vt100_support(): cannot cast a filtered stream on this system.

So, basically, I have this prototype in mind:

class Output
{
    protected $stream;

    public function __construct($stream)
    {
        $this->stream = $stream;
    }

    public function hasColorSupport(): bool
    {
        // other color checking codes
        return stream_isatty($this->stream);
    }

    public function write($text, $foreground = null, $background = null)
    {
        $text = $this->color($text, $foreground, $background);
        fwrite($this->stream, $text);
    }

    public function color($text, $foreground, $background = null)
    {
        if (! $this->hasColorSupport())
        {
            return $text;
        }

       // else format as usual and return the formatted text
       return $text;
    }
}

For the unit test, we can utilize phpunit's setUp() to set the stream to php://memory so that we prevent actual writing of output during the test:

use CodeIgniter\CLI\Output;

class OutputTest extends \CodeIgniter\Test\CIUnitTestCase
{
    private $stream;

    public function setUp(): void
    {
        $this->stream = @fopen('php://memory', 'a', false);
    }

    public function tearDown(): void
    {
        $this->stream = null;
    }

    public function testColorSupport()
    {
        $output = new Output($this->stream);
        $this->assertIsBool($output->hasColorSupport());
        // $output->hasColorSupport() will no longer throw an ErrorException because
       // we did not use filtered streams
    }

    public function testWrite()
    {
        $output = new Output($this->stream);
        $output->write('text', 'light_red');
        rewind($this->stream);
        $this->assertEquals("\033[1;31mtext\033[0m", stream_get_contents($this->stream));
    }
}

BTW, the idea for using php://memory was adopted from Symfony.

Hope this gets your pre-approval so I can start drafting.

@lonnieezell
Copy link
Member

Ah, ok, thanks makes sense to me then. Go for it. We just need to ensure we keep the API with the CLI class identical to what it is now.

@paulbalandan
Copy link
Member Author

Thanks for that. I'll bear that in mind. I'll keep you posted in the following days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Verified issues on the current code behavior or pull requests that will fix them
Projects
None yet
Development

No branches or pull requests

2 participants