From 8f71558cca257bdf52cb5a834b27467a3c1e152b Mon Sep 17 00:00:00 2001 From: "John Paul E. Balandan, CPA" Date: Fri, 14 May 2021 01:29:18 +0800 Subject: [PATCH] Optimize CommandRunner and Commands --- system/CLI/BaseCommand.php | 30 ++++----- system/CLI/CommandRunner.php | 15 +---- system/CLI/Commands.php | 38 +++++------ tests/system/CLI/CommandRunnerTest.php | 92 ++++++++++---------------- 4 files changed, 66 insertions(+), 109 deletions(-) diff --git a/system/CLI/BaseCommand.php b/system/CLI/BaseCommand.php index fc9a33b80f5b..54bf4c935366 100644 --- a/system/CLI/BaseCommand.php +++ b/system/CLI/BaseCommand.php @@ -16,7 +16,16 @@ use Throwable; /** - * Class BaseCommand + * BaseCommand is the base class used in creating CLI commands. + * + * @property string $group + * @property string $name + * @property string $usage + * @property string $description + * @property array $options + * @property array $arguments + * @property LoggerInterface $logger + * @property Commands $commands */ abstract class BaseCommand { @@ -78,7 +87,6 @@ abstract class BaseCommand */ protected $commands; - //-------------------------------------------------------------------- /** * BaseCommand constructor. * @@ -91,8 +99,6 @@ public function __construct(LoggerInterface $logger, Commands $commands) $this->commands = $commands; } - //-------------------------------------------------------------------- - /** * Actually execute a command. * This has to be over-ridden in any concrete implementation. @@ -101,8 +107,6 @@ public function __construct(LoggerInterface $logger, Commands $commands) */ abstract public function run(array $params); - //-------------------------------------------------------------------- - /** * Can be used by a command to run other commands. * @@ -117,8 +121,6 @@ protected function call(string $command, array $params = []) return $this->commands->run($command, $params); } - //-------------------------------------------------------------------- - /** * A simple method to display an error with line/file, in child commands. * @@ -132,8 +134,6 @@ protected function showError(Throwable $e) require APPPATH . 'Views/errors/cli/error_exception.php'; } - //-------------------------------------------------------------------- - /** * Show Help includes (Usage, Arguments, Description, Options). */ @@ -189,8 +189,6 @@ public function showHelp() } } - //-------------------------------------------------------------------- - /** * Pads our string out so that all titles are the same length to nicely line up descriptions. * @@ -208,8 +206,6 @@ public function setPad(string $item, int $max, int $extra = 2, int $indent = 0): return str_pad(str_repeat(' ', $indent) . $item, $max); } - //-------------------------------------------------------------------- - /** * Get pad for $key => $value array output * @@ -225,15 +221,15 @@ public function setPad(string $item, int $max, int $extra = 2, int $indent = 0): public function getPad(array $array, int $pad): int { $max = 0; + foreach (array_keys($array) as $key) { $max = max($max, strlen($key)); } + return $max + $pad; } - //-------------------------------------------------------------------- - /** * Makes it simple to access our protected properties. * @@ -251,8 +247,6 @@ public function __get(string $key) return null; } - //-------------------------------------------------------------------- - /** * Makes it simple to check our protected properties. * diff --git a/system/CLI/CommandRunner.php b/system/CLI/CommandRunner.php index 05763dd58e2e..e1643b60c035 100644 --- a/system/CLI/CommandRunner.php +++ b/system/CLI/CommandRunner.php @@ -21,14 +21,12 @@ class CommandRunner extends Controller { /** - * The Command Manager + * Instance of class managing the collection of commands * * @var Commands */ protected $commands; - //-------------------------------------------------------------------- - /** * Constructor */ @@ -58,8 +56,6 @@ public function _remap($method, ...$params) return $this->index($params); } - //-------------------------------------------------------------------- - /** * Default command. * @@ -70,14 +66,9 @@ public function _remap($method, ...$params) */ public function index(array $params) { - $command = array_shift($params); - - if (is_null($command)) - { - $command = 'list'; - } + $command = array_shift($params) ?? 'list'; - return service('commands')->run($command, $params); + return $this->commands->run($command, $params); } /** diff --git a/system/CLI/Commands.php b/system/CLI/Commands.php index 2b55710a9bdb..cdbbed0299c0 100644 --- a/system/CLI/Commands.php +++ b/system/CLI/Commands.php @@ -11,14 +11,12 @@ namespace CodeIgniter\CLI; +use CodeIgniter\Autoloader\FileLocator; use CodeIgniter\Log\Logger; -use Config\Services; use ReflectionClass; use ReflectionException; /** - * Class Commands - * * Core functionality for running, listing, etc commands. */ class Commands @@ -44,7 +42,8 @@ class Commands */ public function __construct($logger = null) { - $this->logger = $logger ?? Services::logger(); + $this->logger = $logger ?? service('logger'); + $this->discoverCommands(); } /** @@ -55,8 +54,6 @@ public function __construct($logger = null) */ public function run(string $command, array $params) { - $this->discoverCommands(); - if (! $this->verifyCommand($command, $this->commands)) { return; @@ -77,39 +74,39 @@ public function run(string $command, array $params) */ public function getCommands() { - $this->discoverCommands(); - return $this->commands; } /** * Discovers all commands in the framework and within user code, * and collects instances of them to work with. + * + * @return void */ public function discoverCommands() { - if (! empty($this->commands)) + if ($this->commands !== []) { return; } - $files = service('locator')->listFiles('Commands/'); + /** @var FileLocator $locator */ + $locator = service('locator'); + $files = $locator->listFiles('Commands/'); // If no matching command files were found, bail - if (empty($files)) + // This should never happen in unit testing. + if ($files === []) { - // This should never happen in unit testing. - // if it does, we have far bigger problems! - // @codeCoverageIgnoreStart - return; - // @codeCoverageIgnoreEnd + return; // @codeCoverageIgnore } // Loop over each file checking to see if a command with that - // alias exists in the class. If so, return it. Otherwise, try the next. + // alias exists in the class. foreach ($files as $file) { - $className = Services::locator()->findQualifiedNameFromPath($file); + $className = $locator->findQualifiedNameFromPath($file); + if (empty($className) || ! class_exists($className)) { continue; @@ -124,10 +121,10 @@ public function discoverCommands() continue; } + /** @var BaseCommand $class */ $class = new $className($this->logger, $this); - // Store it! - if (! is_null($class->group)) + if (isset($class->group)) { $this->commands[$class->name] = [ 'class' => $className, @@ -137,7 +134,6 @@ public function discoverCommands() ]; } - $class = null; unset($class); } catch (ReflectionException $e) diff --git a/tests/system/CLI/CommandRunnerTest.php b/tests/system/CLI/CommandRunnerTest.php index 305592d14c20..9f33fd7b99be 100644 --- a/tests/system/CLI/CommandRunnerTest.php +++ b/tests/system/CLI/CommandRunnerTest.php @@ -1,66 +1,49 @@ initController(service('request'), service('response'), self::$logger); + } protected function setUp(): void { parent::setUp(); CITestStreamFilter::$buffer = ''; - $this->stream_filter = stream_filter_append(STDOUT, 'CITestStreamFilter'); - - $this->env = new DotEnv(ROOTPATH); - $this->env->load(); - - // Set environment values that would otherwise stop the framework from functioning during tests. - if (! isset($_SERVER['app.baseURL'])) - { - $_SERVER['app.baseURL'] = 'http://example.com/'; - } - - $_SERVER['argv'] = [ - 'spark', - 'list', - ]; - $_SERVER['argc'] = 2; - CLI::init(); - - $this->config = new MockCLIConfig(); - $this->request = new IncomingRequest($this->config, new URI('https://somwhere.com'), null, new UserAgent()); - $this->response = new Response($this->config); - $this->logger = Services::logger(); - $this->runner = new CommandRunner(); - $this->runner->initController($this->request, $this->response, $this->logger); + + $this->streamFilter = stream_filter_append(STDOUT, 'CITestStreamFilter'); + $this->streamFilter = stream_filter_append(STDERR, 'CITestStreamFilter'); } public function tearDown(): void { - stream_filter_remove($this->stream_filter); + stream_filter_remove($this->streamFilter); } public function testGoodCommand() { - $this->runner->index(['list']); + self::$runner->index(['list']); $result = CITestStreamFilter::$buffer; // make sure the result looks like a command list @@ -70,7 +53,7 @@ public function testGoodCommand() public function testDefaultCommand() { - $this->runner->index([]); + self::$runner->index([]); $result = CITestStreamFilter::$buffer; // make sure the result looks like basic help @@ -80,7 +63,7 @@ public function testDefaultCommand() public function testHelpCommand() { - $this->runner->index(['help']); + self::$runner->index(['help']); $result = CITestStreamFilter::$buffer; // make sure the result looks like basic help @@ -90,7 +73,7 @@ public function testHelpCommand() public function testHelpCommandDetails() { - $this->runner->index(['help', 'session:migration']); + self::$runner->index(['help', 'session:migration']); $result = CITestStreamFilter::$buffer; // make sure the result looks like more detailed help @@ -101,42 +84,35 @@ public function testHelpCommandDetails() public function testCommandProperties() { - $this->runner->index(['help']); - $result = CITestStreamFilter::$buffer; - $commands = $this->runner->getCommands(); - $command = new $commands['help']['class']($this->logger, service('commands')); + $commands = self::$runner->getCommands(); + $command = new $commands['help']['class'](self::$logger, Services::commands()); - $this->assertEquals('Displays basic usage information.', $command->description); + $this->assertSame('Displays basic usage information.', $command->description); $this->assertNull($command->notdescription); } public function testEmptyCommand() { - $this->runner->index([null, 'list']); - $result = CITestStreamFilter::$buffer; + self::$runner->index([null, 'list']); // make sure the result looks like a command list - $this->assertStringContainsString('Lists the available commands.', $result); + $this->assertStringContainsString('Lists the available commands.', CITestStreamFilter::$buffer); } public function testBadCommand() { - $this->error_filter = stream_filter_append(STDERR, 'CITestStreamFilter'); - $this->runner->index(['bogus']); - $result = CITestStreamFilter::$buffer; - stream_filter_remove($this->error_filter); + self::$runner->index(['bogus']); // make sure the result looks like a command list - $this->assertStringContainsString('Command "bogus" not found', $result); + $this->assertStringContainsString('Command "bogus" not found', CITestStreamFilter::$buffer); } public function testRemapEmptyFirstParams() { - $this->runner->_remap('anyvalue', null, 'list'); + self::$runner->_remap('anyvalue', null, 'list'); $result = CITestStreamFilter::$buffer; // make sure the result looks like a command list $this->assertStringContainsString('Lists the available commands.', $result); } - }