wp-cli / php-cli-tools

A collection of tools to help with PHP command line utilities
MIT License
673 stars 118 forks source link

Fix potential endless loop in prompt() #129

Closed zipofar closed 6 years ago

zipofar commented 6 years ago

If user must enter zero, he can't exit from cycle.

danielbachhuber commented 6 years ago

Hi @zipofar,

Thanks for the pull request. Could you include a test for this change too?

schlessera commented 6 years ago

I tried adding tests to the Streams class using fake STDIN/STDOUT streams built with php://memory resources, but without any success.

Here's what I had so far:

<?php

use cli\Streams;

/**
 * Class TestStreams
 */
class TestStreams extends PHPUnit_Framework_TestCase {

    /** @var resource */
    protected $in;
    /** @var resource */
    protected $out;

    /**
     * Redirects STDIN/STDOUT to in-memory stream.
     */
    public function setUp() {
        foreach ( array( 'in', 'out' ) as $stream ) {
            $this->$stream = fopen( 'php://memory', 'wb+' );
            Streams::setStream( $stream, $this->$stream );
        }
    }

    public function testPrompt() {
        fwrite( $this->in, 'test' . PHP_EOL );
        $result   = Streams::prompt( 'Question' );
        $this->assertEquals( 'test', $result );
    }
}

@danielbachhuber Do you have another idea on how to test this properly?

danielbachhuber commented 6 years ago

Do you have another idea on how to test this properly?

Nope. If it proves too difficult and we feel confident in the change, I think it can land without tests.

schlessera commented 6 years ago

Thanks for the PR, @zipofar!