Fix false positive with unnamed placeholders (#628)

staabm · Sep 29, 2023 · 70a0fe7 · 70a0fe7
1 parent c473688
commit 70a0fe7
Show file tree

Hide file tree

Showing 4 changed files with 90 additions and 14 deletions.
diff --git a/src/QueryReflection/PlaceholderValidation.php b/src/QueryReflection/PlaceholderValidation.php
@@ -34,18 +34,27 @@ public function checkQuery(Expr $queryExpr, Scope $scope, array $parameters): it
             return;
         }
 
+        $minPlaceholderCount = PHP_INT_MAX;
+        $maxPlaceholderCount = 0;
         foreach ($queryStrings as $queryString) {
             $placeholderCount = $queryReflection->countPlaceholders($queryString);
-            yield from $this->validateUnnamedPlaceholders($parameters, $placeholderCount);
+            if ($placeholderCount < $minPlaceholderCount) {
+                $minPlaceholderCount = $placeholderCount;
+            }
+            if ($placeholderCount > $maxPlaceholderCount) {
+                $maxPlaceholderCount = $placeholderCount;
+            }
         }
+
+        yield from $this->validateUnnamedPlaceholders($parameters, $minPlaceholderCount, $maxPlaceholderCount);
     }
 
     /**
      * @param array<string|int, Parameter> $parameters
      *
      * @return iterable<string>
      */
-    private function validateUnnamedPlaceholders(array $parameters, int $placeholderCount): iterable
+    private function validateUnnamedPlaceholders(array $parameters, int $minPlaceholderCount, int $maxPlaceholderCount): iterable
     {
         $parameterCount = \count($parameters);
         $minParameterCount = 0;
@@ -56,14 +65,22 @@ private function validateUnnamedPlaceholders(array $parameters, int $placeholder
             ++$minParameterCount;
         }
 
-        if (0 === $parameterCount && 0 === $minParameterCount && 0 === $placeholderCount) {
+        if (0 === $parameterCount
+            && 0 === $minParameterCount
+            && 0 === $minPlaceholderCount
+            && 0 === $maxPlaceholderCount
+        ) {
             return;
         }
 
-        if ($parameterCount !== $placeholderCount && $placeholderCount !== $minParameterCount) {
-            $placeholderExpectation = sprintf('Query expects %s placeholder', $placeholderCount);
-            if ($placeholderCount > 1) {
-                $placeholderExpectation = sprintf('Query expects %s placeholders', $placeholderCount);
+        if ($parameterCount > $maxPlaceholderCount || $minParameterCount < $minPlaceholderCount) {
+            $placeholderExpectation = sprintf('Query expects %s placeholder', $minPlaceholderCount);
+            if ($minPlaceholderCount > 1) {
+                if ($minPlaceholderCount !== $maxPlaceholderCount) {
+                    $placeholderExpectation = sprintf('Query expects %s-%s placeholders', $minPlaceholderCount, $maxPlaceholderCount);
+                } else {
+                    $placeholderExpectation = sprintf('Query expects %s placeholders', $minPlaceholderCount);
+                }
             }
 
             if (0 === $parameterCount) {

diff --git a/src/QueryReflection/QueryReflection.php b/src/QueryReflection/QueryReflection.php
@@ -419,14 +419,23 @@ public function resolveParameters(Type $parameterTypes): ?array
 
         if ($parameterTypes instanceof UnionType) {
             foreach ($parameterTypes->getConstantArrays() as $constantArray) {
-                $parameters = $parameters + $this->resolveConstantArray($constantArray, true);
+                foreach ($this->resolveConstantArray($constantArray) as $key => $resolvedParameter) {
+                    if (array_key_exists($key, $parameters)) {
+                        // required parameters should overrule optional parameters
+                        if (! $resolvedParameter->isOptional) {
+                            $parameters[$key] = $resolvedParameter;
+                        }
+                    } else {
+                        $parameters[$key] = $resolvedParameter;
+                    }
+                }
             }
 
             return $parameters;
         }
 
         if ($parameterTypes instanceof ConstantArrayType) {
-            return $this->resolveConstantArray($parameterTypes, false);
+            return $this->resolveConstantArray($parameterTypes);
         }
 
         return null;
@@ -437,7 +446,7 @@ public function resolveParameters(Type $parameterTypes): ?array
      *
      * @throws UnresolvableQueryException
      */
-    private function resolveConstantArray(ConstantArrayType $parameterTypes, bool $forceOptional): array
+    private function resolveConstantArray(ConstantArrayType $parameterTypes): array
     {
         $parameters = [];
 
@@ -447,9 +456,6 @@ private function resolveConstantArray(ConstantArrayType $parameterTypes, bool $f
 
         foreach ($keyTypes as $i => $keyType) {
             $isOptional = \in_array($i, $optionalKeys, true);
-            if ($forceOptional) {
-                $isOptional = true;
-            }
 
             if ($keyType instanceof ConstantStringType) {
                 $placeholderName = $keyType->getValue();

diff --git a/tests/rules/PdoStatementExecuteMethodRuleTest.php b/tests/rules/PdoStatementExecuteMethodRuleTest.php
@@ -115,7 +115,16 @@ public function testParameterErrors(): void
 
     public function testNamedPlaceholderBug(): void
     {
-        $this->analyse([__DIR__ . '/data/named-placeholder-bug.php'], [
+        $this->analyse([__DIR__ . '/data/named-placeholder-bug.php'], []);
+    }
+
+    public function testPlaceholderBug(): void
+    {
+        $this->analyse([__DIR__ . '/data/placeholder-bug.php'], [
+            [
+                'Query expects 2-3 placeholders, but 1-3 values are given.',
+                42,
+            ],
         ]);
     }
 }
diff --git a/tests/rules/data/placeholder-bug.php b/tests/rules/data/placeholder-bug.php
@@ -0,0 +1,44 @@
+<?php
+
+namespace PlaceholderBug;
+
+
+use PDO;
+
+class Foo
+{
+    public function allGood(PDO $pdo, $vkFrom)
+    {
+        $values = [];
+        $values[] = 1;
+
+        $fromCondition = '';
+        if ('0' !== $vkFrom) {
+            $fromCondition = 'and email = ?';
+            $values[] = 'hello world';
+        }
+
+
+        $stmt = $pdo->prepare('SELECT email, adaid FROM ada WHERE adaid = ? ' . $fromCondition);
+        $stmt->execute($values);
+    }
+
+    public function sometimesWrongNumberOfParameters(PDO $pdo, $vkFrom)
+    {
+        $values = [];
+
+        $values[] = 1;
+        if (rand(0,1)) {
+            $values[] = 10;
+        }
+
+        $fromCondition = '';
+        if ('0' !== $vkFrom) {
+            $fromCondition = 'and email = ?';
+            $values[] = 'hello world';
+        }
+
+        $stmt = $pdo->prepare('SELECT email, adaid FROM ada WHERE adaid = ? OR adaid = ? ' . $fromCondition);
+        $stmt->execute($values);
+    }
+}