Handle subcommands in fewer places.

Rework Shell::runCommand() so it doesn't require parameters to be duplicated. This has the side benefit of making option parsing have a simpler interface. Update related tests, and code. CompletionShell had a silly parser definition that I've corrected. If a command's option parser fails, the error is now shown before the help message. Hopefully this gives the end user some idea on what they need to change when trying to call the shell. This change continues to keep sketchy logic around for task's being called through execute(). I'm going to address this in my next set of changes.
cakephp · Apr 24, 2014 · 3e2282c · 3e2282c
1 parent 96bd2c6
commit 3e2282c
Show file tree

Hide file tree

Showing 8 changed files with 46 additions and 49 deletions.
diff --git a/src/Console/Command/CompletionShell.php b/src/Console/Command/CompletionShell.php
@@ -107,8 +107,6 @@ public function getOptionParser() {
 			'help' => __d('cake_console', 'Output a list of available commands'),
 			'parser' => [
 				'description' => __d('cake_console', 'List all availables'),
-				'arguments' => [
-				]
 			]
 		])->addSubcommand('subcommands', [
 			'help' => __d('cake_console', 'Output a list of available subcommands'),
@@ -117,7 +115,7 @@ public function getOptionParser() {
 				'arguments' => [
 					'command' => [
 						'help' => __d('cake_console', 'The command name'),
-						'required' => true,
+						'required' => false,
 					]
 				]
 			]

diff --git a/src/Console/ConsoleOptionParser.php b/src/Console/ConsoleOptionParser.php
@@ -463,9 +463,12 @@ public function subcommands() {
  * @return Array [$params, $args]
  * @throws \Cake\Console\Error\ConsoleException When an invalid parameter is encountered.
  */
-	public function parse($argv, $command = null) {
-		if (isset($this->_subcommands[$command]) && $this->_subcommands[$command]->parser()) {
+	public function parse($argv) {
+		$command = isset($argv[0]) ? $argv[0] : null;
+		if (isset($this->_subcommands[$command])) {
 			array_shift($argv);
+		}
+		if (isset($this->_subcommands[$command]) && $this->_subcommands[$command]->parser()) {
 			return $this->_subcommands[$command]->parser()->parse($argv);
 		}
 		$params = $args = [];

diff --git a/src/Console/Shell.php b/src/Console/Shell.php
@@ -338,11 +338,11 @@ public function dispatchShell() {
  * @return void
  * @link http://book.cakephp.org/2.0/en/console-and-shells.html#Shell::runCommand
  */
-	public function runCommand($command, $argv) {
-
+	public function runCommand($argv) {
+		$command = isset($argv[0]) ? $argv[0] : null;
 		$this->OptionParser = $this->getOptionParser();
 		try {
-			list($this->params, $this->args) = $this->OptionParser->parse($argv, $command);
+			list($this->params, $this->args) = $this->OptionParser->parse($argv);
 		} catch (Error\ConsoleException $e) {
 			$this->err('<error>Error: ' . $e->getMessage() . '</error>');
 			$this->out($this->OptionParser->help($command));
@@ -367,21 +367,22 @@ public function runCommand($command, $argv) {
 		$subcommands = $this->OptionParser->subcommands();
 		$isMethod = $this->hasMethod($command);
 
-		if (
-			$isMethod &&
-			(count($subcommands) === 0 || isset($subcommands[$command]))
-		) {
+		if ($isMethod && count($subcommands) === 0) {
 			array_shift($this->args);
 			$this->startup();
 			return call_user_func_array([$this, $command], $this->args);
 		}
 
+		if ($isMethod && isset($subcommands[$command])) {
+			$this->startup();
+			return call_user_func_array([$this, $command], $this->args);
+		}
+
 		if ($this->hasTask($command) && isset($subcommands[$command])) {
-			array_shift($argv);
 			$this->startup();
 			$command = Inflector::camelize($command);
-			// TODO this is suspicious.
-			return $this->{$command}->runCommand('execute', $argv);
+			$argv[0] = 'execute';
+			return $this->{$command}->runCommand($argv);
 		}
 
 		if ($this->hasMethod('main')) {

diff --git a/src/Console/ShellDispatcher.php b/src/Console/ShellDispatcher.php
@@ -159,13 +159,8 @@ protected function _dispatch() {
 
 		$Shell = $this->findShell($shell);
 
-		$command = null;
-		if (isset($this->args[0])) {
-			$command = $this->args[0];
-		}
-
 		$Shell->initialize();
-		return $Shell->runCommand($command, $this->args);
+		return $Shell->runCommand($this->args);
 	}
 
 /**

diff --git a/tests/TestCase/Console/Command/CompletionShellTest.php b/tests/TestCase/Console/Command/CompletionShellTest.php
@@ -83,7 +83,7 @@ public function tearDown() {
  * @return void
  */
 	public function testStartup() {
-		$this->Shell->runCommand('main', array());
+		$this->Shell->runCommand(['main']);
 		$output = $this->out->output;
 
 		$needle = 'Welcome to CakePHP';
@@ -96,7 +96,7 @@ public function testStartup() {
  * @return void
  */
 	public function testMain() {
-		$this->Shell->runCommand('main', array());
+		$this->Shell->runCommand(['main']);
 		$output = $this->out->output;
 
 		$expected = "/This command is not intended to be called manually/";
@@ -109,7 +109,7 @@ public function testMain() {
  * @return void
  */
 	public function testCommands() {
-		$this->Shell->runCommand('commands', array());
+		$this->Shell->runCommand(['commands']);
 		$output = $this->out->output;
 
 		$expected = "TestPlugin.example TestPluginTwo.example TestPluginTwo.welcome bake i18n server test sample\n";
@@ -122,7 +122,7 @@ public function testCommands() {
  * @return void
  */
 	public function testOptionsNoArguments() {
-		$this->Shell->runCommand('options', array());
+		$this->Shell->runCommand(['options']);
 		$output = $this->out->output;
 
 		$expected = "--help -h --verbose -v --quiet -q\n";
@@ -135,7 +135,7 @@ public function testOptionsNoArguments() {
  * @return void
  */
 	public function testOptionsNonExistingCommand() {
-		$this->Shell->runCommand('options', array('options', 'foo'));
+		$this->Shell->runCommand(['options', 'foo']);
 		$output = $this->out->output;
 
 		$expected = "--help -h --verbose -v --quiet -q\n";
@@ -148,7 +148,7 @@ public function testOptionsNonExistingCommand() {
  * @return void
  */
 	public function testOptions() {
-		$this->Shell->runCommand('options', array('options', 'bake'));
+		$this->Shell->runCommand(['options', 'bake']);
 		$output = $this->out->output;
 
 		$expected = "--help -h --verbose -v --quiet -q --connection -c --theme -t\n";
@@ -161,7 +161,7 @@ public function testOptions() {
  * @return void
  */
 	public function testSubCommandsCorePlugin() {
-		$this->Shell->runCommand('subcommands', array('subcommands', 'CORE.bake'));
+		$this->Shell->runCommand(['subcommands', 'CORE.bake']);
 		$output = $this->out->output;
 
 		$expected = "behavior component controller fixture helper model plugin project shell test view widget zerg\n";
@@ -174,7 +174,7 @@ public function testSubCommandsCorePlugin() {
  * @return void
  */
 	public function testSubCommandsAppPlugin() {
-		$this->Shell->runCommand('subcommands', array('subcommands', 'app.sample'));
+		$this->Shell->runCommand(['subcommands', 'app.sample']);
 		$output = $this->out->output;
 
 		$expected = '';
@@ -187,7 +187,7 @@ public function testSubCommandsAppPlugin() {
  * @return void
  */
 	public function testSubCommandsPlugin() {
-		$this->Shell->runCommand('subcommands', array('subcommands', 'TestPluginTwo.welcome'));
+		$this->Shell->runCommand(['subcommands', 'TestPluginTwo.welcome']);
 		$output = $this->out->output;
 
 		$expected = "say_hello\n";
@@ -200,7 +200,7 @@ public function testSubCommandsPlugin() {
  * @return void
  */
 	public function testSubCommandsNoArguments() {
-		$this->Shell->runCommand('subcommands', array());
+		$this->Shell->runCommand(['subcommands']);
 		$output = $this->out->output;
 
 		$expected = '';
@@ -213,7 +213,7 @@ public function testSubCommandsNoArguments() {
  * @return void
  */
 	public function testSubCommandsNonExistingCommand() {
-		$this->Shell->runCommand('subcommands', array('subcommands', 'foo'));
+		$this->Shell->runCommand(['subcommands', 'foo']);
 		$output = $this->out->output;
 
 		$expected = '';
@@ -226,7 +226,7 @@ public function testSubCommandsNonExistingCommand() {
  * @return void
  */
 	public function testSubCommands() {
-		$this->Shell->runCommand('subcommands', array('subcommands', 'bake'));
+		$this->Shell->runCommand(['subcommands', 'bake']);
 		$output = $this->out->output;
 
 		$expected = "behavior component controller fixture helper model plugin project shell test view widget zerg\n";
@@ -239,7 +239,7 @@ public function testSubCommands() {
  * @return void
  */
 	public function testFuzzy() {
-		$this->Shell->runCommand('fuzzy', array());
+		$this->Shell->runCommand(['fuzzy']);
 		$output = $this->out->output;
 
 		$expected = '';

diff --git a/tests/TestCase/Console/ConsoleOptionParserTest.php b/tests/TestCase/Console/ConsoleOptionParserTest.php
@@ -633,7 +633,7 @@ public function testParsingWithSubParser() {
 				)
 			));
 
-		$result = $parser->parse(array('--secondary', '--fourth', '4', 'c'), 'sub');
+		$result = $parser->parse(array('sub', '--secondary', '--fourth', '4', 'c'));
 		$expected = array(array(
 			'secondary' => true,
 			'fourth' => '4',

diff --git a/tests/TestCase/Console/ShellDispatcherTest.php b/tests/TestCase/Console/ShellDispatcherTest.php
@@ -130,7 +130,7 @@ public function testDispatchShellWithMain() {
 
 		$Shell->expects($this->once())->method('initialize');
 		$Shell->expects($this->once())->method('runCommand')
-			->with(null, array())
+			->with([])
 			->will($this->returnValue(true));
 
 		$dispatcher->expects($this->any())
@@ -155,7 +155,7 @@ public function testDispatchShellWithoutMain() {
 
 		$Shell->expects($this->once())->method('initialize');
 		$Shell->expects($this->once())->method('runCommand')
-			->with('initdb', array('initdb'))
+			->with(['initdb'])
 			->will($this->returnValue(true));
 
 		$dispatcher->expects($this->any())

diff --git a/tests/TestCase/Console/ShellTest.php b/tests/TestCase/Console/ShellTest.php
@@ -531,7 +531,7 @@ public function testRunCommandMain() {
 		$shell->expects($this->once())->method('main')
 			->with('cakes')
 			->will($this->returnValue(true));
-		$result = $shell->runCommand('cakes', ['cakes', '--verbose']);
+		$result = $shell->runCommand(['cakes', '--verbose']);
 		$this->assertTrue($result);
 	}
 
@@ -548,7 +548,7 @@ public function testRunCommandWithMethod() {
 		$shell->expects($this->once())->method('hit_me')
 			->with('cakes')
 			->will($this->returnValue(true));
-		$result = $shell->runCommand('hit_me', ['hit_me', 'cakes', '--verbose']);
+		$result = $shell->runCommand(['hit_me', 'cakes', '--verbose']);
 		$this->assertTrue($result);
 	}
 
@@ -574,7 +574,7 @@ public function testRunCommandWithMethodNotInSubcommands() {
 		$shell->expects($this->never())->method('startup');
 		$shell->expects($this->never())->method('roll');
 
-		$result = $shell->runCommand('roll', ['roll', 'cakes', '--verbose']);
+		$result = $shell->runCommand(['roll', 'cakes', '--verbose']);
 		$this->assertFalse($result);
 	}
 
@@ -599,7 +599,7 @@ public function testRunCommandWithMethodInSubcommands() {
 			->method('slice')
 			->with('cakes');
 
-		$shell->runCommand('slice', ['slice', 'cakes', '--verbose']);
+		$shell->runCommand(['slice', 'cakes', '--verbose']);
 	}
 
 /**
@@ -623,7 +623,7 @@ public function testRunCommandWithMissingMethodInSubcommands() {
 		$parser->expects($this->once())
 			->method('help');
 
-		$shell->runCommand('slice', ['slice', 'cakes', '--verbose']);
+		$shell->runCommand(['slice', 'cakes', '--verbose']);
 	}
 
 /**
@@ -641,7 +641,7 @@ public function testRunCommandBaseclassMethod() {
 		$shell->expects($this->never())->method('hr');
 		$shell->expects($this->once())->method('out');
 
-		$shell->runCommand('hr', array());
+		$shell->runCommand(['hr']);
 	}
 
 /**
@@ -658,7 +658,7 @@ public function testRunCommandMissingMethod() {
 			->will($this->returnValue($parser));
 		$shell->expects($this->once())->method('out');
 
-		$result = $shell->runCommand('idontexist', array());
+		$result = $shell->runCommand(['idontexist']);
 		$this->assertFalse($result);
 	}
 
@@ -679,7 +679,7 @@ public function testRunCommandTriggeringHelp() {
 			->will($this->returnValue($Parser));
 		$Shell->expects($this->once())->method('out');
 
-		$Shell->runCommand(null, array('--help'));
+		$Shell->runCommand(['--help']);
 	}
 
 /**
@@ -701,7 +701,7 @@ public function testRunCommandNotCallUnexposedTask() {
 		$shell->expects($this->once())->method('out');
 		$shell->RunCommand = $task;
 
-		$result = $shell->runCommand('run_command', ['run_command', 'one']);
+		$result = $shell->runCommand(['run_command', 'one']);
 		$this->assertFalse($result);
 	}
 
@@ -718,7 +718,7 @@ public function testRunCommandHittingTaskInSubcommand() {
 		$task = $this->getMock('Cake\Console\Shell', ['execute', 'runCommand'], [], '', false);
 		$task->expects($this->any())
 			->method('runCommand')
-			->with('execute', ['one']);
+			->with(['execute', 'one']);
 
 		$shell->expects($this->once())->method('getOptionParser')
 			->will($this->returnValue($parser));
@@ -729,7 +729,7 @@ public function testRunCommandHittingTaskInSubcommand() {
 			->will($this->returnValue(true));
 
 		$shell->Slice = $task;
-		$shell->runCommand('slice', ['slice', 'one']);
+		$shell->runCommand(['slice', 'one']);
 	}
 
 /**
@@ -799,7 +799,7 @@ public function testQuietLog() {
 			->with(false);
 
 		$this->Shell = $this->getMock(__NAMESPACE__ . '\ShellTestShell', array('_useLogger'), array($io));
-		$this->Shell->runCommand('foo', array('--quiet'));
+		$this->Shell->runCommand(['foo', '--quiet']);
 	}
 
 }