Merge pull request #4324 from dhensby/pulls/namespace-import-manifest-fix

FIX Imported namespaces now correctly used to determine class inhertance

core/manifest/ClassManifest.php

@@ -106,6 +106,33 @@ class SS_ClassManifest {
 		));
 	}
 
+	/**
+	 * Create a {@link TokenisedRegularExpression} that extracts the namespaces imported with the 'use' keyword
+	 *
+	 * This searches symbols for a `use` followed by 1 or more namespaces which are optionally aliased using the `as`
+	 * keyword. The relevant matching tokens are added one-by-one into an array (using `save_to` param).
+	 *
+	 * eg: use Namespace\ClassName as Alias, OtherNamespace\ClassName;
+	 *
+	 * @return TokenisedRegularExpression
+	 */
+	public static function get_imported_namespace_parser() {
+		return new TokenisedRegularExpression(array(
+			0 => T_USE,
+			1 => T_WHITESPACE,
+			2 => array(T_NS_SEPARATOR, 'save_to' => 'importString[]', 'optional' => true),
+			3 => array(T_STRING, 'save_to' => 'importString[]', 'can_jump_to' => array(2, 8)),
+			4 => array(T_WHITESPACE, 'save_to' => 'importString[]'),
+			5 => array(T_AS, 'save_to' => 'importString[]'),
+			6 => array(T_WHITESPACE, 'save_to' => 'importString[]'),
+			7 => array(T_STRING, 'save_to' => 'importString[]'),
+			8 => array(T_WHITESPACE, 'optional' => true),
+			9 => array(',', 'save_to' => 'importString[]', 'optional' => true, 'can_jump_to' => 2),
+			10 => array(T_WHITESPACE, 'optional' => true, 'can_jump_to' => 2),
+			11 => ';',
+		));
+	}
+
 	/**
 	 * Constructs and initialises a new class manifest, either loading the data
 	 * from the cache or re-scanning for classes.
@@ -333,6 +360,124 @@ class SS_ClassManifest {
 		}
 	}
 
+	/**
+	 * Find a the full namespaced declaration of a class (or interface) from a list of candidate imports
+	 *
+	 * This is typically used to determine the full class name in classes that have imported namesapced symbols (having
+	 * used the `use` keyword)
+	 *
+	 * NB: remember the '\\' is an escaped backslash and is interpreted as a single \
+	 *
+	 * @param string $class The class (or interface) name to find in the candidate imports
+	 * @param string $namespace The namespace that was declared for the classes definition (if there was one)
+	 * @param array $imports The list of imported symbols (Classes or Interfaces) to test against
+	 *
+	 * @return string The fully namespaced class name
+	 */
+	protected function findClassOrInterfaceFromCandidateImports($class, $namespace = '', $imports = array()) {
+
+		//normalise the namespace
+		$namespace = rtrim($namespace, '\\');
+
+		//by default we'll use the $class as our candidate
+		$candidateClass = $class;
+
+		if (!$class) {
+			return $candidateClass;
+		}
+		//if the class starts with a \ then it is explicitly in the global namespace and we don't need to do
+		// anything else
+		if (substr($class, 0, 1) == '\\') {
+			$candidateClass = substr($class, 1);
+			return $candidateClass;
+		}
+		//if there's a namespace, starting assumption is the class is defined in that namespace
+		if ($namespace) {
+			$candidateClass = $namespace . '\\' . $class;
+		}
+
+		if (empty($imports)) {
+			return $candidateClass;
+		}
+
+		//normalised class name (PHP is case insensitive for symbols/namespaces
+		$lClass = strtolower($class);
+
+		//go through all the imports and see if the class exists within one of them
+		foreach ($imports as $alias => $import) {
+			//normalise import
+			$import = trim($import, '\\');
+
+			//if there is no string key, then there was no declared alias - we'll use the main declaration
+			if (is_int($alias)) {
+				$alias = strtolower($import);
+			} else {
+				$alias = strtolower($alias);
+			}
+
+			//exact match? Then it's a class in the global namespace that was imported OR it's an alias of
+			// another namespace
+			// or if it ends with the \ClassName then it's the class we are looking for
+			if ($lClass == $alias
+				|| substr_compare(
+					$alias,
+					'\\' . $lClass,
+					strlen($alias) - strlen($lClass) - 1,
+					// -1 because the $lClass length is 1 longer due to \
+					strlen($alias)
+				) === 0
+			) {
+				$candidateClass = $import;
+				break;
+			}
+		}
+		return $candidateClass;
+	}
+
+	/**
+	 * Return an array of array($alias => $import) from tokenizer's tokens of a PHP file
+	 *
+	 * NB: If there is no alias we don't set a key to the array
+	 *
+	 * @param array $tokens The parsed tokens from tokenizer's parsing of a PHP file
+	 *
+	 * @return array The array of imports as (optional) $alias => $import
+	 */
+	protected function getImportsFromTokens($tokens) {
+		//parse out the imports
+		$imports = self::get_imported_namespace_parser()->findAll($tokens);
+
+		//if there are any imports, clean them up
+		// imports come to us as array('importString' => array([array of matching tokens]))
+		// we need to join this nested array into a string and split out the alias and the import
+		if (!empty($imports)) {
+			$cleanImports = array();
+			foreach ($imports as $import) {
+				if (!empty($import['importString'])) {
+					//join the array up into a string
+					$importString = implode('', $import['importString']);
+					//split at , to get each import declaration
+					$importSet = explode(',', $importString);
+					foreach ($importSet as $importDeclaration) {
+						//split at ' as ' (any case) to see if we are aliasing the namespace
+						$importDeclaration = preg_split('/\s+as\s+/i', $importDeclaration);
+						//shift off the fully namespaced import
+						$qualifiedImport = array_shift($importDeclaration);
+						//if there are still items in the array, it's the alias
+						if (!empty($importDeclaration)) {
+							$cleanImports[array_shift($importDeclaration)] = $qualifiedImport;
+						}
+						else {
+							$cleanImports[] = $qualifiedImport;
+						}
+					}
+				}
+			}
+			$imports = $cleanImports;
+		}
+		return $imports;
+	}
+
 	public function handleFile($basename, $pathname, $depth) {
 		if ($basename == self::CONF_FILE) {
 			$this->configs[] = $pathname;
@@ -342,6 +487,7 @@ class SS_ClassManifest {
 		$classes    = null;
 		$interfaces = null;
 		$namespace = null;
+		$imports = null;
 
 		// The results of individual file parses are cached, since only a few
 		// files will have changed and TokenisedRegularExpression is quite
@@ -352,14 +498,17 @@ class SS_ClassManifest {
 
 		if ($data = $this->cache->load($key)) {
 			$valid = (
-				isset($data['classes']) && isset($data['interfaces']) && isset($data['namespace'])
+				isset($data['classes']) && is_array($data['classes'])
-				&& is_array($data['classes']) && is_array($data['interfaces']) && is_string($data['namespace'])
+				&& isset($data['interfaces']) && is_array($data['interfaces'])
+				&& isset($data['namespace']) && is_string($data['namespace'])
+				&& isset($data['imports']) && is_array($data['imports'])
 			);
 
 			if ($valid) {
 				$classes = $data['classes'];
 				$interfaces = $data['interfaces'];
 				$namespace = $data['namespace'];
+				$imports = $data['imports'];
 			}
 		}
 
@@ -367,28 +516,52 @@ class SS_ClassManifest {
 			$tokens     = token_get_all($file);
 
 			$classes = self::get_namespaced_class_parser()->findAll($tokens);
+
 			$namespace = self::get_namespace_parser()->findAll($tokens);
+
 			if($namespace) {
-				$namespace = implode('', $namespace[0]['namespaceName']) . '\\';
+				$namespace = implode('', $namespace[0]['namespaceName']);
 			} else {
 				$namespace = '';
 			}
 
+			$imports = $this->getImportsFromTokens($tokens);
+
 			$interfaces = self::get_interface_parser()->findAll($tokens);
 
-			$cache = array('classes' => $classes, 'interfaces' => $interfaces, 'namespace' => $namespace);
+			$cache = array(
+				'classes' => $classes,
+				'interfaces' => $interfaces,
+				'namespace' => $namespace,
+				'imports' => $imports
+			);
 			$this->cache->save($cache, $key);
 		}
 
 		foreach ($classes as $class) {
-			$name       = $namespace . $class['className'];
+			$name = $class['className'];
+			if ($namespace) {
+				$namespace = rtrim($namespace, '\\');
+				$name = $namespace . '\\' . $name;
+			}
 			$extends = isset($class['extends']) ? implode('', $class['extends']) : null;
 			$implements = isset($class['interfaces']) ? $class['interfaces'] : null;
 
-			if($extends && $extends[0] != '\\') {
+			if ($extends) {
-				$extends = $namespace . $extends;
+				$extends = $this->findClassOrInterfaceFromCandidateImports($extends, $namespace, $imports);
-			} elseif($extends) {
+			}
-				$extends = substr($extends, 1);
+
+			if (!empty($implements)) {
+				//join all the tokens
+				$implements = implode('', $implements);
+				//split at comma
+				$implements = explode(',', $implements);
+				//normalise interfaces
+				foreach ($implements as &$interface) {
+					$interface = $this->findClassOrInterfaceFromCandidateImports($interface, $namespace, $imports);
+				}
+				//release the var name
+				unset($interface);
 			}
 
 			$lowercaseName = strtolower($name);
@@ -414,18 +587,7 @@ class SS_ClassManifest {
 			}
 
 			if ($implements) {
-				$interface = $namespace;
+				foreach ($implements as $interface) {
-				for($i = 0; $i < count($implements); ++$i) {
-					if($implements[$i] == ',') {
-						$interface = $namespace;
-						continue;
-					}
-					if($implements[$i] == '\\' && $interface == $namespace) {
-						$interface = '';
-					} else {
-						$interface .= $implements[$i];
-					}
-					if($i == count($implements)-1 || $implements[$i+1] == ',') {
 					$interface = strtolower($interface);
 
 					if (!isset($this->implementors[$interface])) {
@@ -436,10 +598,13 @@ class SS_ClassManifest {
 				}
 			}
 		}
-		}
 
+		$interfaceBase = '';
+		if ($namespace) {
+			$interfaceBase = $namespace . '\\';
+		}
 		foreach ($interfaces as $interface) {
-			$this->interfaces[strtolower($namespace . $interface['interfaceName'])] = $pathname;
+			$this->interfaces[strtolower($interfaceBase . $interface['interfaceName'])] = $pathname;
 		}
 	}
 

tests/core/manifest/NamespacedClassManifestTest.php

@@ -15,6 +15,152 @@ class NamespacedClassManifestTest extends SapphireTest {
 
 		$this->base = dirname(__FILE__) . '/fixtures/namespaced_classmanifest';
 		$this->manifest      = new SS_ClassManifest($this->base, false, true, false);
+		SS_ClassLoader::instance()->pushManifest($this->manifest, false);
+	}
+
+	public function tearDown() {
+		parent::tearDown();
+		SS_ClassLoader::instance()->popManifest();
+	}
+
+	public function testGetImportedNamespaceParser() {
+		$file = file_get_contents($this->base . DIRECTORY_SEPARATOR . 'module/classes/ClassI.php');
+		$tokens = token_get_all($file);
+		$parsedTokens = SS_ClassManifest::get_imported_namespace_parser()->findAll($tokens);
+
+		$expectedItems = array(
+			array('ModelAdmin'),
+			array('Controller', '  ', 'as', '  ', 'Cont'),
+			array(
+				'SS_HTTPRequest', ' ', 'as', ' ', 'Request', ',',
+				'SS_HTTPResponse', ' ', 'AS', ' ', 'Response', ',',
+				'PermissionProvider', ' ', 'AS', ' ', 'P',
+			),
+			array('silverstripe', '\\', 'test', '\\', 'ClassA'),
+			array('\\', 'DataObject'),
+		);
+
+		$this->assertEquals(count($expectedItems), count($parsedTokens));
+
+		foreach ($expectedItems as $i => $item) {
+			$this->assertEquals($item, $parsedTokens[$i]['importString']);
+		}
+	}
+
+	public function testGetImportsFromTokens() {
+		$file = file_get_contents($this->base . DIRECTORY_SEPARATOR . 'module/classes/ClassI.php');
+		$tokens = token_get_all($file);
+
+		$method = new ReflectionMethod($this->manifest, 'getImportsFromTokens');
+		$method->setAccessible(true);
+
+		$expectedImports = array(
+			'ModelAdmin',
+			'Cont' => 'Controller',
+			'Request' => 'SS_HTTPRequest',
+			'Response' => 'SS_HTTPResponse',
+			'P' => 'PermissionProvider',
+			'silverstripe\test\ClassA',
+			'\DataObject',
+		);
+
+		$imports = $method->invoke($this->manifest, $tokens);
+
+		$this->assertEquals($expectedImports, $imports);
+
+	}
+
+	public function testClassInfoIsCorrect() {
+		$this->assertContains('SilverStripe\Framework\Tests\ClassI', ClassInfo::implementorsOf('PermissionProvider'));
+
+		//because we're using a nested manifest we have to "coalesce" the descendants again to correctly populate the
+		// descendants of the core classes we want to test against - this is a limitation of the test manifest not
+		// including all core classes
+		$method = new ReflectionMethod($this->manifest, 'coalesceDescendants');
+		$method->setAccessible(true);
+		$method->invoke($this->manifest, 'ModelAdmin');
+
+		$this->assertContains('SilverStripe\Framework\Tests\ClassI', ClassInfo::subclassesFor('ModelAdmin'));
+	}
+
+	public function testFindClassOrInterfaceFromCandidateImports() {
+		$method = new ReflectionMethod($this->manifest, 'findClassOrInterfaceFromCandidateImports');
+		$method->setAccessible(true);
+
+		$this->assertTrue(ClassInfo::exists('silverstripe\test\ClassA'));
+
+		$this->assertEquals('PermissionProvider', $method->invokeArgs($this->manifest, array(
+			'\PermissionProvider',
+			'Test\Namespace',
+			array(
+				'TestOnly',
+				'Controller',
+			),
+		)));
+
+		$this->assertEquals('PermissionProvider', $method->invokeArgs($this->manifest, array(
+			'PermissionProvider',
+			'Test\NAmespace',
+			array(
+				'PermissionProvider',
+			)
+		)));
+
+		$this->assertEmpty($method->invokeArgs($this->manifest, array(
+			'',
+			'TextNamespace',
+			array(
+				'PermissionProvider',
+			),
+		)));
+
+		$this->assertEmpty($method->invokeArgs($this->manifest, array(
+			'',
+			'',
+			array()
+		)));
+
+		$this->assertEquals('silverstripe\test\ClassA', $method->invokeArgs($this->manifest, array(
+			'ClassA',
+			'Test\Namespace',
+			array(
+				'silverstripe\test\ClassA',
+				'PermissionProvider',
+			),
+		)));
+
+		$this->assertEquals('ClassA', $method->invokeArgs($this->manifest, array(
+			'\ClassA',
+			'Test\Namespace',
+			array(
+				'silverstripe\test',
+			),
+		)));
+
+		$this->assertEquals('ClassA', $method->invokeArgs($this->manifest, array(
+			'ClassA',
+			'silverstripe\test',
+			array(
+				'\ClassA',
+			),
+		)));
+
+		$this->assertEquals('ClassA', $method->invokeArgs($this->manifest, array(
+			'Alias',
+			'silverstripe\test',
+			array(
+				'Alias' => '\ClassA',
+			),
+		)));
+
+		$this->assertEquals('silverstripe\test\ClassA', $method->invokeArgs($this->manifest, array(
+			'ClassA',
+			'silverstripe\test',
+			array(
+				'silverstripe\test\ClassB',
+			),
+		)));
+
 	}
 
 	public function testGetItemPath() {
@@ -43,7 +189,8 @@ class NamespacedClassManifestTest extends SapphireTest {
 			'silverstripe\test\classg' => "{$this->base}/module/classes/ClassG.php",
 			'silverstripe\test\classh' => "{$this->base}/module/classes/ClassH.php",
 			'sstemplateparser'         => FRAMEWORK_PATH."/view/SSTemplateParser.php",
-			'sstemplateparseexception' => FRAMEWORK_PATH."/view/SSTemplateParser.php"
+			'sstemplateparseexception' => FRAMEWORK_PATH."/view/SSTemplateParser.php",
+			'silverstripe\framework\tests\classi' => "{$this->base}/module/classes/ClassI.php",
 		);
 
 		$this->assertEquals($expect, $this->manifest->getClasses());
@@ -54,7 +201,7 @@ class NamespacedClassManifestTest extends SapphireTest {
 			array('sstemplateparser', 'sstemplateparseexception', 'silverstripe\test\classa',
 				'silverstripe\test\classb', 'silverstripe\test\classc', 'silverstripe\test\classd',
 				'silverstripe\test\classe', 'silverstripe\test\classf', 'silverstripe\test\classg',
-				'silverstripe\test\classh'),
+				'silverstripe\test\classh', 'silverstripe\framework\tests\classi'),
 			$this->manifest->getClassNames());
 	}
 
@@ -88,7 +235,8 @@ class NamespacedClassManifestTest extends SapphireTest {
 		$expect = array(
 			'silverstripe\test\interfacea' => array('silverstripe\test\ClassE'),
 			'interfacea' => array('silverstripe\test\ClassF'),
-			'silverstripe\test\subtest\interfacea' => array('silverstripe\test\ClassG')
+			'silverstripe\test\subtest\interfacea' => array('silverstripe\test\ClassG'),
+			'permissionprovider' => array('SilverStripe\Framework\Tests\ClassI'),
 		);
 		$this->assertEquals($expect, $this->manifest->getImplementors());
 	}

tests/core/manifest/fixtures/namespaced_classmanifest/module/classes/ClassI.php

@@ -0,0 +1,12 @@
+<?php
+namespace SilverStripe\Framework\Tests;
+
+//whitespace here is important for tests, please don't change it
+use  ModelAdmin;
+use Controller  as  Cont ;
+use SS_HTTPRequest as Request,SS_HTTPResponse AS Response, PermissionProvider AS P;
+use silverstripe\test\ClassA;
+use \DataObject;
+
+class ClassI extends ModelAdmin implements P {
+}