Skip to content

Commit 9ac3d11

Browse files
paulbalandanpaulbalandan
committed
address review
1 parent 7acafa2 commit 9ac3d11

2 files changed

Lines changed: 70 additions & 29 deletions

File tree

‎system/CLI/AbstractCommand.php‎

Lines changed: 17 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -114,12 +114,17 @@ abstract class AbstractCommand
114114
private ?string $lastArrayArgument = null;
115115

116116
/**
117-
* Whether the command is in interactive mode. When `null`, the interactive state is resolved based
118-
* on the presence of the `--no-interaction` option and whether STDIN is a TTY. If boolean, this value
119-
* takes precedence over the flag and TTY detection.
117+
* Interactive state pinned by `setInteractive()`. When boolean, it takes precedence over
118+
* the per-run flag and TTY detection, and remains in effect across `run()` calls on
119+
* the same instance.
120120
*/
121121
private ?bool $interactive = null;
122122

123+
/**
124+
* Per-run interactive state derived from `--no-interaction` / `-N` in the current `$options`.
125+
*/
126+
private ?bool $runtimeInteractive = null;
127+
123128
/**
124129
* @throws InvalidArgumentDefinitionException
125130
* @throws InvalidOptionDefinitionException
@@ -355,28 +360,23 @@ public function hasNegation(string $name): bool
355360
*
356361
* Resolution order:
357362
* 1. An explicit `setInteractive()` call wins.
358-
* 2. Otherwise, the command is interactive when STDIN is a TTY.
363+
* 2. Otherwise, the `--no-interaction` / `-N` flag from the current `run()`
364+
* forces non-interactive.
365+
* 3. Otherwise, the command is interactive when STDIN is a TTY.
359366
*
360367
* Non-CLI contexts (e.g., a controller invoking `command()`) don't expose
361368
* `STDIN` at all; those always resolve as non-interactive.
362-
*
363-
* Note: the `--no-interaction` / `-N` flag is folded into the explicit state
364-
* by `run()` before interactive hooks fire, so callers do not need to
365-
* inspect the options array themselves.
366369
*/
367370
public function isInteractive(): bool
368371
{
369-
if ($this->interactive !== null) {
370-
return $this->interactive;
371-
}
372-
373-
return defined('STDIN') && CLI::streamSupports('stream_isatty', \STDIN);
372+
return $this->interactive
373+
?? $this->runtimeInteractive
374+
?? (defined('STDIN') && CLI::streamSupports('stream_isatty', \STDIN));
374375
}
375376

