Skip to content

Fix potential endless loop in prompt()#129

Merged
schlessera merged 2 commits intowp-cli:masterfrom
zipofar:patch-1
Apr 10, 2018
Merged

Fix potential endless loop in prompt()#129
schlessera merged 2 commits intowp-cli:masterfrom
zipofar:patch-1

Conversation

@zipofar
Copy link
Copy Markdown
Contributor

@zipofar zipofar commented Mar 16, 2018

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

@danielbachhuber
Copy link
Copy Markdown
Member

Hi @zipofar,

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

@schlessera schlessera changed the title If user enter zero, then cycle never stop with empty() check Fix potential endless loop in prompt() Mar 20, 2018
@schlessera
Copy link
Copy Markdown
Member

schlessera commented Apr 10, 2018

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
Copy link
Copy Markdown
Member

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 schlessera added this to the 0.11.9 milestone Apr 10, 2018
@schlessera
Copy link
Copy Markdown
Member

Thanks for the PR, @zipofar!

@schlessera schlessera merged commit dcdef5d into wp-cli:master Apr 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants