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

Fix potential endless loop in prompt() #129

Merged
merged 2 commits into from Apr 10, 2018

Conversation

3 participants
@zipofar
Contributor

zipofar commented Mar 16, 2018

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

@danielbachhuber

This comment has been minimized.

Member

danielbachhuber commented Mar 18, 2018

Hi @zipofar,

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

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

@danielbachhuber danielbachhuber referenced this pull request Apr 10, 2018

Closed

WP-CLI v1.5.1 release checklist #4766

8 of 8 tasks complete
@schlessera

This comment has been minimized.

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

This comment has been minimized.

Member

danielbachhuber commented Apr 10, 2018

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

This comment has been minimized.

Member

schlessera commented Apr 10, 2018

Thanks for the PR, @zipofar!

@schlessera schlessera merged commit dcdef5d into wp-cli:master Apr 10, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment