Scrutinizer fixes (goaop#76)

Fixing issues, reported by Scrutinizer CI 1. most of issues addressed 2. normalized typecast operator look 3. replaced magic numbers with PHP constants 4. added parameter type checks for `ReflectionFile` and `ReflectionFileNamespace` classes
dg · Aug 29, 2017 · 3c4f82c · 3c4f82c
1 parent 21fb570
commit 3c4f82c
Show file tree

Hide file tree

Showing 16 changed files with 341 additions and 25 deletions.
diff --git a/src/ReflectionClass.php b/src/ReflectionClass.php
@@ -125,4 +125,18 @@ protected function __initialize()
     {
         parent::__construct($this->getName());
     }
+
+    /**
+     * Create a ReflectionClass for a given class name.
+     *
+     * @param string $className
+     *     The name of the class to create a reflection for.
+     *
+     * @return ReflectionClass
+     *     The apropriate reflection object.
+     */
+    protected function createReflectionForClass($className)
+    {
+        return class_exists($className, false) ? new parent($className) : new static($className);
+    }
 }
diff --git a/src/ReflectionFile.php b/src/ReflectionFile.php
@@ -56,6 +56,14 @@ class ReflectionFile
      */
     public function __construct($fileName, $topLevelNodes = null, ReflectionContext $context = null)
     {
+        if (!is_string($fileName)) {
+            throw new \InvalidArgumentException(
+                sprintf(
+                    '$fileName must be a string, but a %s was passed',
+                    gettype($fileName)
+                )
+            );
+        }
         $fileName            = PathResolver::realpath($fileName);
         $this->fileName      = $fileName;
         $this->context       = $context ?: ReflectionEngine::getReflectionContext();

diff --git a/src/ReflectionFileNamespace.php b/src/ReflectionFileNamespace.php
@@ -89,6 +89,14 @@ class ReflectionFileNamespace
      */
     public function __construct($fileName, $namespaceName, Namespace_ $namespaceNode = null, ReflectionContext $context = null)
     {
+        if (!is_string($fileName)) {
+            throw new \InvalidArgumentException(
+                sprintf(
+                    '$fileName must be a string, but a %s was passed',
+                    gettype($fileName)
+                )
+            );
+        }
         $fileName = PathResolver::realpath($fileName);
         $this->context = $context ?: ReflectionEngine::getReflectionContext();
         if (!$namespaceNode) {
@@ -179,7 +187,7 @@ public function getDocComment()
         $comments   = $this->namespaceNode->getAttribute('comments');
 
         if ($comments) {
-            $docComment = (string) $comments[0];
+            $docComment = (string)$comments[0];
         }
 
         return $docComment;

diff --git a/src/ReflectionMethod.php b/src/ReflectionMethod.php
@@ -83,7 +83,9 @@ public function ___debugInfo()
      */
     public function __toString()
     {
-        $hasReturnType    = $this->hasReturnType();
+        // Internally $this->getReturnType() !== null is the same as $this->hasReturnType()
+        $returnType       = $this->getReturnType();
+        $hasReturnType    = $returnType !== null;
         $paramsNeeded     = $hasReturnType || $this->getNumberOfParameters() > 0;
         $paramFormat      = $paramsNeeded ? "\n\n  - Parameters [%d] {%s\n  }" : '';
         $returnFormat     = $hasReturnType ? "\n  - Return [ %s ]" : '';
@@ -110,14 +112,19 @@ public function __toString()
             $this->isFinal() ? ' final' : '',
             $this->isStatic() ? ' static' : '',
             $this->isAbstract() ? ' abstract' : '',
-            join(' ', \Reflection::getModifierNames($this->getModifiers() & 1792)),
+            join(
+                ' ',
+                \Reflection::getModifierNames(
+                    $this->getModifiers() & (self::IS_PUBLIC | self::IS_PROTECTED | self::IS_PRIVATE)
+                )
+            ),
             $this->getName(),
             $this->getFileName(),
             $this->getStartLine(),
             $this->getEndLine(),
             count($methodParameters),
             $paramString,
-            $hasReturnType ? ReflectionType::convertToDisplayType($this->getReturnType()) : ''
+            $returnType ? ReflectionType::convertToDisplayType($returnType) : ''
         );
     }
 

diff --git a/src/ReflectionParameter.php b/src/ReflectionParameter.php
@@ -140,7 +140,7 @@ public function __toString()
             }
             /* @see https://3v4l.org/DJOEb for behaviour changes */
             if (is_double($defaultValue) && fmod($defaultValue, 1.0) === 0.0) {
-                $defaultValue = (int) $defaultValue;
+                $defaultValue = (int)$defaultValue;
             }
 
             $defaultValue = str_replace('\\\\', '\\', var_export($defaultValue, true));

diff --git a/src/ReflectionType.php b/src/ReflectionType.php
@@ -85,7 +85,7 @@ public static function convertToDisplayType(\ReflectionType $type)
             'int'  => 'integer',
             'bool' => 'boolean'
         ];
-        $displayType = $type->type;
+        $displayType = (string)$type;
         if (isset($typeMap[$displayType])) {
             $displayType = $typeMap[$displayType];
         }

diff --git a/src/Traits/ReflectionClassLikeTrait.php b/src/Traits/ReflectionClassLikeTrait.php
@@ -328,6 +328,7 @@ public function getInterfaces()
 
     /**
      * {@inheritdoc}
+     * @param string $name
      */
     public function getMethod($name)
     {
@@ -598,6 +599,7 @@ public function hasConstant($name)
 
     /**
      * {@inheritdoc}
+     * @param string $name
      */
     public function hasMethod($name)
     {
@@ -628,6 +630,7 @@ public function hasProperty($name)
 
     /**
      * {@inheritDoc}
+     * @param string $interfaceName
      */
     public function implementsInterface($interfaceName)
     {
@@ -704,7 +707,6 @@ public function isInstance($object)
         }
 
         $className = $this->getName();
-
         return $className === get_class($object) || is_subclass_of($object, $className);
     }
 
@@ -960,4 +962,15 @@ private function collectSelfConstants()
             }
         }
     }
+
+    /**
+     * Create a ReflectionClass for a given class name.
+     *
+     * @param string $className
+     *     The name of the class to create a reflection for.
+     *
+     * @return ReflectionClass
+     *     The apropriate reflection object.
+     */
+    abstract protected function createReflectionForClass($className);
 }
