Skip to content

Commit 48c1ad3

Browse files
paulbalandanpaulbalandan
committed
address review
1 parent 503f449 commit 48c1ad3

2 files changed

Lines changed: 92 additions & 55 deletions

File tree

‎system/CLI/AbstractCommand.php‎

Lines changed: 39 additions & 44 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
{
@@ -390,22 +390,20 @@ public function setInteractive(bool $interactive): static
390390
*
391391
* The lifecycle is:
392392
*
393-
* 1. {@see initialize()} and {@see interact()} are handed the raw parsed
394-
* input by reference, in that order. Both can mutate the tokens before
395-
* the framework interprets them against the declared definitions.
396-
* 2. The post-hook input is snapshotted into `$unboundArguments` and
397-
* `$unboundOptions` so the unbound accessors can report the tokens
398-
* carried into binding (as opposed to what defaults resolved to).
399-
* Any mutations performed in `initialize()` or `interact()` are
400-
* therefore reflected in the snapshot.
401-
* 3. {@see bind()} maps the raw tokens onto the declared arguments and
402-
* options, applying defaults and coercing flag/negation values.
403-
* 4. {@see validate()} rejects the bound result if it violates any of the
404-
* declarations — missing required argument, unknown option, value/flag
405-
* mismatches, and so on.
406-
* 5. The bound-and-validated values are snapshotted into
407-
* `$validatedArguments` / `$validatedOptions` and then passed to
408-
* {@see execute()}, whose integer return is the command's exit code.
393+
* 1. `initialize()` and `interact()` are handed the raw parsed input by reference, in that order.
394+
* Both can mutate the tokens before the framework interprets them against the declared definitions.
395+
* Note: the per-run interactive state is captured from `$options` before `initialize()` runs, so
396+
* mutating `--no-interaction` from within `initialize()` will not affect this invocation. Use
397+
* `setInteractive()` instead.
398+
* 2. The post-hook input is snapshotted into `$unboundArguments` and `$unboundOptions` so the unbound
399+
* accessors can report the tokens carried into binding (as opposed to what defaults resolved to).
400+
* Any mutations performed in `initialize()` or `interact()` are therefore reflected in the snapshot.
401+
* 3. `bind()` maps the raw tokens onto the declared arguments and options, applying defaults and
402+
* coercing flag/negation values.
403+
* 4. `validate()` rejects the bound result if it violates any of the declarations — missing required
404+
* argument, unknown option, value/flag mismatches, and so on.
405+
* 5. The bound-and-validated values are snapshotted into `$validatedArguments` / `$validatedOptions`
406+
* and then passed to `execute()`, whose integer return is the command's exit code.
409407
*
410408
* @param list<string> $arguments Parsed arguments from command line.
411409
* @param array<string, list<string|null>|string|null> $options Parsed options from command line.
@@ -417,9 +415,8 @@ public function setInteractive(bool $interactive): static
417415
*/
418416
final public function run(array $arguments, array $options): int
419417
{
420-
if ($this->interactive === null && $this->hasUnboundOption('no-interaction', $options)) {
421-
$this->interactive = false;
422-
}
418+
// Reset per-run interactive state from the current options.
419+
$this->runtimeInteractive = $this->hasUnboundOption('no-interaction', $options) ? false : null;
423420

424421
$this->initialize($arguments, $options);
425422

@@ -542,13 +539,11 @@ protected function getUnboundOptions(): array
542539
}
543540

544541
/**
545-
* Reads the raw (unbound) value of the option with the given declared name,
546-
* resolving through its shortcut and negation. Returns `null` when the
547-
* option was not provided under any of those aliases.
542+
* Reads the raw (unbound) value of the option with the given declared name, resolving through its
543+
* shortcut and negation. Returns `null` when the option was not provided under any of those aliases.
548544
*
549-
* Inside {@see interact()}, pass the `$options` parameter explicitly because
550-
* the instance state is not yet populated at that point. Elsewhere, omit
551-
* `$options` to read from the instance state.
545+
* Inside `interact()`, pass the `$options` parameter explicitly because the instance state is not yet
546+
* populated at that point. Elsewhere, omit `$options` to read from the instance state.
552547
*
553548
* @param array<string, list<string|null>|string|null>|null $options
554549
*
@@ -580,11 +575,11 @@ protected function getUnboundOption(string $name, ?array $options = null): array
580575
}
581576

582577
/**
583-
* Returns whether the option with the given declared name was provided in
584-
* the raw (unbound) input — under its long name, shortcut, or negation.
578+
* Returns whether the option with the given declared name was provided in the raw (unbound) input —
579+
* under its long name, shortcut, or negation.
585580
*
586-
* Inside {@see interact()}, pass the `$options` parameter explicitly; elsewhere
587-
* omit it to read from instance state.
581+
* Inside `interact()`, pass the `$options` parameter explicitly; elsewhere omit it to read from
582+
* instance state.
588583
*
589584
* @param array<string, list<string|null>|string|null>|null $options
590585
*

‎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)