376377
/**
377378
* Pins the interactive state, overriding both the `--no-interaction` flag
378-
* and STDIN TTY detection. Typically called from `initialize()` or by
379-
* an outer caller that needs to force a specific mode.
379+
* and STDIN TTY detection.
380380
*/
381381
public function setInteractive(bool $interactive): static
382382
{
@@ -417,9 +417,8 @@ public function setInteractive(bool $interactive): static
417417
*/
418418
final public function run(array $arguments, array $options): int
419419
{
420-
if ($this->interactive === null && $this->hasUnboundOption('no-interaction', $options)) {
421-
$this->interactive = false;
422-
}
420+
// Reset per-run interactive state from the current options.
421+
$this->runtimeInteractive = $this->hasUnboundOption('no-interaction', $options) ? false : null;
423422

424423
$this->initialize($arguments, $options);
425424

‎tests/system/CLI/AbstractCommandTest.php‎

Lines changed: 53 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -669,12 +669,10 @@ public static function provideValidationOfOptions(): iterable
669669

670670
public function testInteractMutationsCarryThroughToExecute(): void
671671
{
672-
// Supply neither the positional argument nor the --force flag.
673-
// interact() populates both; bind() and validate() run afterwards, so
674-
// execute() should see the mutated values fully bound and validated.
675672
$command = new InteractFixtureCommand(new Commands());
676673
$command->run([], []);
677674

675+
$this->assertTrue($command->isInteractive());
678676
$this->assertSame(['name' => 'from-interact'], $command->executedArguments);
679677
$this->assertTrue($command->executedOptions['force']);
680678
}
@@ -684,6 +682,7 @@ public function testInteractIsSkippedWhenNoInteractionFlagIsPassed(): void
684682
$command = new InteractFixtureCommand(new Commands());
685683
$command->run([], ['no-interaction' => null]);
686684

685+
$this->assertFalse($command->isInteractive());
687686
$this->assertSame(['name' => 'anonymous'], $command->executedArguments);
688687
$this->assertFalse($command->executedOptions['force']);
689688
}
@@ -693,6 +692,7 @@ public function testInteractIsSkippedWhenShortcutFlagIsPassed(): void
693692
$command = new InteractFixtureCommand(new Commands());
694693
$command->run([], ['N' => null]);
695694

695+
$this->assertFalse($command->isInteractive());
696696
$this->assertSame(['name' => 'anonymous'], $command->executedArguments);
697697
$this->assertFalse($command->executedOptions['force']);
698698
}
@@ -701,8 +701,11 @@ public function testInteractIsSkippedWhenSetInteractiveFalseIsCalled(): void
701701
{
702702
$command = new InteractFixtureCommand(new Commands());
703703
$command->setInteractive(false);
704+
$this->assertFalse($command->isInteractive());
705+
704706
$command->run([], []);
705707

708+
$this->assertFalse($command->isInteractive());
706709
$this->assertSame(['name' => 'anonymous'], $command->executedArguments);
707710
$this->assertFalse($command->executedOptions['force']);
708711
}
@@ -714,10 +717,41 @@ public function testSetInteractiveTrueOverridesNoInteractionFlag(): void
714717
$command->setInteractive(true);
715718
$command->run([], ['no-interaction' => null]);
716719

720+
$this->assertTrue($command->isInteractive());
721+
$this->assertSame(['name' => 'from-interact'], $command->executedArguments);
722+
$this->assertTrue($command->executedOptions['force']);
723+
}
724+
725+
public function testNoInteractionFlagDoesNotLeakAcrossRuns(): void
726+
{
727+
$command = new InteractFixtureCommand(new Commands());
728+
729+
$command->run([], ['no-interaction' => null]);
730+
$this->assertFalse($command->isInteractive());
731+
$this->assertSame(['name' => 'anonymous'], $command->executedArguments);
732+
$this->assertFalse($command->executedOptions['force']);
733+
734+
$command->run([], []);
735+
$this->assertTrue($command->isInteractive());
717736
$this->assertSame(['name' => 'from-interact'], $command->executedArguments);
718737
$this->assertTrue($command->executedOptions['force']);
719738
}
720739

740+
public function testSetInteractiveCallPersistsAcrossRuns(): void
741+
{
742+
$command = new InteractFixtureCommand(new Commands());
743+
$command->setInteractive(false);
744+
$this->assertFalse($command->isInteractive());
745+
746+
$command->run([], []);
747+
$this->assertFalse($command->isInteractive());
748+
$this->assertSame(['name' => 'anonymous'], $command->executedArguments);
749+
750+
$command->run([], []);
751+
$this->assertFalse($command->isInteractive());
752+
$this->assertSame(['name' => 'anonymous'], $command->executedArguments);
753+
}
754+
721755
public function testIsInteractiveReflectsExplicitState(): void
722756
{
723757
$command = new InteractFixtureCommand(new Commands());
@@ -740,6 +774,7 @@ public function testNoInteractionCascadesToSubCommandsViaCall(): void
740774
$exitCode = $command->run([], ['no-interaction' => null]);
741775

742776
$this->assertSame(EXIT_SUCCESS, $exitCode);
777+
$this->assertFalse($command->isInteractive());
743778
$this->assertFalse(InteractiveStateProbeCommand::$interactCalled);
744779
$this->assertFalse(InteractiveStateProbeCommand::$observedInteractive);
745780
}
@@ -751,6 +786,7 @@ public function testSubCommandStaysInteractiveWhenParentIsInteractive(): void
751786
$exitCode = $command->run([], []);
752787