diff --git a/src/Traits/ReflectionFunctionLikeTrait.php b/src/Traits/ReflectionFunctionLikeTrait.php
@@ -227,6 +227,15 @@ public function getStartLine()
      */
     public function getStaticVariables()
     {
+        // In nikic/PHP-Parser < 2.0.0 the default behavior is cloning
+        //     nodes when traversing them. Passing FALSE to the constructor
+        //     prevents this.
+        // In nikic/PHP-Parser >= 2.0.0 and < 3.0.0 the default behavior was
+        //     changed to not clone nodes, but the parameter was retained as
+        //     an option.
+        // In nikic/PHP-Parser >= 3.0.0 the option to clone nodes was removed
+        //     as a constructor parameter, so Scrutinizer will pick this up as
+        //     an issue. It is retained for legacy compatibility.
         $nodeTraverser      = new NodeTraverser(false);
         $variablesCollector = new StaticVariablesCollector($this, $this->context);
         $nodeTraverser->addVisitor($variablesCollector);
@@ -281,6 +290,15 @@ public function isDeprecated()
      */
     public function isGenerator()
     {
+        // In nikic/PHP-Parser < 2.0.0 the default behavior is cloning
+        //     nodes when traversing them. Passing FALSE to the constructor
+        //     prevents this.
+        // In nikic/PHP-Parser >= 2.0.0 and < 3.0.0 the default behavior was
+        //     changed to not clone nodes, but the parameter was retained as
+        //     an option.
+        // In nikic/PHP-Parser >= 3.0.0 the option to clone nodes was removed
+        //     as a constructor parameter, so Scrutinizer will pick this up as
+        //     an issue. It is retained for legacy compatibility.
         $nodeTraverser = new NodeTraverser(false);
         $nodeDetector  = new GeneratorDetector();
         $nodeTraverser->addVisitor($nodeDetector);

diff --git a/src/ValueResolver/NodeExpressionResolver.php b/src/ValueResolver/NodeExpressionResolver.php
@@ -119,8 +119,7 @@ protected function resolve(Node $node)
         try {
             ++$this->nodeLevel;
 
-            $nodeType   = $node->getType();
-            $methodName = 'resolve' . str_replace('_', '', $nodeType);
+            $methodName = $this->getDispatchMethodFor($node);
             if (method_exists($this, $methodName)) {
                 $value = $this->$methodName($node);
             }
@@ -148,8 +147,8 @@ protected function resolveScalarString(Scalar\String_ $node)
 
     protected function resolveScalarMagicConstMethod()
     {
-        if ($this->subject instanceof \ReflectionMethod) {
-            $fullName = $this->subject->getDeclaringClass()->getName() . '::' . $this->subject->getShortName();
+        if ($this->context instanceof \ReflectionMethod) {
+            $fullName = $this->context->getDeclaringClass()->getName() . '::' . $this->context->getShortName();
 
             return $fullName;
         }
@@ -181,13 +180,13 @@ protected function resolveScalarMagicConstNamespace()
 
     protected function resolveScalarMagicConstClass()
     {
-        if ($this->subject instanceof \ReflectionClass) {
-            return $this->subject->getName();
+        if ($this->context instanceof \ReflectionClass) {
+            return $this->context->getName();
         }
         if (method_exists($this->subject, 'getDeclaringClass')) {
             $declaringClass = $this->subject->getDeclaringClass();
             if ($declaringClass instanceof \ReflectionClass) {
-                return $declaringClass->getName();
+                return $declaringClass->name;
             }
         }
 
@@ -219,8 +218,8 @@ protected function resolveScalarMagicConstLine(MagicConst\Line $node)
 
     protected function resolveScalarMagicConstTrait()
     {
-        if ($this->subject instanceof \ReflectionClass && $this->subject->isTrait()) {
-            return $this->subject->getName();
+        if ($this->context instanceof \ReflectionClass && $this->context->isTrait()) {
+            return $this->context->getName();
         }
 
         return '';
@@ -231,8 +230,6 @@ protected function resolveExprConstFetch(Expr\ConstFetch $node)
         $constantValue = null;
         $isResolved    = false;
 
-        /** @var ReflectionFileNamespace|null $fileNamespace */
-        $fileNamespace = null;
         $isFQNConstant = $node->name instanceof Node\Name\FullyQualified;
         $constantName  = $node->name->toString();
 
@@ -263,7 +260,22 @@ protected function resolveExprConstFetch(Expr\ConstFetch $node)
 
     protected function resolveExprClassConstFetch(Expr\ClassConstFetch $node)
     {
-        $refClass     = $this->fetchReflectionClass($node->class);
+        $classToReflect = $node->class;
+        if (!($classToReflect instanceof Node\Name)) {
+            $classToReflect = $this->resolve($classToReflect) ?: $classToReflect;
+            if (!is_string($classToReflect)) {
+                $reason = 'Unable';
+                if ($classToReflect instanceof Expr) {
+                    $methodName = $this->getDispatchMethodFor($classToReflect);
+                    $reason = "Method " . __CLASS__ . "::{$methodName}() not found trying";
+                }
+                throw new ReflectionException("$reason to resolve class constant.");
+            }
+            // Strings evaluated as class names are always treated as fully
+            // qualified.
+            $classToReflect = new Node\Name\FullyQualified(ltrim($classToReflect, '\\'));
+        }
+        $refClass = $this->fetchReflectionClass($classToReflect);
         $constantName = $node->name;
 
         // special handling of ::class constants
@@ -272,7 +284,7 @@ protected function resolveExprClassConstFetch(Expr\ClassConstFetch $node)
         }
 
         $this->isConstant = true;
-        $this->constantName = (string)$node->class . '::' . $constantName;
+        $this->constantName = (string)$classToReflect . '::' . $constantName;
 
         return $refClass->getConstant($constantName);
     }
@@ -437,6 +449,12 @@ protected function resolveExprBinaryOpLogicalXor(Expr\BinaryOp\LogicalXor $node)
         return $this->resolve($node->left) xor $this->resolve($node->right);
     }
 
+    private function getDispatchMethodFor(Node $node)
+    {
+        $nodeType = $node->getType();
+        return 'resolve' . str_replace('_', '', $nodeType);
+    }
+
     /**
      * Utility method to fetch reflection class instance by name
      *

diff --git a/src/bootstrap.php b/src/bootstrap.php
@@ -12,10 +12,9 @@
 use Go\ParserReflection\ReflectionEngine;
 
 /**
- * This file is used for automatic configuration of Go\ParserReflection\ReflectionEngine class
+ * This file is used for automatic configuration of
+ * Go\ParserReflection\ReflectionEngine class
  */
 ReflectionEngine::init(new ComposerLocator());
-// Polifyll for PHP<7.0
-if (!class_exists(ReflectionType::class, false)) {
-    class ReflectionType {}
-}
+
+require(__DIR__ . '/polyfill.php');
diff --git a/src/polyfill.php b/src/polyfill.php
@@ -0,0 +1,34 @@
+<?php
+/**
+ * Parser Reflection API
+ *
+ * @copyright Copyright 2015, Lisachenko Alexander <[email protected]>
+ *
+ * This source file is subject to the license that is bundled
+ * with this source code in the file LICENSE.
+ */
+
+/**
+ * This file is for ployfilling classes not defined in all supported
+ * versions of PHP, (i.e. PHP < 7).
+ */
+if (!class_exists(ReflectionType::class, false)) {
+    /* Dummy polyfill class */
+    class ReflectionType
+    {
+        public function allowsNull()
+        {
+            return true;
+        }
+
+        public function isBuiltin()
+        {
+            return false;
+        }
+
+        public function __toString()
+        {
+            return '';
+        }
+    }
+}
diff --git a/tests/ReflectionFileNamespaceTest.php b/tests/ReflectionFileNamespaceTest.php
@@ -26,6 +26,24 @@ protected function setUp()
         include_once $fileName;
     }
 
+    /**
+     * @expectedException        InvalidArgumentException
+     * @expectedExceptionMessage $fileName must be a string, but a array was passed
+     */
+    public function testBadFilenameTypeArray()
+    {
+        new ReflectionFileNamespace([1, 3, 5, 7], 'BogusNamespace');
+    }
+
+    /**
+     * @expectedException        InvalidArgumentException
+     * @expectedExceptionMessage $fileName must be a string, but a object was passed
+     */
+    public function testBadFilenameTypeObject()
+    {
+        new ReflectionFileNamespace(new \DateTime(), 'BogusNamespace');
+    }
+
     public function testGetClass()
     {
         $refClass = $this->parsedRefFileNamespace->getClass('Unknown');

diff --git a/tests/ReflectionFileTest.php b/tests/ReflectionFileTest.php
@@ -21,6 +21,24 @@ protected function setUp()
         $this->parsedRefFile = $reflectionFile;
     }
 
+    /**
+     * @expectedException        InvalidArgumentException
+     * @expectedExceptionMessage $fileName must be a string, but a array was passed
+     */
+    public function testBadFilenameTypeArray()
+    {
+        new ReflectionFile([1, 3, 5, 7]);
+    }
+
+    /**
+     * @expectedException        InvalidArgumentException
+     * @expectedExceptionMessage $fileName must be a string, but a object was passed
+     */
+    public function testBadFilenameTypeObject()
+    {
+        new ReflectionFile(new \DateTime());
+    }
+
     public function testGetName()
     {
         $fileName     = $this->parsedRefFile->getName();