Skip to content

Commit 01f3bc6

Browse files
paulbalandanpaulbalandan
committed
address reviews
1 parent a944b89 commit 01f3bc6

13 files changed

Lines changed: 379 additions & 42 deletions

File tree

‎system/CLI/AbstractCommand.php‎

Lines changed: 19 additions & 9 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

@@ -692,7 +696,7 @@ private function bind(array $arguments, array $options): array
692696

693697
// 4. If there are still options left that are not defined, we will mark them as extraneous.
694698
foreach ($options as $name => $value) {
695-
if (array_key_exists($name, $this->shortcuts)) {
699+
if ($this->hasShortcut($name)) {
696700
// This scenario can happen when the command has an array option with a shortcut,
697701
// and the shortcut is used alongside the long name, causing it to be not bound
698702
// in the previous loop.
@@ -707,7 +711,7 @@ private function bind(array $arguments, array $options): array
707711
continue;
708712
}
709713

710-
if (array_key_exists($name, $this->negations)) {
714+
if ($this->hasNegation($name)) {
711715
// This scenario can happen when the command has a negatable option,
712716
// and both the option and its negation are used, causing the negation
713717
// to be not bound in the previous loop.
@@ -816,8 +820,14 @@ private function validateOption(string $name, Option $definition, array|bool|str
816820
throw new OptionValueMismatchException(lang('Commands.nonArrayOptionWithArrayValue', [$name]));
817821
}
818822

819-
if ($definition->requiresValue && ! is_string($value) && ! is_array($value)) {
820-
throw new OptionValueMismatchException(lang('Commands.optionRequiresValue', [$name]));
823+
if ($definition->requiresValue) {
824+
$elements = is_array($value) ? $value : [$value];
825+
826+
foreach ($elements as $element) {
827+
if (! is_string($element)) {
828+
throw new OptionValueMismatchException(lang('Commands.optionRequiresValue', [$name]));
829+
}
830+
}
821831
}
822832

823833
if (! $definition->negatable || is_bool($value)) {
@@ -859,7 +869,7 @@ private function validateNegatableOption(string $name, Option $definition, array
859869
*/
860870
private function getOptionDefinitionFor(string $name): Option
861871
{
862-
if (! array_key_exists($name, $this->optionsDefinition)) {
872+
if (! $this->hasOption($name)) {
863873
throw new LogicException(sprintf('Option "%s" is not defined on this command.', $name));
864874
}
865875

‎system/CLI/Commands.php‎

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -217,11 +217,13 @@ public function discoverCommands()
217217

218218
foreach (array_keys(array_intersect_key($this->commands, $this->modernCommands)) as $name) {
219219
CLI::write(
220-
lang('Commands.duplicateCommandName', [
221-
$name,
222-
$this->commands[$name]['class'],
223-
$this->modernCommands[$name]['class'],
224-
]),
220+
CLI::wrap(
221+
lang('Commands.duplicateCommandName', [
222+
$name,
223+
$this->commands[$name]['class'],
224+
$this->modernCommands[$name]['class'],
225+
]),
226+
),
225227
'yellow',
226228
);
227229
}
@@ -248,7 +250,7 @@ public function verifyCommand(string $command, array $commands = [], bool $legac
248250

249251
$message = lang('CLI.commandNotFound', [$command]);
250252

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

253255
if ($alternatives !== []) {
254256
$message = sprintf(
@@ -265,24 +267,22 @@ public function verifyCommand(string $command, array $commands = [], bool $legac
265267
}
266268

267269
/**
268-
* Finds alternative of `$name` among collection of commands.
270+
* Finds alternative of `$name` across both legacy and modern commands.
269271
*
270272
* @param legacy_commands $collection (no longer used)
271273
*
272274
* @return list<string>
273275
*/
274-
protected function getCommandAlternatives(string $name, array $collection = [], bool $legacy = true): array
276+
protected function getCommandAlternatives(string $name, array $collection = []): array
275277
{
276278
if ($collection !== []) {
277279
@trigger_error(sprintf('Since v4.8.0, the $collection parameter of %s() is no longer used.', __METHOD__), E_USER_DEPRECATED);
278280
}
279281

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

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

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

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

‎system/Commands/Server/Serve.php‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -37,9 +37,9 @@ class Serve extends AbstractCommand
3737
protected function configure(): void
3838
{
3939
$this
40-
->addOption(new Option(name: 'php', description: 'The PHP binary to use.', acceptsValue: true, default: PHP_BINARY))
41-
->addOption(new Option(name: 'host', description: 'The host to serve on.', acceptsValue: true, default: 'localhost'))
42-
->addOption(new Option(name: 'port', description: 'The port to serve on.', acceptsValue: true, default: '8080'));
40+
->addOption(new Option(name: 'php', description: 'The PHP binary to use.', requiresValue: true, default: PHP_BINARY))
41+
->addOption(new Option(name: 'host', description: 'The host to serve on.', requiresValue: true, default: 'localhost'))
42+
->addOption(new Option(name: 'port', description: 'The port to serve on.', requiresValue: true, default: '8080'));
4343
}
4444

4545
protected function execute(array $arguments, array $options): int

‎system/Language/en/Commands.php‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@
2020
'arrayOptionEmptyArrayDefault' => 'Array option "--{0}" cannot have an empty array as the default value.',
2121
'argumentAfterArrayArgument' => 'Argument "{0}" cannot be defined after array argument "{1}".',
2222
'duplicateArgument' => 'An argument with the name "{0}" is already defined.',
23-
'duplicateCommandName' => 'Warning: command "{0}" is defined as both legacy ({1}) and modern ({2}); the legacy command will execute. Please rename or remove one.',
23+
'duplicateCommandName' => 'Warning: The "{0}" command is defined as both legacy ({1}) and modern ({2}). The legacy command will be executed. Please rename or remove one.',
2424
'duplicateOption' => 'An option with the name "--{0}" is already defined.',
2525
'duplicateShortcut' => 'Shortcut "-{0}" cannot be used for option "--{1}"; it is already assigned to option "--{2}".',
2626
'emptyCommandName' => 'Command name cannot be empty.',
@@ -47,6 +47,7 @@
4747
'noArgumentsExpected' => 'No arguments expected for "{0}" command. Received: "{1}".',
4848
'nonArrayArgumentWithArrayDefault' => 'Argument "{0}" does not accept an array default value.',
4949
'nonArrayOptionWithArrayValue' => 'Option "--{0}" does not accept an array value.',
50+
'optionClashesWithExistingNegation' => 'Option "--{0}" clashes with the negation of negatable option "--{1}".',
5051
'optionNoValueAndNoDefault' => 'Option "--{0}" does not accept a value and cannot have a default value.',
5152
'optionNotAcceptingValue' => 'Option "--{0}" does not accept a value.',
5253
'optionalArgumentNoDefault' => 'Argument "{0}" is optional and must have a default value.',

‎tests/system/CLI/AbstractCommandTest.php‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -193,6 +193,14 @@ public static function provideCollectionLevelOptionRegistrationIsRejected(): ite
193193
['name' => 'test', 'negatable' => true, 'default' => false],
194194
],
195195
];
196+
197+
yield 'option name clashes with existing negation' => [
198+
'Option "--no-test" clashes with the negation of negatable option "--test".',
199+
[
200+
['name' => 'test', 'negatable' => true, 'default' => false],
201+
['name' => 'no-test'],
202+
],
203+
];
196204
}
197205

198206
public function testRenderThrowable(): void
@@ -509,6 +517,24 @@ public static function provideValidationOfOptions(): iterable
509517
['foo' => null],
510518
];
511519

520+
yield 'array option requiring value passed without value [app:about a --baz]' => [
521+
OptionValueMismatchException::class,
522+
'Option "--baz" requires a value to be provided.',
523+
['baz' => null],
524+
];
525+
526+
yield 'array option requiring value passed without value multiple times [app:about a --baz --baz]' => [
527+
OptionValueMismatchException::class,
528+
'Option "--baz" requires a value to be provided.',
529+
['baz' => [null, null]],
530+
];
531+
532+
yield 'array option requiring value mixing value and no-value [app:about a --baz=v1 --baz]' => [
533+
OptionValueMismatchException::class,
534+
'Option "--baz" requires a value to be provided.',
535+
['baz' => ['v1', null]],
536+
];
537+
512538
yield 'option not accepting value passed with value [app:about a --no-header=value]' => [
513539
OptionValueMismatchException::class,
514540
'Option "--no-header" does not accept a value.',

‎tests/system/CLI/CommandsTest.php‎

Lines changed: 51 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -79,13 +79,14 @@ public function testRunOnUnknownCommand(): void
7979

8080
$this->assertSame(EXIT_ERROR, $commands->runLegacy('app:unknown', []));
8181
$this->assertArrayNotHasKey('app:unknown', $commands->getCommands());
82-
$this->assertStringContainsString('Command "app:unknown" not found', $this->getStreamFilterBuffer());
82+
$this->assertSame("\nCommand \"app:unknown\" not found.\n", $this->getUndecoratedBuffer());
8383

8484
$this->resetStreamFilterBuffer();
85+
CLI::resetLastWrite();
8586

8687
$this->assertSame(EXIT_ERROR, $commands->runCommand('app:unknown', [], []));
8788
$this->assertArrayNotHasKey('app:unknown', $commands->getModernCommands());
88-
$this->assertStringContainsString('Command "app:unknown" not found', $this->getStreamFilterBuffer());
89+
$this->assertSame("\nCommand \"app:unknown\" not found.\n", $this->getUndecoratedBuffer());
8990
}
9091

9192
public function testRunOnUnknownLegacyCommandButWithOneAlternative(): void
@@ -135,6 +136,7 @@ public function testRunOnUnknownLegacyCommandButWithMultipleAlternatives(): void
135136
Command "app:" not found.
136137
137138
Did you mean one of these?
139+
app:about
138140
app:destructive
139141
app:info
140142

@@ -143,6 +145,42 @@ public function testRunOnUnknownLegacyCommandButWithMultipleAlternatives(): void
143145
);
144146
}
145147

148+
public function testRunOnUnknownLegacyCommandAlsoSuggestsModernAlternatives(): void
149+
{
150+
$commands = new Commands();
151+
152+
$this->assertSame(EXIT_ERROR, $commands->runLegacy('app:ab', []));
153+
$this->assertSame(
154+
<<<'EOT'
155+
156+
Command "app:ab" not found.
157+
158+
Did you mean this?
159+
app:about
160+
161+
EOT,
162+
$this->getUndecoratedBuffer(),
163+
);
164+
}
165+
166+
public function testRunOnUnknownModernCommandAlsoSuggestsLegacyAlternatives(): void
167+
{
168+
$commands = new Commands();
169+
170+
$this->assertSame(EXIT_ERROR, $commands->runCommand('app:inf', [], []));
171+
$this->assertSame(
172+
<<<'EOT'
173+
174+
Command "app:inf" not found.
175+
176+
Did you mean this?
177+
app:info
178+
179+
EOT,
180+
$this->getUndecoratedBuffer(),
181+
);
182+
}
183+
146184
public function testRunOnUnknownModernCommandButWithMultipleAlternatives(): void
147185
{
148186
$commands = new Commands();
@@ -169,7 +207,7 @@ public function testRunOnAbstractLegacyCommandCannotBeRun(): void
169207

170208
$this->assertSame(EXIT_ERROR, $commands->runLegacy('app:pablo', []));
171209
$this->assertArrayNotHasKey('app:pablo', $commands->getCommands());
172-
$this->assertStringContainsString('Command "app:pablo" not found', $this->getStreamFilterBuffer());
210+
$this->assertSame("\nCommand \"app:pablo\" not found.\n", $this->getUndecoratedBuffer());
173211
}
174212

175213
public function testRunOnKnownLegacyCommand(): void
@@ -178,7 +216,10 @@ public function testRunOnKnownLegacyCommand(): void
178216

179217
$this->assertSame(EXIT_SUCCESS, $commands->runLegacy('app:info', []));
180218
$this->assertArrayHasKey('app:info', $commands->getCommands());
181-
$this->assertStringContainsString('CodeIgniter Version', $this->getStreamFilterBuffer());
219+
$this->assertSame(
220+
sprintf("\nCodeIgniter Version: %s\n", CodeIgniter::CI_VERSION),
221+
$this->getUndecoratedBuffer(),
222+
);
182223
}
183224

184225
public function testRunOnKnownModernCommand(): void
@@ -218,12 +259,14 @@ public function testDiscoveryWarnsWhenSameCommandNameExistsInBothRegistries(): v
218259
$this->copyCommand($modernFixture);
219260

220261
try {
262+
$message = wordwrap(
263+
'Warning: The "dup:test" command is defined as both legacy (App\\Commands\\DuplicateLegacy) and modern (App\\Commands\\DuplicateModern). The legacy command will be executed. Please rename or remove one.',
264+
CLI::getWidth(),
265+
);
266+
221267
$commands = new Commands();
222268

223-
$this->assertStringContainsString(
224-
'Warning: command "dup:test" is defined as both legacy (App\\Commands\\DuplicateLegacy) and modern (App\\Commands\\DuplicateModern)',
225-
$this->getUndecoratedBuffer(),
226-
);
269+
$this->assertSame("\n{$message}\n", $this->getUndecoratedBuffer());
227270
$this->assertArrayHasKey('dup:test', $commands->getCommands());
228271
$this->assertArrayHasKey('dup:test', $commands->getModernCommands());
229272
} finally {

‎tests/system/Commands/HelpCommandTest.php‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -173,6 +173,23 @@ public function testDescribeInexistentCommandButWithAlternatives(): void
173173
);
174174
}
175175

176+
public function testDescribeInexistentCommandSuggestsLegacyAlternatives(): void
177+
{
178+
command('help app:inf');
179+
180+
$this->assertSame(
181+
<<<'EOT'
182+
183+
Command "app:inf" not found.
184+
185+
Did you mean this?
186+
app:info
187+
188+
EOT,
189+
$this->getUndecoratedBuffer(),
190+
);
191+
}
192+
176193
public function testDescribeUsingHelpOption(): void
177194
{
178195
command('cache:clear --help');

0 commit comments

Comments
 (0)