From 1103a97ef330e6d907a4a5c375eecdc8c5e11fec Mon Sep 17 00:00:00 2001 From: Arif Hoque Date: Fri, 2 Oct 2026 23:58:09 +0600 Subject: [PATCH] Pool console: full signature syntax, original exceptions, and resilient command discovery --- src/Phaseolies/Console/Command.php | 41 ++- src/Phaseolies/Console/Schedule/Command.php | 139 +++++++-- tests/Console/CommandSignatureGrammarTest.php | 266 ++++++++++++++++++ 3 files changed, 429 insertions(+), 17 deletions(-) create mode 100644 tests/Console/CommandSignatureGrammarTest.php diff --git a/src/Phaseolies/Console/Command.php b/src/Phaseolies/Console/Command.php index e5e9a313..0445e496 100644 --- a/src/Phaseolies/Console/Command.php +++ b/src/Phaseolies/Console/Command.php @@ -2,6 +2,7 @@ namespace Phaseolies\Console; +use Symfony\Component\Console\Command\Command as SymfonyCommand; use RecursiveIteratorIterator; use RecursiveDirectoryIterator; use Phaseolies\Application; @@ -74,9 +75,47 @@ public function registerCommands(Console $console): void $commands = []; foreach ($commandClasses as $command) { - $commands[] = $this->app->make($command); + // One broken command must not take every other `pool` command down with it. + try { + if (!$this->isCommandClass($command)) { + continue; + } + + $commands[] = $this->app->make($command); + } catch (\Throwable $e) { + $this->reportSkippedCommand($command, $e); + } } $console->addCommands($commands); } + + /** + * Tell the user a command could not be loaded. + * + * @param string $class + * @param \Throwable $e + * @return void + */ + protected function reportSkippedCommand(string $class, \Throwable $e): void + { + fwrite(STDERR, sprintf("Skipped command [%s]: %s\n", $class, $e->getMessage())); + } + + /** + * Whether a discovered class is a concrete console command. Helpers, traits, + * interfaces and abstract base classes that live next to commands are ignored. + * + * @param string $class + * @return bool + */ + protected function isCommandClass(string $class): bool + { + if (!class_exists($class)) { + return false; + } + + return is_subclass_of($class, SymfonyCommand::class) + && !(new \ReflectionClass($class))->isAbstract(); + } } diff --git a/src/Phaseolies/Console/Schedule/Command.php b/src/Phaseolies/Console/Schedule/Command.php index 41e31bb3..6176baca 100644 --- a/src/Phaseolies/Console/Schedule/Command.php +++ b/src/Phaseolies/Console/Schedule/Command.php @@ -83,20 +83,10 @@ protected function parseSignature(): void $definition = trim($definition); - if (preg_match('/^(\w+)(\?)?$/', $definition, $m)) { - $this->addArgument( - $m[1], - !empty($m[2]) ? InputArgument::OPTIONAL : InputArgument::REQUIRED, - $description - ); - } elseif (preg_match('/^(?:-([a-zA-Z])\|)?--([\w-]+)(?:=(.*))?$/', $definition, $m)) { - $shortcut = $m[1] ?? null; - $name = $m[2]; - $default = $m[3] ?? null; - - $mode = $default !== null ? InputOption::VALUE_REQUIRED : InputOption::VALUE_NONE; - - $this->addOption($name, $shortcut, $mode, $description, $default); + if (str_starts_with($definition, '-')) { + $this->addOptionFromDefinition($definition, $description); + } else { + $this->addArgumentFromDefinition($definition, $description); } } @@ -105,6 +95,117 @@ protected function parseSignature(): void } } + /** + * Register an argument from its signature definition: + * + * name required + * name? optional + * name=guest optional, with a default + * name* required, accepts several values + * name?* optional, accepts several values + * + * @param string $definition + * @param string $description + * @return void + * @throws \LogicException + */ + protected function addArgumentFromDefinition(string $definition, string $description): void + { + if (!preg_match('/^(\w+)(\?)?(\*)?(?:=(.*))?$/s', $definition, $m)) { + throw $this->invalidDefinition($definition); + } + + $name = $m[1]; + $optional = !empty($m[2]); + $array = !empty($m[3]); + $hasDefault = isset($m[4]); + + if ($array && $hasDefault) { + throw $this->invalidDefinition($definition, 'an array argument cannot have a default'); + } + + $mode = ($optional || $hasDefault) ? InputArgument::OPTIONAL : InputArgument::REQUIRED; + + if ($array) { + $mode |= InputArgument::IS_ARRAY; + } + + $this->addArgument($name, $mode, $description, $hasDefault ? $m[4] : null); + } + + /** + * Register an option from its signature definition: + * + * --force flag + * -f|--force flag with a shortcut + * --queue= takes a value + * --queue=default takes a value, with a default + * --tag=* takes a value and can be repeated: --tag=a --tag=b + * --cache! negatable: --cache and --no-cache + * --cache!=true negatable, defaulting to true + * + * @param string $definition + * @param string $description + * @return void + * @throws \LogicException + */ + protected function addOptionFromDefinition(string $definition, string $description): void + { + if (!preg_match('/^(?:-([a-zA-Z])\|)?--([\w-]+)(!)?(?:=(.*))?$/s', $definition, $m)) { + throw $this->invalidDefinition($definition); + } + + $shortcut = $m[1] !== '' ? $m[1] : null; + $name = $m[2]; + $negatable = !empty($m[3]); + $default = $m[4] ?? null; + + if ($negatable) { + if ($default === null) { + $this->addOption($name, $shortcut, InputOption::VALUE_NONE | InputOption::VALUE_NEGATABLE, $description); + + return; + } + + if (!in_array($default, ['true', 'false'], true)) { + throw $this->invalidDefinition($definition, 'a negatable option can only default to true or false'); + } + + $this->addOption($name, $shortcut, InputOption::VALUE_NONE | InputOption::VALUE_NEGATABLE, $description, $default === 'true'); + + return; + } + + if ($default === null) { + $this->addOption($name, $shortcut, InputOption::VALUE_NONE, $description); + + return; + } + + if ($default === '*') { + $this->addOption($name, $shortcut, InputOption::VALUE_REQUIRED | InputOption::VALUE_IS_ARRAY, $description, []); + + return; + } + + $this->addOption($name, $shortcut, InputOption::VALUE_REQUIRED, $description, $default); + } + + /** + * @param string $definition + * @param string|null $reason + * @return \LogicException + */ + private function invalidDefinition(string $definition, ?string $reason = null): \LogicException + { + return new \LogicException(sprintf( + 'Invalid definition "{%s}" in the signature of [%s]%s.', + $definition, + static::class, + $reason ? ': ' . $reason : '' + )); + } + /** * Get the command name from name. * @@ -146,8 +247,14 @@ protected function execute(InputInterface $input, OutputInterface $output): int return is_int($result) ? $result : self::SUCCESS; } catch (\Throwable $e) { - Log::error($e); - throw new \Exception($e->getMessage()); + // Logging must never hide the error being logged. + try { + Log::error($e); + } catch (\Throwable) { + } + + // Rethrown as is, so the real class, code and origin are reported instead of this line. + throw $e; } } diff --git a/tests/Console/CommandSignatureGrammarTest.php b/tests/Console/CommandSignatureGrammarTest.php new file mode 100644 index 00000000..0972b35d --- /dev/null +++ b/tests/Console/CommandSignatureGrammarTest.php @@ -0,0 +1,266 @@ +seen = ['arguments' => $this->argument(), 'options' => $this->option()]; + + return 0; + } +} + +class CommandSignatureGrammarTest extends TestCase +{ + protected function setUp(): void + { + Container::setInstance(new MockContainer()); + } + + protected function tearDown(): void + { + (new \ReflectionClass(Container::class))->getProperty('instance')->setValue(null, null); + } + + private function command(string $signature): GrammarCommand + { + return new class($signature) extends GrammarCommand { + public function __construct(string $signature) + { + $this->name = $signature; + parent::__construct(); + } + }; + } + + private function run_(string $signature, array $input = []): GrammarCommand + { + $command = $this->command($signature); + (new CommandTester($command))->execute($input, ['interactive' => false]); + + return $command; + } + + public function testPlainArgumentsAndOptionsStillWork(): void + { + $command = $this->run_('plain {user} {nick?} {--force} {-F|--flag} {--queue=} {--mode=fast}', [ + 'user' => 'ada', '--force' => true, '--queue' => 'high', + ]); + + $this->assertSame('ada', $command->seen['arguments']['user']); + $this->assertNull($command->seen['arguments']['nick']); + $this->assertTrue($command->seen['options']['force']); + $this->assertFalse($command->seen['options']['flag']); + $this->assertSame('high', $command->seen['options']['queue']); + $this->assertSame('fast', $command->seen['options']['mode']); + $this->assertSame('F', $command->getDefinition()->getOption('flag')->getShortcut()); + } + + public function testArgumentDefault(): void + { + $this->assertSame('guest', $this->run_('a {name=guest}')->seen['arguments']['name']); + $this->assertSame('ada', $this->run_('a {name=guest}', ['name' => 'ada'])->seen['arguments']['name']); + $this->assertFalse($this->command('a {name=guest}')->getDefinition()->getArgument('name')->isRequired()); + } + + public function testRequiredArrayArgument(): void + { + $definition = $this->command('a {files*}')->getDefinition()->getArgument('files'); + + $this->assertTrue($definition->isArray()); + $this->assertTrue($definition->isRequired()); + $this->assertSame(['a.txt', 'b.txt'], $this->run_('a {files*}', ['files' => ['a.txt', 'b.txt']])->seen['arguments']['files']); + } + + public function testOptionalArrayArgument(): void + { + $this->assertSame([], $this->run_('a {files?*}')->seen['arguments']['files']); + $this->assertSame(['x'], $this->run_('a {files?*}', ['files' => ['x']])->seen['arguments']['files']); + } + + public function testArrayOption(): void + { + $definition = $this->command('a {--tag=*}')->getDefinition()->getOption('tag'); + + $this->assertTrue($definition->isArray()); + $this->assertSame([], $definition->getDefault()); + $this->assertSame([], $this->run_('a {--tag=*}')->seen['options']['tag']); + $this->assertSame(['x', 'y'], $this->run_('a {--tag=*}', ['--tag' => ['x', 'y']])->seen['options']['tag']); + } + + public function testArrayOptionWithShortcut(): void + { + $this->assertSame('t', $this->command('a {-t|--tag=*}')->getDefinition()->getOption('tag')->getShortcut()); + } + + public function testNegatableOption(): void + { + $definition = $this->command('a {--cache!}')->getDefinition()->getOption('cache'); + + $this->assertTrue($definition->isNegatable()); + $this->assertNull($this->run_('a {--cache!}')->seen['options']['cache']); + $this->assertTrue($this->run_('a {--cache!}', ['--cache' => true])->seen['options']['cache']); + $this->assertFalse($this->run_('a {--cache!}', ['--no-cache' => true])->seen['options']['cache']); + } + + public function testNegatableOptionWithBooleanDefault(): void + { + $this->assertTrue($this->run_('a {--cache!=true}')->seen['options']['cache']); + $this->assertFalse($this->run_('a {--cache!=true}', ['--no-cache' => true])->seen['options']['cache']); + $this->assertFalse($this->run_('a {--cache!=false}')->seen['options']['cache']); + $this->assertTrue($this->run_('a {--cache!=false}', ['--cache' => true])->seen['options']['cache']); + } + + public function testDescriptionsAreKept(): void + { + $definition = $this->command('a {name : Who to greet} {--tag=* : Labels to attach} {--cache! : Use the cache}')->getDefinition(); + + $this->assertSame('Who to greet', $definition->getArgument('name')->getDescription()); + $this->assertSame('Labels to attach', $definition->getOption('tag')->getDescription()); + $this->assertSame('Use the cache', $definition->getOption('cache')->getDescription()); + } + + public function testEverythingTogether(): void + { + $command = $this->run_('deploy {env=staging} {services?*} {--tag=*} {--cache!=true} {-F|--force}', [ + 'services' => ['api', 'web'], '--tag' => ['v1'], '--no-cache' => true, '--force' => true, + ]); + + $this->assertSame('staging', $command->seen['arguments']['env']); + $this->assertSame(['api', 'web'], $command->seen['arguments']['services']); + $this->assertSame(['v1'], $command->seen['options']['tag']); + $this->assertFalse($command->seen['options']['cache']); + $this->assertTrue($command->seen['options']['force']); + } + + #[DataProvider('invalidDefinitions')] + public function testInvalidDefinitionsFailLoudlyInsteadOfBeingDropped(string $signature, string $message): void + { + $this->expectException(\LogicException::class); + $this->expectExceptionMessage($message); + + $this->command($signature); + } + + public static function invalidDefinitions(): array + { + return [ + 'junk argument' => ['a {not valid}', 'Invalid definition "{not valid}"'], + 'junk option' => ['a {--bad option}', 'Invalid definition "{--bad option}"'], + 'array with a default' => ['a {files*=x}', 'an array argument cannot have a default'], + 'negatable with a value' => ['a {--cache!=maybe}', 'can only default to true or false'], + 'single dash long name' => ['a {-force}', 'Invalid definition "{-force}"'], + ]; + } + + public function testSymfonyOrderingRulesStillApply(): void + { + $this->expectException(\Symfony\Component\Console\Exception\LogicException::class); + + $this->command('a {files*} {after}'); + } + + public function testTheOriginalExceptionIsRethrownNotAGenericOne(): void + { + $original = new \DomainException('the real problem', 7); + + $command = new class($original) extends Command { + public function __construct(private \Throwable $error) + { + $this->name = 'fail'; + parent::__construct(); + } + + public function handle(): int + { + throw $this->error; + } + }; + + try { + (new CommandTester($command))->execute([]); + $this->fail('Expected the exception to propagate'); + } catch (\Throwable $e) { + $this->assertSame($original, $e, 'class, code and trace must survive'); + } + } + + public function testAFailingLoggerDoesNotHideTheRealError(): void + { + // The test container has no `log` binding, so Log::error() itself throws. + $command = new class extends Command { + protected $name = 'fail'; + + public function handle(): int + { + throw new \RuntimeException('the real problem'); + } + }; + + $this->expectException(\RuntimeException::class); + $this->expectExceptionMessage('the real problem'); + + (new CommandTester($command))->execute([]); + } + + public function testIsCommandClassIgnoresHelpersAbstractsTraitsAndInterfaces(): void + { + $loader = new class($this->createStub(Application::class)) extends CommandLoader { + public function check(string $class): bool + { + return $this->isCommandClass($class); + } + }; + + $this->assertTrue($loader->check(\Phaseolies\Console\Commands\Server\ServerStartCommand::class)); + $this->assertFalse($loader->check(GrammarCommand::class), 'abstract'); + $this->assertFalse($loader->check(\Phaseolies\Console\Support\InteractsWithMigrations::class), 'trait'); + $this->assertFalse($loader->check(\Phaseolies\Console\Console::class), 'not a Command'); + $this->assertFalse($loader->check(\Stringable::class), 'interface'); + $this->assertFalse($loader->check('App\\Does\\Not\\Exist'), 'missing'); + } + + public function testOneBrokenCommandDoesNotStopTheOthersFromLoading(): void + { + $broken = \Phaseolies\Console\Commands\Server\ServerStartCommand::class; + + $app = $this->createStub(Application::class); + $app->method('make')->willReturnCallback(function (string $class) use ($broken) { + if ($class === $broken) { + throw new \RuntimeException('cannot be built'); + } + + return new SymfonyCommand('stub:' . md5($class)); + }); + + $loader = new class($app) extends CommandLoader { + public array $skipped = []; + + protected function reportSkippedCommand(string $class, \Throwable $e): void + { + $this->skipped[$class] = $e->getMessage(); + } + }; + + $console = new Console($app); + $loader->registerCommands($console); + + $this->assertSame([$broken => 'cannot be built'], $loader->skipped); + $this->assertGreaterThan(40, count($console->all()), 'the other commands are still registered'); + } +}