Fix for issue doctrine#81 Traits Composed from Traits

Use statements are now also imported for properties and methods from traits in traits. Conflict resolution and aliases are respected. For backwards compatibility reasons the use statements from the declaring class are still merged. PHP itself does not show this behaviour regarding to imported types in traits.
hboomsma · Oct 27, 2016 · 8d10181 · 8d10181
1 parent e1dd28b
commit 8d10181
Show file tree

Hide file tree

Showing 9 changed files with 170 additions and 26 deletions.
diff --git a/lib/Doctrine/Common/Annotations/AnnotationReader.php b/lib/Doctrine/Common/Annotations/AnnotationReader.php
@@ -344,22 +344,15 @@ private function getClassImports(ReflectionClass $class)
     private function getMethodImports(ReflectionMethod $method)
     {
         $class = $method->getDeclaringClass();
-        $classImports = $this->getClassImports($class);
-        if (!method_exists($class, 'getTraits')) {
-            return $classImports;
-        }
+        $classImports  = $this->getClassImports($class);
 
-        $traitImports = array();
+        // Since there is no way to know where a method originates
+        // when conflict resolution for traits has been used we fall
+        // back to using the filename form the \ReflectionMethod
+        // directly.
+        $methodImports = $this->phpParser->parseMethod($method);
 
-        foreach ($class->getTraits() as $trait) {
-            if ($trait->hasMethod($method->getName())
-                && $trait->getFileName() === $method->getFileName()
-            ) {
-                $traitImports = array_merge($traitImports, $this->phpParser->parseClass($trait));
-            }
-        }
-
-        return array_merge($classImports, $traitImports);
+        return array_merge($classImports, $methodImports);
     }
 
     /**
@@ -373,19 +366,45 @@ private function getPropertyImports(ReflectionProperty $property)
     {
         $class = $property->getDeclaringClass();
         $classImports = $this->getClassImports($class);
+
         if (!method_exists($class, 'getTraits')) {
             return $classImports;
         }
 
-        $traitImports = array();
+        $traitImports = $this->getDefiningTraitImports($property, $class);
+
+        return array_merge($classImports, $traitImports);
+    }
+
+    /**
+     * Perform depth first search for trait defining the property.
+     * Return the imports from the defining trait.
+     *
+     * @param ReflectionProperty $property
+     * @param ReflectionClass    $classOrTrait
+     *
+     * @return array
+     */
+    private function getDefiningTraitImports(ReflectionProperty $property, ReflectionClass $classOrTrait)
+    {
+        $traitImports = [];
+
+        foreach ($classOrTrait->getTraits() as $trait)
+        {
+            // Depth first.
+            if ($traitImports === []) {
+                $traitImports = $this->getDefiningTraitImports($property, $trait);
+            } else {
+                // exit early if we found our leaf definition of the property.
+                break;
+            }
 
-        foreach ($class->getTraits() as $trait) {
-            if ($trait->hasProperty($property->getName())) {
-                $traitImports = array_merge($traitImports, $this->phpParser->parseClass($trait));
+            // Do not add additional imports when going back up the recursion tree.
+            if ($traitImports === [] && $trait->hasProperty($property->getName())) {
+                return $this->phpParser->parseClass($trait);
             }
         }
-
-        return array_merge($classImports, $traitImports);
+        return $traitImports;
     }
 
     /**

diff --git a/lib/Doctrine/Common/Annotations/PhpParser.php b/lib/Doctrine/Common/Annotations/PhpParser.php
@@ -42,23 +42,54 @@ public function parseClass(\ReflectionClass $class)
             return $class->getUseStatements();
         }
 
-        if (false === $filename = $class->getFilename()) {
+        if (false === $filename = $class->getFileName()) {
             return array();
         }
 
-        $content = $this->getFileContent($filename, $class->getStartLine());
+        return $this->parse($filename, $class->getStartLine(), $class->getNamespaceName());
+    }
+
+    /**
+     * Parses a method.
+     *
+     * @param \Reflectionmethod $method A <code>ReflectionMethod</code> object.
+     *
+     * @return array A list with use statements in the form (Alias => FQN).
+     */
+    public function parseMethod(\ReflectionMethod $method)
+    {
+        if (method_exists($method, 'getUseStatements')) {
+            return $method->getUseStatements();
+        }
+
+        if (false === $filename = $method->getFileName()) {
+            return array();
+        }
+
+        return $this->parse($filename, $method->getStartLine(), $method->getNamespaceName());
+    }
+
+    /**
+     * Parse use statements from file.
+     *
+     * @param $filename
+     * @param $lineNumber
+     * @param $namespace
+     *
+     * @return array
+     */
+    private function parse($filename, $lineNumber, $namespace)
+    {
+        $content = $this->getFileContent($filename, $lineNumber);
 
         if (null === $content) {
             return array();
         }
 
-        $namespace = preg_quote($class->getNamespaceName());
         $content = preg_replace('/^.*?(\bnamespace\s+' . $namespace . '\s*[;{].*)$/s', '\\1', $content);
         $tokenizer = new TokenParser('<?php ' . $content);
 
-        $statements = $tokenizer->parseUseStatements($class->getNamespaceName());
-
-        return $statements;
+        return $tokenizer->parseUseStatements($namespace);
     }
 
     /**

diff --git a/tests/Doctrine/Tests/Common/Annotations/AnnotationReaderTest.php b/tests/Doctrine/Tests/Common/Annotations/AnnotationReaderTest.php
@@ -4,6 +4,12 @@
 
 use Doctrine\Common\Annotations\AnnotationReader;
 use Doctrine\Common\Annotations\DocParser;
+use Doctrine\Common\Reflection\Psr0FindFile;
+use Doctrine\Common\Reflection\StaticReflectionClass;
+use Doctrine\Common\Reflection\StaticReflectionMethod;
+use Doctrine\Common\Reflection\StaticReflectionParser;
+use Doctrine\Tests\Common\Annotations\Fixtures\Annotation\Autoload;
+use Doctrine\Tests\Common\Annotations\Fixtures\Annotation\Version;
 use Doctrine\Tests\Common\Annotations\Fixtures\IgnoredNamespaces\AnnotatedAtClassLevel;
 use Doctrine\Tests\Common\Annotations\Fixtures\IgnoredNamespaces\AnnotatedAtMethodLevel;
 use Doctrine\Tests\Common\Annotations\Fixtures\IgnoredNamespaces\AnnotatedAtPropertyLevel;
@@ -52,6 +58,35 @@ public function testPropertyAnnotationFromTrait()
         $this->assertInstanceOf('Doctrine\Tests\Common\Annotations\Fixtures\Annotation\Autoload', $annotations[0]);
     }
 
+    public function testPropertyAnnotationFromTraitThatUsesAnotherTrait()
+    {
+        $reader = $this->getReader();
+        $ref =
+            new \ReflectionClass('Doctrine\Tests\Common\Annotations\Fixtures\ClassThatUsesTraitThatUsesAnotherTrait');
+
+        $annotations = $reader->getPropertyAnnotations($ref->getProperty('route'));
+        $this->assertInstanceOf(Version::class, $annotations[0]);
+
+        $annotations = $reader->getPropertyAnnotations($ref->getProperty('intermediate'));
+        $this->assertInstanceOf(Autoload::class, $annotations[0]);
+    }
+
+    public function testMethodAnnotationFromTraitThatUsesAnotherTrait()
+    {
+        $reader = $this->getReader();
+        $ref =
+            new \ReflectionClass('Doctrine\Tests\Common\Annotations\Fixtures\ClassThatUsesTraitThatUsesAnotherTrait');
+
+        $annotations = $reader->getMethodAnnotations($ref->getMethod('secretAction'));
+        $this->assertInstanceOf('Doctrine\Tests\Common\Annotations\Fixtures\Annotation\Route', $annotations[0]);
+
+        $annotations = $reader->getMethodAnnotations($ref->getMethod('conflict'));
+        $this->assertInstanceOf('Doctrine\Tests\Common\Annotations\Fixtures\Annotation\Template', $annotations[0]);
+
+        $annotations = $reader->getMethodAnnotations($ref->getMethod('noConflict'));
+        $this->assertInstanceOf('Doctrine\Tests\Common\Annotations\Fixtures\Annotation\Autoload', $annotations[0]);
+    }
+
     public function testOmitNotRegisteredAnnotation()
     {
         $parser = new DocParser();

diff --git a/tests/Doctrine/Tests/Common/Annotations/CachedReaderTest.php b/tests/Doctrine/Tests/Common/Annotations/CachedReaderTest.php
@@ -52,6 +52,7 @@ public function testIgnoresStaleCacheWithTraitsThatUseOtherTraits()
 
         touch(__DIR__ . '/Fixtures/ClassThatUsesTraitThatUsesAnotherTrait.php', $cache - 10);
         touch(__DIR__ . '/Fixtures/Traits/EmptyTrait.php', $cache + 10);
+        touch(__DIR__.'/Fixtures/Traits/SecretRouteTrait.php', $cache + 10);
 
         $this->doTestCacheStale(
             'Doctrine\Tests\Common\Annotations\Fixtures\ClassThatUsesTraitThatUsesAnotherTrait',

diff --git a/tests/Doctrine/Tests/Common/Annotations/Fixtures/Traits/ConflictTraitA.php b/tests/Doctrine/Tests/Common/Annotations/Fixtures/Traits/ConflictTraitA.php
@@ -0,0 +1,13 @@
+<?php
+
+namespace Doctrine\Tests\Common\Annotations\Fixtures\Traits;
+
+use Doctrine\Tests\Common\Annotations\Fixtures\Annotation\Template;
+
+trait ConflictTraitA
+{
+    /**
+     * @Template
+     */
+    public function conflict(){}
+}
diff --git a/tests/Doctrine/Tests/Common/Annotations/Fixtures/Traits/ConflictTraitB.php b/tests/Doctrine/Tests/Common/Annotations/Fixtures/Traits/ConflictTraitB.php
@@ -0,0 +1,13 @@
+<?php
+
+namespace Doctrine\Tests\Common\Annotations\Fixtures\Traits;
+
+use Doctrine\Tests\Common\Annotations\Fixtures\Annotation\Route;
+
+trait ConflictTraitB
+{
+    /**
+     * @Route
+     */
+    public function conflict(){}
+}
diff --git a/tests/Doctrine/Tests/Common/Annotations/Fixtures/Traits/ConflictTraitC.php b/tests/Doctrine/Tests/Common/Annotations/Fixtures/Traits/ConflictTraitC.php
@@ -0,0 +1,13 @@
+<?php
+
+namespace Doctrine\Tests\Common\Annotations\Fixtures\Traits;
+
+use Doctrine\Tests\Common\Annotations\Fixtures\Annotation\Autoload;
+
+trait ConflictTraitC
+{
+    /**
+     * @Autoload
+     */
+    public function conflict(){}
+}
diff --git a/tests/Doctrine/Tests/Common/Annotations/Fixtures/Traits/SecretRouteTrait.php b/tests/Doctrine/Tests/Common/Annotations/Fixtures/Traits/SecretRouteTrait.php
@@ -4,9 +4,15 @@
 
 use Doctrine\Tests\Common\Annotations\Fixtures\Annotation\Template;
 use Doctrine\Tests\Common\Annotations\Fixtures\Annotation\Route;
+use Doctrine\Tests\Common\Annotations\Fixtures\Annotation\Version as Property;
 
 trait SecretRouteTrait
 {
+    /**
+     * @Property
+     */
+    private $route;
+
     /**
      * @Route("/secret", name="_secret")
      * @Template()

diff --git a/tests/Doctrine/Tests/Common/Annotations/Fixtures/Traits/TraitThatUsesAnotherTrait.php b/tests/Doctrine/Tests/Common/Annotations/Fixtures/Traits/TraitThatUsesAnotherTrait.php
@@ -2,7 +2,20 @@
 
 namespace Doctrine\Tests\Common\Annotations\Fixtures\Traits;
 
+use Doctrine\Tests\Common\Annotations\Fixtures\Annotation\Autoload as In;
+
 trait TraitThatUsesAnotherTrait
 {
     use EmptyTrait;
+    use SecretRouteTrait;
+    use ConflictTraitA, ConflictTraitB, ConflictTraitC {
+        ConflictTraitA::conflict insteadof ConflictTraitB;
+        ConflictTraitA::conflict insteadOf ConflictTraitC;
+        ConflictTraitC::conflict as protected noConflict;
+    }
+
+    /**
+     * @In
+     */
+    private $intermediate;
 }