753788
$this->assertSame(EXIT_SUCCESS, $exitCode);
789+
$this->assertTrue($command->isInteractive());
754790
$this->assertTrue(InteractiveStateProbeCommand::$interactCalled);
755791
$this->assertTrue(InteractiveStateProbeCommand::$observedInteractive);
756792
}
@@ -764,6 +800,7 @@ public function testCallAllowsSubCommandInteractiveEvenWhenParentIsNonInteractiv
764800
$exitCode = $command->run([], []);
765801

766802
$this->assertSame(EXIT_SUCCESS, $exitCode);
803+
$this->assertFalse($command->isInteractive());
767804
$this->assertTrue(InteractiveStateProbeCommand::$interactCalled);
768805
$this->assertTrue(InteractiveStateProbeCommand::$observedInteractive);
769806
}
@@ -777,16 +814,18 @@ public function testCallForcesSubCommandNonInteractiveEvenWhenParentIsInteractiv
777814
$exitCode = $command->run([], []);
778815

779816
$this->assertSame(EXIT_SUCCESS, $exitCode);
817+
$this->assertTrue($command->isInteractive());
780818
$this->assertFalse(InteractiveStateProbeCommand::$interactCalled);
781819
$this->assertFalse(InteractiveStateProbeCommand::$observedInteractive);
782820
}
783821

822+
/**
823+
* Caller passes --no-interaction in the sub-command's options, but also
824+
* sets noInteractionOverride to false: the explicit parameter wins and
825+
* the inherited flag is stripped under both its long name and its shortcut.
826+
*/
784827
public function testCallStripsInheritedNoInteractionWhenCallerAllowsInteraction(): void
785828
{
786-
// Caller passes --no-interaction in the sub-command's options, but also
787-
// sets noInteractionOverride to false: the explicit parameter wins and
788-
// the inherited flag is stripped under both its long name and its shortcut.
789-
790829
$command = new ParentCallsInteractFixtureCommand(new Commands());
791830

792831
$command->childNoInteractionOverride = false;
@@ -796,16 +835,18 @@ public function testCallStripsInheritedNoInteractionWhenCallerAllowsInteraction(
796835
$exitCode = $command->run([], []);
797836

798837
$this->assertSame(EXIT_SUCCESS, $exitCode);
838+
$this->assertTrue($command->isInteractive());
799839
$this->assertTrue(InteractiveStateProbeCommand::$interactCalled);
800840
$this->assertTrue(InteractiveStateProbeCommand::$observedInteractive);
801841
}
802842

843+
/**
844+
* When $noInteractionOverride is true and the caller already supplied the flag,
845+
* the resolver must not touch the caller's entry. The child still sees a
846+
* non-interactive state.
847+
*/
803848
public function testCallPreservesCallerFlagWhenForcingNonInteractive(): void
804849
{
805-
// When $noInteractionOverride is true and the caller already supplied the flag,
806-
// the resolver must not touch the caller's entry. The child still sees a
807-
// non-interactive state.
808-
809850
$command = new ParentCallsInteractFixtureCommand(new Commands());
810851

811852
$command->childNoInteractionOverride = true;
@@ -815,6 +856,7 @@ public function testCallPreservesCallerFlagWhenForcingNonInteractive(): void
815856
$exitCode = $command->run([], []);
816857

817858
$this->assertSame(EXIT_SUCCESS, $exitCode);
859+
$this->assertTrue($command->isInteractive());
818860
$this->assertFalse(InteractiveStateProbeCommand::$interactCalled);
819861
$this->assertFalse(InteractiveStateProbeCommand::$observedInteractive);
820862
}

0 commit comments

Comments
 (0)