Refactor shell finding so aliases do not overwrite app classes.

Aliasing a shell plugin should not overwrite a core/app shell. Instead
aliases should be fallbacks for when a core/app class cannot be found.

Refs #3325

src/Console/ShellDispatcher.php

@@ -77,6 +77,15 @@ class ShellDispatcher {
 	}
 
 /**
+ * Clear any aliases that have been set.
+ *
+ * @return void
+ */
+	public static function resetAliases() {
+		static::$_aliases = [];
+	}
+
+/**
  * Run the dispatcher
  *
  * @param array $argv The argv from PHP
@@ -148,7 +157,7 @@ class ShellDispatcher {
 			return true;
 		}
 
-		$Shell = $this->_getShell($shell);
+		$Shell = $this->findShell($shell);
 
 		$command = null;
 		if (isset($this->args[0])) {
@@ -188,27 +197,36 @@ class ShellDispatcher {
  * @return mixed An object
  * @throws \Cake\Console\Error\MissingShellException when errors are encountered.
  */
-	protected function _getShell($shell) {
-		if (isset(static::$_aliases[$shell])) {
+	public function findShell($shell) {
+		$classname = $this->_shellExists($shell);
+		if (!$classname && isset(static::$_aliases[$shell])) {
 			$shell = static::$_aliases[$shell];
+			$classname = $this->_shellExists($shell);
+		}
+		if ($classname) {
+			list($plugin) = pluginSplit($shell);
+			$instance = new $classname();
+			$instance->plugin = Inflector::camelize(trim($plugin, '.'));
+			return $instance;
 		}
-		list($plugin, $shell) = pluginSplit($shell);
+		throw new Error\MissingShellException([
+			'class' => $shell,
+		]);
+	}
 
-		$plugin = Inflector::camelize($plugin);
+/**
+ * Check if a shell class exists for the given name.
+ *
+ * @param string $shell The shell name to look for.
+ * @return string|boolean Either the classname or false.
+ */
+	protected function _shellExists($shell) {
 		$class = Inflector::camelize($shell);
-		if ($plugin) {
-			$class = $plugin . '.' . $class;
-		}
 		$class = App::classname($class, 'Console/Command', 'Shell');
-
-		if (!class_exists($class)) {
-			throw new Error\MissingShellException([
-				'class' => $shell,
-			]);
+		if (class_exists($class)) {
+			return $class;
 		}
-		$Shell = new $class();
-		$Shell->plugin = trim($plugin, '.');
-		return $Shell;
+		return false;
 	}
 
 /**

tests/TestCase/Console/ShellDispatcherTest.php

@@ -21,98 +21,51 @@ use Cake\Core\Plugin;
 use Cake\TestSuite\TestCase;
 
 /**
- * TestShellDispatcher class
- *
- */
-class TestShellDispatcher extends ShellDispatcher {
-
-/**
- * params property
- *
- * @var array
- */
-	public $params = array();
-
-/**
- * stopped property
- *
- * @var string
- */
-	public $stopped = null;
-
-/**
- * TestShell
+ * ShellDispatcherTest
  *
- * @var mixed
  */
-	public $TestShell;
+class ShellDispatcherTest extends TestCase {
 
 /**
- * _initEnvironment method
+ * setUp method
  *
  * @return void
  */
-	protected function _initEnvironment() {
+	public function setUp() {
+		parent::setUp();
+		Plugin::load('TestPlugin');
+		Configure::write('App.namespace', 'TestApp');
+		$this->dispatcher = $this->getMock('Cake\Console\ShellDispatcher', ['_stop']);
 	}
 
 /**
- * clear method
+ * teardown
  *
  * @return void
  */
-	public function clear() {
+	public function tearDown() {
+		parent::tearDown();
+		ShellDispatcher::resetAliases();
 	}
 
 /**
- * _stop method
+ * Test error on missing shell
  *
+ * @expectedException Cake\Console\Error\MissingShellException
  * @return void
  */
-	protected function _stop($status = 0) {
-		$this->stopped = 'Stopped with status: ' . $status;
-		return $status;
-	}
-
-/**
- * getShell
- *
- * @param string $shell
- * @return mixed
- */
-	public function getShell($shell) {
-		return $this->_getShell($shell);
-	}
-
-/**
- * _getShell
- *
- * @param string $shell
- * @return mixed
- */
-	protected function _getShell($shell) {
-		if (isset($this->TestShell)) {
-			return $this->TestShell;
-		}
-		return parent::_getShell($shell);
+	public function testFindShellMissing() {
+		$this->dispatcher->findShell('nope');
 	}
 
-}
-
-/**
- * ShellDispatcherTest
- *
- */
-class ShellDispatcherTest extends TestCase {
-
 /**
- * setUp method
+ * Test error on missing plugin shell
  *
+ * @expectedException Cake\Console\Error\MissingShellException
  * @return void
  */
-	public function setUp() {
-		parent::setUp();
-		Plugin::load('TestPlugin');
-		Configure::write('App.namespace', 'TestApp');
+	public function testFindShellMissingPlugin() {
+		$this->dispatcher->findShell('test_plugin.nope');
 	}
 
 /**
@@ -120,20 +73,19 @@ class ShellDispatcherTest extends TestCase {
  *
  * @return void
  */
-	public function testGetShell() {
-		$Dispatcher = new TestShellDispatcher();
-
-		$result = $Dispatcher->getShell('sample');
+	public function testFindShell() {
+		$result = $this->dispatcher->findShell('sample');
 		$this->assertInstanceOf('TestApp\Console\Command\SampleShell', $result);
 
-		$Dispatcher = new TestShellDispatcher();
-		$result = $Dispatcher->getShell('test_plugin.example');
+		$result = $this->dispatcher->findShell('test_plugin.example');
 		$this->assertInstanceOf('TestPlugin\Console\Command\ExampleShell', $result);
 		$this->assertEquals('TestPlugin', $result->plugin);
 		$this->assertEquals('Example', $result->name);
 
-		$Dispatcher = new TestShellDispatcher();
-		$result = $Dispatcher->getShell('TestPlugin.example');
+		$result = $this->dispatcher->findShell('TestPlugin.example');
+		$this->assertInstanceOf('TestPlugin\Console\Command\ExampleShell', $result);
+
+		$result = $this->dispatcher->findShell('TestPlugin.Example');
 		$this->assertInstanceOf('TestPlugin\Console\Command\ExampleShell', $result);
 	}
 
@@ -142,23 +94,38 @@ class ShellDispatcherTest extends TestCase {
  *
  * @return void
  */
-	public function testGetShellAliased() {
-		TestShellDispatcher::alias('short', 'test_plugin.example');
+	public function testFindShellAliased() {
+		ShellDispatcher::alias('short', 'test_plugin.example');
 
-		$dispatcher = new TestShellDispatcher();
-		$result = $dispatcher->getShell('short');
+		$result = $this->dispatcher->findShell('short');
 		$this->assertInstanceOf('TestPlugin\Console\Command\ExampleShell', $result);
 		$this->assertEquals('TestPlugin', $result->plugin);
 		$this->assertEquals('Example', $result->name);
 	}
 
 /**
+ * Test finding a shell that has a matching alias.
+ *
+ * Aliases should not overload concrete shells.
+ *
+ * @return void
+ */
+	public function testFindShellAliasedAppShadow() {
+		ShellDispatcher::alias('sample', 'test_plugin.example');
+
+		$result = $this->dispatcher->findShell('sample');
+		$this->assertInstanceOf('TestApp\Console\Command\SampleShell', $result);
+		$this->assertEmpty($result->plugin);
+		$this->assertEquals('Sample', $result->name);
+	}
+
+/**
  * Verify correct dispatch of Shell subclasses with a main method
  *
  * @return void
  */
 	public function testDispatchShellWithMain() {
-		$Dispatcher = new TestShellDispatcher();
+		$dispatcher = $this->getMock('Cake\Console\ShellDispatcher', ['findShell']);
 		$Shell = $this->getMock('Cake\Console\Shell');
 
 		$Shell->expects($this->once())->method('initialize');
@@ -166,12 +133,15 @@ class ShellDispatcherTest extends TestCase {
 			->with(null, array())
 			->will($this->returnValue(true));
 
-		$Dispatcher->TestShell = $Shell;
+		$dispatcher->expects($this->any())
+			->method('findShell')
+			->with('mock_with_main')
+			->will($this->returnValue($Shell));
 
-		$Dispatcher->args = array('mock_with_main');
-		$result = $Dispatcher->dispatch();
+		$dispatcher->args = array('mock_with_main');
+		$result = $dispatcher->dispatch();
 		$this->assertEquals(0, $result);
-		$this->assertEquals(array(), $Dispatcher->args);
+		$this->assertEquals(array(), $dispatcher->args);
 	}
 
 /**
@@ -180,7 +150,7 @@ class ShellDispatcherTest extends TestCase {
  * @return void
  */
 	public function testDispatchShellWithoutMain() {
-		$Dispatcher = new TestShellDispatcher();
+		$dispatcher = $this->getMock('Cake\Console\ShellDispatcher', ['findShell']);
 		$Shell = $this->getMock('Cake\Console\Shell');
 
 		$Shell->expects($this->once())->method('initialize');
@@ -188,70 +158,13 @@ class ShellDispatcherTest extends TestCase {
 			->with('initdb', array('initdb'))
 			->will($this->returnValue(true));
 
-		$Dispatcher->TestShell = $Shell;
+		$dispatcher->expects($this->any())
+			->method('findShell')
+			->with('mock_without_main')
+			->will($this->returnValue($Shell));
 
-		$Dispatcher->args = array('mock_without_main', 'initdb');
-		$result = $Dispatcher->dispatch();
-		$this->assertEquals(0, $result);
-	}
-
-/**
- * Verify correct dispatch of custom classes with a main method
- *
- * @return void
- */
-	public function testDispatchNotAShellWithMain() {
-		$Dispatcher = new TestShellDispatcher();
-		$methods = ['main', 'initdb', 'initialize', 'loadTasks', 'startup', '_secret'];
-		$Shell = $this->getMock('stdClass', $methods);
-
-		$Shell->expects($this->never())->method('initialize');
-		$Shell->expects($this->once())->method('startup');
-		$Shell->expects($this->once())->method('main')->will($this->returnValue(true));
-		$Dispatcher->TestShell = $Shell;
-
-		$Dispatcher->args = array('mock_with_main_not_a');
-		$result = $Dispatcher->dispatch();
-		$this->assertEquals(0, $result);
-		$this->assertEquals(array(), $Dispatcher->args);
-
-		$Shell = $this->getMock('stdClass', $methods);
-		$Shell->expects($this->once())->method('initdb')->will($this->returnValue(true));
-		$Shell->expects($this->once())->method('startup');
-		$Dispatcher->TestShell = $Shell;
-
-		$Dispatcher->args = array('mock_with_main_not_a', 'initdb');
-		$result = $Dispatcher->dispatch();
-		$this->assertEquals(0, $result);
-	}
-
-/**
- * Verify correct dispatch of custom classes without a main method
- *
- * @return void
- */
-	public function testDispatchNotAShellWithoutMain() {
-		$Dispatcher = new TestShellDispatcher();
-		$methods = ['main', 'initdb', 'initialize', 'loadTasks', 'startup', '_secret'];
-		$Shell = $this->getMock('stdClass', $methods);
-
-		$Shell->expects($this->never())->method('initialize');
-		$Shell->expects($this->once())->method('startup');
-		$Shell->expects($this->once())->method('main')->will($this->returnValue(true));
-		$Dispatcher->TestShell = $Shell;
-
-		$Dispatcher->args = array('mock_without_main_not_a');
-		$result = $Dispatcher->dispatch();
-		$this->assertEquals(0, $result);
-		$this->assertEquals(array(), $Dispatcher->args);
-
-		$Shell = $this->getMock('stdClass', $methods);
-		$Shell->expects($this->once())->method('initdb')->will($this->returnValue(true));
-		$Shell->expects($this->once())->method('startup');
-		$Dispatcher->TestShell = $Shell;
-
-		$Dispatcher->args = array('mock_without_main_not_a', 'initdb');
-		$result = $Dispatcher->dispatch();
+		$dispatcher->args = array('mock_without_main', 'initdb');
+		$result = $dispatcher->dispatch();
 		$this->assertEquals(0, $result);
 	}
 
@@ -261,27 +174,25 @@ class ShellDispatcherTest extends TestCase {
  * @return void
  */
 	public function testShiftArgs() {
-		$Dispatcher = new TestShellDispatcher();
-
-		$Dispatcher->args = array('a', 'b', 'c');
-		$this->assertEquals('a', $Dispatcher->shiftArgs());
-		$this->assertSame($Dispatcher->args, array('b', 'c'));
+		$this->dispatcher->args = array('a', 'b', 'c');
+		$this->assertEquals('a', $this->dispatcher->shiftArgs());
+		$this->assertSame($this->dispatcher->args, array('b', 'c'));
 
-		$Dispatcher->args = array('a' => 'b', 'c', 'd');
-		$this->assertEquals('b', $Dispatcher->shiftArgs());
-		$this->assertSame($Dispatcher->args, array('c', 'd'));
+		$this->dispatcher->args = array('a' => 'b', 'c', 'd');
+		$this->assertEquals('b', $this->dispatcher->shiftArgs());
+		$this->assertSame($this->dispatcher->args, array('c', 'd'));
 
-		$Dispatcher->args = array('a', 'b' => 'c', 'd');
-		$this->assertEquals('a', $Dispatcher->shiftArgs());
-		$this->assertSame($Dispatcher->args, array('b' => 'c', 'd'));
+		$this->dispatcher->args = array('a', 'b' => 'c', 'd');
+		$this->assertEquals('a', $this->dispatcher->shiftArgs());
+		$this->assertSame($this->dispatcher->args, array('b' => 'c', 'd'));
 
-		$Dispatcher->args = array(0 => 'a', 2 => 'b', 30 => 'c');
-		$this->assertEquals('a', $Dispatcher->shiftArgs());
-		$this->assertSame($Dispatcher->args, array(0 => 'b', 1 => 'c'));
+		$this->dispatcher->args = array(0 => 'a', 2 => 'b', 30 => 'c');
+		$this->assertEquals('a', $this->dispatcher->shiftArgs());
+		$this->assertSame($this->dispatcher->args, array(0 => 'b', 1 => 'c'));
 
-		$Dispatcher->args = array();
-		$this->assertNull($Dispatcher->shiftArgs());
-		$this->assertSame(array(), $Dispatcher->args);
+		$this->dispatcher->args = array();
+		$this->assertNull($this->dispatcher->shiftArgs());
+		$this->assertSame(array(), $this->dispatcher->args);
 	}
 
 }