Skip to content

Commit a00cb58

Browse files
paulbalandanpaulbalandan
committed
address reviews
1 parent 9d3695e commit a00cb58

20 files changed

Lines changed: 624 additions & 144 deletions

File tree

‎system/CLI/AbstractCommand.php‎

Lines changed: 51 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -217,7 +217,7 @@ public function addArgument(Argument $argument): static
217217
{
218218
$name = $argument->name;
219219

220-
if (array_key_exists($name, $this->argumentsDefinition)) {
220+
if ($this->hasArgument($name)) {
221221
throw new InvalidArgumentDefinitionException(lang('Commands.duplicateArgument', [$name]));
222222
}
223223

@@ -253,15 +253,19 @@ public function addOption(Option $option): static
253253
{
254254
$name = $option->name;
255255

256-
if (array_key_exists($name, $this->optionsDefinition)) {
256+
if ($this->hasOption($name)) {
257257
throw new InvalidOptionDefinitionException(lang('Commands.duplicateOption', [$name]));
258258
}
259259

260-
if ($option->shortcut !== null && array_key_exists($option->shortcut, $this->shortcuts)) {
260+
if ($this->hasNegation($name)) {
261+
throw new InvalidOptionDefinitionException(lang('Commands.optionClashesWithExistingNegation', [$name, $this->negations[$name]]));
262+
}
263+
264+
if ($option->shortcut !== null && $this->hasShortcut($option->shortcut)) {
261265
throw new InvalidOptionDefinitionException(lang('Commands.duplicateShortcut', [$option->shortcut, $name, $this->shortcuts[$option->shortcut]]));
262266
}
263267

264-
if ($option->negation !== null && array_key_exists($option->negation, $this->optionsDefinition)) {
268+
if ($option->negation !== null && $this->hasOption($option->negation)) {
265269
throw new InvalidOptionDefinitionException(lang('Commands.negatableOptionNegationExists', [$name]));
266270
}
267271

@@ -347,9 +351,11 @@ public function hasNegation(string $name): bool
347351
* 1. {@see initialize()} and {@see interact()} are handed the raw parsed
348352
* input by reference, in that order. Both can mutate the tokens before
349353
* the framework interprets them against the declared definitions.
350-
* 2. The resulting raw input is snapshotted into `$unboundArguments` and
351-
* `$unboundOptions` so the unbound accessors can report what the user
352-
* actually typed (as opposed to what defaults resolved to).
354+
* 2. The post-hook input is snapshotted into `$unboundArguments` and
355+
* `$unboundOptions` so the unbound accessors can report the tokens
356+
* carried into binding (as opposed to what defaults resolved to).
357+
* Any mutations performed in `initialize()` or `interact()` are
358+
* therefore reflected in the snapshot.
353359
* 3. {@see bind()} maps the raw tokens onto the declared arguments and
354360
* options, applying defaults and coercing flag/negation values.
355361
* 4. {@see validate()} rejects the bound result if it violates any of the
@@ -485,30 +491,31 @@ protected function getUnboundOptions(): array
485491

486492
/**
487493
* Reads the raw (unbound) value of the option with the given declared name,
488-
* resolving through its shortcut and negation. Returns `$default` when the
494+
* resolving through its shortcut and negation. Returns `null` when the
489495
* option was not provided under any of those aliases.
490496
*
491497
* Inside {@see interact()}, pass the `$options` parameter explicitly because
492498
* the instance state is not yet populated at that point. Elsewhere, omit
493499
* `$options` to read from the instance state.
494500
*
495501
* @param array<string, list<string|null>|string|null>|null $options
496-
* @param list<string|null>|string|null $default
497502
*
498503
* @return list<string|null>|string|null
499504
*
500505
* @throws LogicException
501506
*/
502-
protected function getUnboundOption(string $name, ?array $options = null, array|string|null $default = null): array|string|null
507+
protected function getUnboundOption(string $name, ?array $options = null): array|string|null
503508
{
504-
$definition = $this->getOptionDefinitionFor($name);
509+
$this->assertOptionIsDefined($name);
505510

506511
$options ??= $this->unboundOptions;
507512

508513
if (array_key_exists($name, $options)) {
509514
return $options[$name];
510515
}
511516

517+
$definition = $this->optionsDefinition[$name];
518+
512519
if ($definition->shortcut !== null && array_key_exists($definition->shortcut, $options)) {
513520
return $options[$definition->shortcut];
514521
}
@@ -517,7 +524,7 @@ protected function getUnboundOption(string $name, ?array $options = null, array|
517524
return $options[$definition->negation];
518525
}
519526

520-
return $default;
527+
return null;
521528
}
522529

523530
/**
@@ -533,14 +540,16 @@ protected function getUnboundOption(string $name, ?array $options = null, array|
533540
*/
534541
protected function hasUnboundOption(string $name, ?array $options = null): bool
535542
{
536-
$definition = $this->getOptionDefinitionFor($name);
543+
$this->assertOptionIsDefined($name);
537544

538545
$options ??= $this->unboundOptions;
539546

540547
if (array_key_exists($name, $options)) {
541548
return true;
542549
}
543550

551+
$definition = $this->optionsDefinition[$name];
552+
544553
if ($definition->shortcut !== null && array_key_exists($definition->shortcut, $options)) {
545554
return true;
546555
}
@@ -692,33 +701,38 @@ private function bind(array $arguments, array $options): array
692701

693702
// 4. If there are still options left that are not defined, we will mark them as extraneous.
694703
foreach ($options as $name => $value) {
695-
if (array_key_exists($name, $this->shortcuts)) {
704+
if ($this->hasShortcut($name)) {
696705
// This scenario can happen when the command has an array option with a shortcut,
697706
// and the shortcut is used alongside the long name, causing it to be not bound
698-
// in the previous loop.
707+
// in the previous loop. The leftover shortcut value can itself be an array when
708+
// the shortcut was passed multiple times, so merge arrays and append scalars.
699709
$option = $this->shortcuts[$name];
710+
$values = is_array($value) ? $value : [$value];
700711

701712
if (array_key_exists($option, $boundOptions) && is_array($boundOptions[$option])) {
702-
$boundOptions[$option][] = $value;
713+
$boundOptions[$option] = [...$boundOptions[$option], ...$values];
703714
} else {
704-
$boundOptions[$option] = [$boundOptions[$option], $value];
715+
$boundOptions[$option] = [$boundOptions[$option], ...$values];
705716
}
706717

707718
continue;
708719
}
709720

710-
if (array_key_exists($name, $this->negations)) {
721+
if ($this->hasNegation($name)) {
711722
// This scenario can happen when the command has a negatable option,
712723
// and both the option and its negation are used, causing the negation
713-
// to be not bound in the previous loop.
724+
// to be not bound in the previous loop. The leftover negation value can
725+
// be scalar (including a string when the negation was passed with a value)
726+
// or an array — normalise to an array before mapping null → false.
714727
$option = $this->negations[$name];
715-
$value = array_map(static fn (mixed $v): mixed => $v ?? false, $value ?? [null]);
728+
$values = is_array($value) ? $value : [$value];
729+
$values = array_map(static fn (mixed $v): mixed => $v ?? false, $values);
716730

717731
if (! is_array($boundOptions[$option])) {
718732
$boundOptions[$option] = [$boundOptions[$option]];
719733
}
720734

721-
$boundOptions[$option] = [...$boundOptions[$option], ...$value];
735+
$boundOptions[$option] = [...$boundOptions[$option], ...$values];
722736

723737
continue;
724738
}
@@ -816,8 +830,14 @@ private function validateOption(string $name, Option $definition, array|bool|str
816830
throw new OptionValueMismatchException(lang('Commands.nonArrayOptionWithArrayValue', [$name]));
817831
}
818832

819-
if ($definition->requiresValue && ! is_string($value) && ! is_array($value)) {
820-
throw new OptionValueMismatchException(lang('Commands.optionRequiresValue', [$name]));
833+
if ($definition->requiresValue) {
834+
$elements = is_array($value) ? $value : [$value];
835+
836+
foreach ($elements as $element) {
837+
if (! is_string($element)) {
838+
throw new OptionValueMismatchException(lang('Commands.optionRequiresValue', [$name]));
839+
}
840+
}
821841
}
822842

823843
if (! $definition->negatable || is_bool($value)) {
@@ -843,7 +863,12 @@ private function validateNegatableOption(string $name, Option $definition, array
843863
throw new OptionValueMismatchException(lang('Commands.negatedOptionNoValue', [$definition->negation]));
844864
}
845865

846-
if (array_values(array_intersect(array_unique($value), [true, false])) === [true, false]) {
866+
// Both forms appearing together is the primary user mistake; flag it
867+
// regardless of whether either form carried a value.
868+
if (
869+
array_key_exists($name, $this->unboundOptions)
870+
&& array_key_exists($definition->negation, $this->unboundOptions)
871+
) {
847872
throw new LogicException(lang('Commands.negatableOptionWithNegation', [$name, $definition->negation]));
848873
}
849874

@@ -857,13 +882,11 @@ private function validateNegatableOption(string $name, Option $definition, array
857882
/**
858883
* @throws LogicException
859884
*/
860-
private function getOptionDefinitionFor(string $name): Option
885+
private function assertOptionIsDefined(string $name): void
861886
{
862-
if (! array_key_exists($name, $this->optionsDefinition)) {
887+
if (! $this->hasOption($name)) {
863888
throw new LogicException(sprintf('Option "%s" is not defined on this command.', $name));
864889
}
865-
866-
return $this->optionsDefinition[$name];
867890
}
868891

869892
/**

‎system/CLI/Commands.php‎

Lines changed: 33 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -96,7 +96,7 @@ public function runLegacy(string $command, array $params): int
9696

9797
Events::trigger('pre_command');
9898

99-
$exitCode = $this->getCommand($command)->run($params);
99+
$exitCode = $this->getCommand($command, legacy: true)->run($params);
100100

101101
Events::trigger('post_command');
102102

@@ -153,12 +153,32 @@ public function getModernCommands(): array
153153
return $this->modernCommands;
154154
}
155155

156+
/**
157+
* Checks if a legacy command with the given name has been discovered.
158+
*/
159+
public function hasLegacyCommand(string $name): bool
160+
{
161+
return array_key_exists($name, $this->commands);
162+
}
163+
164+
/**
165+
* Checks if a modern command with the given name has been discovered.
166+
*
167+
* A name present in both registries signals a collision; legacy wins
168+
* at runtime. Callers can combine this with {@see hasLegacyCommand()}
169+
* to detect that case.
170+
*/
171+
public function hasModernCommand(string $name): bool
172+
{
173+
return array_key_exists($name, $this->modernCommands);
174+
}
175+
156176
/**
157177
* @return ($legacy is true ? BaseCommand : AbstractCommand)
158178
*
159179
* @throws CommandNotFoundException
160180
*/
161-
public function getCommand(string $command, bool $legacy = true): AbstractCommand|BaseCommand
181+
public function getCommand(string $command, bool $legacy = false): AbstractCommand|BaseCommand
162182
{
163183
if ($legacy && isset($this->commands[$command])) {
164184
$className = $this->commands[$command]['class'];
@@ -217,11 +237,13 @@ public function discoverCommands()
217237

218238
foreach (array_keys(array_intersect_key($this->commands, $this->modernCommands)) as $name) {
219239
CLI::write(
220-
lang('Commands.duplicateCommandName', [
221-
$name,
222-
$this->commands[$name]['class'],
223-
$this->modernCommands[$name]['class'],
224-
]),
240+
CLI::wrap(
241+
lang('Commands.duplicateCommandName', [
242+
$name,
243+
$this->commands[$name]['class'],
244+
$this->modernCommands[$name]['class'],
245+
]),
246+
),
225247
'yellow',
226248
);
227249
}
@@ -248,7 +270,7 @@ public function verifyCommand(string $command, array $commands = [], bool $legac
248270

249271
$message = lang('CLI.commandNotFound', [$command]);
250272

251-
$alternatives = $this->getCommandAlternatives($command, legacy: $legacy);
273+
$alternatives = $this->getCommandAlternatives($command);
252274

253275
if ($alternatives !== []) {
254276
$message = sprintf(
@@ -265,24 +287,22 @@ public function verifyCommand(string $command, array $commands = [], bool $legac
265287
}
266288

267289
/**
268-
* Finds alternative of `$name` among collection of commands.
290+
* Finds alternative of `$name` across both legacy and modern commands.
269291
*
270292
* @param legacy_commands $collection (no longer used)
271293
*
272294
* @return list<string>
273295
*/
274-
protected function getCommandAlternatives(string $name, array $collection = [], bool $legacy = true): array
296+
protected function getCommandAlternatives(string $name, array $collection = []): array
275297
{
276298
if ($collection !== []) {
277299
@trigger_error(sprintf('Since v4.8.0, the $collection parameter of %s() is no longer used.', __METHOD__), E_USER_DEPRECATED);
278300
}
279301

280-
$commandCollection = $legacy ? $this->commands : $this->modernCommands;
281-
282302
/** @var array<string, int> */
283303
$alternatives = [];
284304

285-
foreach (array_keys($commandCollection) as $commandName) {
305+
foreach (array_keys($this->commands + $this->modernCommands) as $commandName) {
286306
$lev = levenshtein($name, $commandName);
287307

288308
if ($lev <= strlen($commandName) / 3 || str_contains($commandName, $name)) {

‎system/CLI/Console.php‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -69,8 +69,11 @@ public function run(array $tokens = [])
6969

7070
$this->command = array_shift($arguments) ?? self::DEFAULT_COMMAND;
7171

72-
if (array_key_exists($this->command, $commands->getCommands())) {
73-
return $commands->runLegacy($this->command, array_merge($arguments, $this->options));
72+
if ($commands->hasLegacyCommand($this->command)) {
73+
$legacyOptions = $this->options;
74+
unset($legacyOptions['no-header']);
75+
76+
return $commands->runLegacy($this->command, array_merge($arguments, $legacyOptions));
7477
}
7578

7679
return $commands->runCommand($this->command, $arguments, $this->options);

‎system/Commands/Help.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ protected function execute(array $arguments, array $options): int
4141
$commands = $this->getCommandRunner();
4242

4343
if (array_key_exists($command, $commands->getCommands())) {
44-
$commands->getCommand($command)->showHelp();
44+
$commands->getCommand($command, legacy: true)->showHelp();
4545

4646
return EXIT_SUCCESS;
4747
}

‎system/Commands/ListCommands.php‎

Lines changed: 11 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -43,10 +43,11 @@ protected function execute(array $arguments, array $options): int
4343

4444
private function describeCommandsSimple(): int
4545
{
46-
$commands = [
47-
...array_keys($this->getCommandRunner()->getCommands()),
48-
...array_keys($this->getCommandRunner()->getModernCommands()),
49-
];
46+
// Legacy takes precedence on key collision so the listing reflects the
47+
// command that would actually be invoked.
48+
$commands = array_keys(
49+
$this->getCommandRunner()->getCommands() + $this->getCommandRunner()->getModernCommands(),
50+
);
5051
sort($commands);
5152

5253
foreach ($commands as $command) {
@@ -64,10 +65,11 @@ private function describeCommandsDetailed(): int
6465
$entries = [];
6566
$maxPad = 0;
6667

67-
foreach ([
68-
...$this->getCommandRunner()->getCommands(),
69-
...$this->getCommandRunner()->getModernCommands(),
70-
] as $command => $details) {
68+
// Legacy takes precedence on key collision so the listing reflects the
69+
// command that would actually be invoked.
70+
$all = $this->getCommandRunner()->getCommands() + $this->getCommandRunner()->getModernCommands();
71+
72+
foreach ($all as $command => $details) {
7173
$maxPad = max($maxPad, strlen($command) + 4);
7274

7375
$entries[] = [$details['group'], $command, $details['description']];
@@ -105,7 +107,7 @@ private function describeCommandsDetailed(): int
105107
CLI::write(sprintf(
106108
'%s%s',
107109
CLI::color($this->addPadding($command[0], 2, $maxPad), 'green'),
108-
CLI::wrap($command[1]),
110+
CLI::wrap($command[1], 0, $maxPad),
109111
));
110112
}
111113
}

0 commit comments

Comments
 (0)