Skip to content

Commit 7364c84

Browse files
ljonesflljonesfl
committed
fixes
1 parent 230bf04 commit 7364c84

5 files changed

Lines changed: 260 additions & 41 deletions

File tree

‎src/Cli/Commands/Command.php‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -311,11 +311,18 @@ public function validate(): void
311311
* Creates a default StdinInputReader if not already set.
312312
* This enables testable CLI commands by abstracting user input.
313313
*
314+
* If output hasn't been set, a default Output instance will be created automatically.
315+
*
314316
* @return IInputReader
315317
*/
316318
protected function getInputReader(): IInputReader
317319
{
318320
if( !$this->inputReader ) {
321+
// Ensure output is initialized before creating StdinInputReader
322+
if( !isset( $this->output ) ) {
323+
$this->output = new Output();
324+
}
325+
319326
$this->inputReader = new StdinInputReader( $this->output );
320327
}
321328

‎src/Cli/IO/StdinInputReader.php‎

Lines changed: 96 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,13 @@
1414
*/
1515
class StdinInputReader implements IInputReader
1616
{
17+
/**
18+
* Maximum number of retry attempts for invalid input.
19+
* After this many failed attempts, the method will fall back to the default
20+
* or throw an exception if no default is provided.
21+
*/
22+
private const MAX_RETRY_ATTEMPTS = 10;
23+
1724
/**
1825
* @param Output $output Output instance for displaying prompts
1926
*/
@@ -55,67 +62,117 @@ public function secret( string $message ): string
5562
{
5663
$this->output->write( $message, false );
5764

58-
// Only hide input on Unix-like systems
59-
if( strtoupper( substr( PHP_OS, 0, 3 ) ) !== 'WIN' ) {
60-
// Disable terminal echo
61-
system( 'stty -echo' );
62-
$input = fgets( STDIN );
63-
// Re-enable terminal echo
64-
system( 'stty echo' );
65+
// Check if we can hide input (Unix-like system + TTY)
66+
$canHideInput = strtoupper( substr( PHP_OS, 0, 3 ) ) !== 'WIN' && $this->isTty();
67+
68+
if( $canHideInput ) {
69+
// Disable terminal echo with exception safety
70+
try {
71+
system( 'stty -echo' );
72+
$input = fgets( STDIN );
73+
} finally {
74+
// Always restore terminal echo, even if an exception occurred
75+
system( 'stty echo' );
76+
}
77+
6578
// Add newline since user's enter key wasn't echoed
6679
$this->output->writeln( '' );
6780
} else {
68-
// On Windows, fall back to visible input
69-
// A proper Windows implementation would use COM or other Windows-specific methods
81+
// Fall back to visible input (Windows or non-TTY)
82+
// On Windows, a proper implementation would use COM or other Windows-specific methods
83+
// For non-TTY (CI/CD, piped input), we just read normally without trying stty
7084
$input = fgets( STDIN );
7185
}
7286

7387
return $input !== false ? trim( $input ) : '';
7488
}
7589

90+
/**
91+
* Check if STDIN is a TTY (terminal).
92+
*
93+
* This prevents stty errors when running in non-interactive environments
94+
* like CI/CD pipelines, automated scripts, or with piped input.
95+
*
96+
* @return bool True if STDIN is a TTY, false otherwise
97+
*/
98+
private function isTty(): bool
99+
{
100+
// Use posix_isatty if available (preferred, more reliable)
101+
if( function_exists( 'posix_isatty' ) ) {
102+
return @posix_isatty( STDIN );
103+
}
104+
105+
// Fall back to stream_isatty (PHP 7.2+)
106+
if( function_exists( 'stream_isatty' ) ) {
107+
return @stream_isatty( STDIN );
108+
}
109+
110+
// If neither function is available, assume it's not a TTY to be safe
111+
// This prevents errors in environments where we can't check
112+
return false;
113+
}
114+
76115
/**
77116
* @inheritDoc
78117
*/
79118
public function choice( string $message, array $options, ?string $default = null ): string
80119
{
81-
// Display the prompt message
82-
$this->output->writeln( $message );
83-
$this->output->writeln( '' );
84-
85-
// Display options with index numbers
86-
foreach( $options as $index => $option ) {
87-
$marker = ($default === $option) ? '*' : ' ';
88-
$this->output->writeln( " [{$marker}] {$index}. {$option}" );
89-
}
120+
$attempts = 0;
90121

91-
$this->output->writeln( '' );
122+
while( $attempts < self::MAX_RETRY_ATTEMPTS ) {
123+
// Display the prompt message (only on first attempt)
124+
if( $attempts === 0 ) {
125+
$this->output->writeln( $message );
126+
$this->output->writeln( '' );
92127

93-
// Prompt for selection
94-
$prompt = $default !== null ? "Choice [{$default}]: " : "Choice: ";
95-
$response = $this->prompt( $prompt );
128+
// Display options with index numbers
129+
foreach( $options as $index => $option ) {
130+
$marker = ($default === $option) ? '*' : ' ';
131+
$this->output->writeln( " [{$marker}] {$index}. {$option}" );
132+
}
96133

97-
// If user just presses enter and there's a default, use it
98-
if( $response === '' && $default !== null ) {
99-
return $default;
100-
}
134+
$this->output->writeln( '' );
135+
}
101136

102-
// Check if response is a numeric index
103-
if( is_numeric( $response ) ) {
104-
$index = (int)$response;
105-
if( isset( $options[$index] ) ) {
106-
return $options[$index];
137+
// Prompt for selection
138+
$prompt = $default !== null ? "Choice [{$default}]: " : "Choice: ";
139+
$response = $this->prompt( $prompt );
140+
141+
// If user just presses enter and there's a default, use it
142+
if( $response === '' && $default !== null ) {
143+
return $default;
107144
}
108-
}
109145

110-
// Check if response matches an option exactly
111-
if( in_array( $response, $options, true ) ) {
112-
return $response;
146+
// Check if response is a numeric index
147+
if( is_numeric( $response ) ) {
148+
$index = (int)$response;
149+
if( isset( $options[$index] ) ) {
150+
return $options[$index];
151+
}
152+
}
153+
154+
// Check if response matches an option exactly
155+
if( in_array( $response, $options, true ) ) {
156+
return $response;
157+
}
158+
159+
// Invalid choice - increment counter and show error
160+
$attempts++;
161+
162+
if( $attempts < self::MAX_RETRY_ATTEMPTS ) {
163+
$this->output->error( "Invalid choice. Please try again." );
164+
$this->output->writeln( '' );
165+
}
113166
}
114167

115-
// Invalid choice - ask again
116-
$this->output->error( "Invalid choice. Please try again." );
117-
$this->output->writeln( '' );
168+
// Max retries exceeded - fall back to default or throw exception
169+
if( $default !== null ) {
170+
$this->output->warning( "Maximum retry attempts exceeded. Using default: {$default}" );
171+
return $default;
172+
}
118173

119-
return $this->choice( $message, $options, $default );
174+
throw new \RuntimeException(
175+
'Maximum retry attempts exceeded and no default value provided for choice prompt'
176+
);
120177
}
121178
}

‎tests/Cli/Commands/CommandTest.php‎

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -202,6 +202,27 @@ public function testGetInputReaderCreatesDefaultWhenNotSet(): void
202202
$this->assertInstanceOf( IInputReader::class, $reader );
203203
}
204204

205+
public function testGetInputReaderCreatesDefaultOutputWhenNotSet(): void
206+
{
207+
// Don't set output - should auto-initialize
208+
// Access protected method using reflection
209+
$reflection = new \ReflectionClass( $this->command );
210+
$method = $reflection->getMethod( 'getInputReader' );
211+
$method->setAccessible( true );
212+
213+
// Should not throw exception even though output wasn't set
214+
$reader = $method->invoke( $this->command );
215+
216+
$this->assertInstanceOf( IInputReader::class, $reader );
217+
218+
// Verify output was auto-initialized
219+
$outputProp = $reflection->getProperty( 'output' );
220+
$outputProp->setAccessible( true );
221+
$output = $outputProp->getValue( $this->command );
222+
223+
$this->assertInstanceOf( Output::class, $output );
224+
}
225+
205226
public function testGetInputReaderReturnsInjectedReader(): void
206227
{
207228
$inputReader = new TestInputReader();
@@ -330,6 +351,36 @@ public function testChoiceWithDefault(): void
330351

331352
$this->assertEquals( 'dev', $result );
332353
}
354+
355+
public function testPromptWorksWithoutOutputSet(): void
356+
{
357+
// Test that prompt works even if setOutput() was never called
358+
$inputReader = new TestInputReader();
359+
$inputReader->addResponse( 'test' );
360+
361+
$command = new InteractiveTestCommand();
362+
$command->setInputReader( $inputReader );
363+
364+
// Should not throw exception even though output wasn't set
365+
$result = $command->callPrompt( 'Enter: ' );
366+
367+
$this->assertEquals( 'test', $result );
368+
}
369+
370+
public function testConfirmWorksWithoutOutputSet(): void
371+
{
372+
// Test that confirm works even if setOutput() was never called
373+
$inputReader = new TestInputReader();
374+
$inputReader->addResponse( 'yes' );
375+
376+
$command = new InteractiveTestCommand();
377+
$command->setInputReader( $inputReader );
378+
379+
// Should not throw exception even though output wasn't set
380+
$result = $command->callConfirm( 'Continue?' );
381+
382+
$this->assertTrue( $result );
383+
}
333384
}
334385

335386
/**

‎tests/Cli/IO/StdinInputReaderTest.php‎

Lines changed: 99 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -158,7 +158,7 @@ public function testChoiceWithExactMatch(): void
158158
$this->assertEquals( 'staging', $result );
159159
}
160160

161-
public function testChoiceWithInvalidSelectionRetriesOnce(): void
161+
public function testChoiceWithInvalidSelectionRetries(): void
162162
{
163163
$mock = $this->getMockBuilder( StdinInputReader::class )
164164
->setConstructorArgs( [$this->output] )
@@ -190,4 +190,102 @@ public function testSecretDelegatesToPrompt(): void
190190
// This test just verifies the interface works
191191
$this->assertInstanceOf( StdinInputReader::class, $mock );
192192
}
193+
194+
public function testSecretMethodExistsAndIsCallable(): void
195+
{
196+
// Verify secret method exists and can be called
197+
$reader = new StdinInputReader( $this->output );
198+
$reflection = new \ReflectionClass( $reader );
199+
200+
$this->assertTrue( $reflection->hasMethod( 'secret' ) );
201+
$method = $reflection->getMethod( 'secret' );
202+
$this->assertTrue( $method->isPublic() );
203+
204+
// The method should handle both TTY and non-TTY environments gracefully
205+
// We can't easily test actual STDIN reading in unit tests, but we verify
206+
// the method exists and has the correct visibility
207+
}
208+
209+
public function testIsTtyMethodExists(): void
210+
{
211+
// Use reflection to verify the isTty method exists
212+
$reflection = new \ReflectionClass( StdinInputReader::class );
213+
214+
$this->assertTrue( $reflection->hasMethod( 'isTty' ) );
215+
216+
$method = $reflection->getMethod( 'isTty' );
217+
$this->assertTrue( $method->isPrivate() );
218+
}
219+
220+
public function testIsTtyReturnsBooleanValue(): void
221+
{
222+
// Use reflection to invoke the private isTty method
223+
$reader = new StdinInputReader( $this->output );
224+
$reflection = new \ReflectionClass( $reader );
225+
$method = $reflection->getMethod( 'isTty' );
226+
$method->setAccessible( true );
227+
228+
$result = $method->invoke( $reader );
229+
230+
// Should return a boolean value (true or false depending on environment)
231+
$this->assertIsBool( $result );
232+
}
233+
234+
public function testChoiceWithMaxRetriesUsesDefault(): void
235+
{
236+
$mock = $this->getMockBuilder( StdinInputReader::class )
237+
->setConstructorArgs( [$this->output] )
238+
->onlyMethods( ['prompt'] )
239+
->getMock();
240+
241+
// All responses will be invalid
242+
$mock->expects( $this->exactly( 10 ) ) // MAX_RETRY_ATTEMPTS
243+
->method( 'prompt' )
244+
->willReturn( 'invalid' );
245+
246+
$options = ['dev', 'staging', 'prod'];
247+
$result = $mock->choice( 'Select:', $options, 'dev' );
248+
249+
// Should fall back to default after max retries
250+
$this->assertEquals( 'dev', $result );
251+
}
252+
253+
public function testChoiceWithMaxRetriesThrowsExceptionWithoutDefault(): void
254+
{
255+
$mock = $this->getMockBuilder( StdinInputReader::class )
256+
->setConstructorArgs( [$this->output] )
257+
->onlyMethods( ['prompt'] )
258+
->getMock();
259+
260+
// All responses will be invalid
261+
$mock->expects( $this->exactly( 10 ) ) // MAX_RETRY_ATTEMPTS
262+
->method( 'prompt' )
263+
->willReturn( 'invalid' );
264+
265+
$options = ['dev', 'staging', 'prod'];
266+
267+
$this->expectException( \RuntimeException::class );
268+
$this->expectExceptionMessage( 'Maximum retry attempts exceeded and no default value provided' );
269+
270+
$mock->choice( 'Select:', $options ); // No default
271+
}
272+
273+
public function testChoiceSucceedsBeforeMaxRetries(): void
274+
{
275+
$mock = $this->getMockBuilder( StdinInputReader::class )
276+
->setConstructorArgs( [$this->output] )
277+
->onlyMethods( ['prompt'] )
278+
->getMock();
279+
280+
// First 3 invalid, then valid on 4th attempt
281+
$mock->expects( $this->exactly( 4 ) )
282+
->method( 'prompt' )
283+
->willReturnOnConsecutiveCalls( 'invalid', 'bad', 'nope', 'staging' );
284+
285+
$options = ['dev', 'staging', 'prod'];
286+
$result = $mock->choice( 'Select:', $options );
287+
288+
// Should succeed with valid response before hitting max retries
289+
$this->assertEquals( 'staging', $result );
290+
}
193291
}

‎versionlog.md‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,11 @@
11
## 0.8.9
2-
* Testability refactoring.
2+
* Testability refactoring: Added IInputReader abstraction for testing CLI commands with user input.
3+
* Fixed potential stack overflow in StdinInputReader::choice() by replacing recursive retry with iterative loop.
4+
* Added MAX_RETRY_ATTEMPTS constant to prevent infinite loops on invalid input.
5+
* Fixed empty() bug where '0' was incorrectly treated as empty response.
6+
* Added defensive guard in Command::getInputReader() to prevent uninitialized property access by lazily initializing Output when needed.
7+
* CRITICAL: Added exception safety to secret() method with try-finally to ensure terminal echo is always restored, preventing broken terminal states.
8+
* CRITICAL: Added TTY check (isTty() method) to prevent stty errors in non-interactive environments (CI/CD, piped input, automated scripts).
39

410
## 0.8.8 2025-11-28
511
## 0.8.7 2025-11-24

0 commit comments

Comments
 (0)