Skip to content

Prototype support for bindValue/bindParam on PDOStatement #265

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Feb 10, 2022
Merged

Prototype support for bindValue/bindParam on PDOStatement #265

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5630c82
Add support for PDOStatement bindValue/bindParam
xPaw Feb 10, 2022
8548928
fix
xPaw Feb 10, 2022
1fb1877
fix
xPaw Feb 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions src/PdoReflection/PdoStatementReflection.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,16 @@ public function findPrepareQueryStringExpression(MethodCall $methodCall): ?Expr
return null;
}

/**
* @return MethodCall[]
*/
public function findPrepareBindCalls(MethodCall $methodCall): array
{
$exprFinder = new ExpressionFinder();

return $exprFinder->findBindCalls($methodCall);
}

/**
* Turns a PDO::FETCH_* parameter-type into a QueryReflector::FETCH_TYPE* constant.
*
Expand Down
33 changes: 33 additions & 0 deletions src/QueryReflection/ExpressionFinder.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,39 @@ public function findQueryStringExpression(Expr $expr): ?Expr
return null;
}

/**
* @param MethodCall $expr
*
* @return MethodCall[]
*/
public function findBindCalls(Expr $expr): array
{
$result = [];
$current = $expr;
while (null !== $current) {
/** @var Assign|MethodCall|null $call */
$call = $this->findFirstPreviousOfNode($current, function ($node) {
return $node instanceof MethodCall || $node instanceof Assign;
});

if (null !== $call && $this->resolveName($call->var) === $this->resolveName($expr->var)) {
if ($call instanceof Assign) { // found the prepare call
return $result;
}

$name = $this->resolveName($call);

if (null !== $name && \in_array(strtolower($name), ['bindparam', 'bindvalue'], true)) {
$result[] = $call;
}
}

$current = $call;
}

return $result;
}

/**
* XXX use astral simpleNameResolver instead.
*
Expand Down
36 changes: 28 additions & 8 deletions src/Rules/PdoStatementExecuteMethodRule.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@
use PHPStan\Rules\Rule;
use PHPStan\Rules\RuleError;
use PHPStan\Rules\RuleErrorBuilder;
use PHPStan\Type\Constant\ConstantArrayType;
use PHPStan\Type\Constant\ConstantIntegerType;
use PHPStan\Type\Constant\ConstantStringType;
use PHPStan\Type\MixedType;
use staabm\PHPStanDba\PdoReflection\PdoStatementReflection;
use staabm\PHPStanDba\QueryReflection\PlaceholderValidation;
Expand Down Expand Up @@ -70,16 +73,33 @@ private function checkErrors(MethodReflection $methodReflection, MethodCall $met

$args = $methodCall->getArgs();
if (\count($args) < 1) {
$parameters = [];
$parameterKeys = [];
$parameterValues = [];

$calls = $stmtReflection->findPrepareBindCalls($methodCall);

foreach ($calls as $bindCall) {
$args = $bindCall->getArgs();
if (\count($args) >= 2) {
$keyType = $scope->getType($args[0]->value);
if ($keyType instanceof ConstantIntegerType || $keyType instanceof ConstantStringType) {
Copy link
Owner

@staabm staabm Feb 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need this check here, or could we just feed everything into $parameterKeys and resolveParameters later on would decide which types are supported and which not?

Copy link
Contributor Author

@xPaw xPaw Feb 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's constrained by the constructor, so phpstan complained.

$parameterKeys[] = $keyType;
$parameterValues[] = $scope->getType($args[1]->value);
}
}
}

$parameterTypes = new ConstantArrayType($parameterKeys, $parameterValues);
} else {
$parameterTypes = $scope->getType($args[0]->value);
try {
$parameters = $queryReflection->resolveParameters($parameterTypes) ?? [];
} catch (UnresolvableQueryException $exception) {
return [
RuleErrorBuilder::message($exception->asRuleMessage())->tip(UnresolvableQueryException::RULE_TIP)->line($methodCall->getLine())->build(),
];
}
}

try {
$parameters = $queryReflection->resolveParameters($parameterTypes) ?? [];
} catch (UnresolvableQueryException $exception) {
return [
RuleErrorBuilder::message($exception->asRuleMessage())->tip(UnresolvableQueryException::RULE_TIP)->line($methodCall->getLine())->build(),
];
}

try {
Expand Down
24 changes: 24 additions & 0 deletions tests/default/PdoStatementExecuteMethodRuleTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,30 @@ public function testParameterErrors(): void
'Value :adaid is given, but the query does not contain this placeholder.',
54,
],
[
'Query expects placeholder :adaid, but it is missing from values given.',
61,
],
[
'Value :wrongParamName is given, but the query does not contain this placeholder.',
61,
],
[
'Query expects placeholder :adaid, but it is missing from values given.',
65,
],
[
'Value :wrongParamName is given, but the query does not contain this placeholder.',
65,
],
[
'Query expects placeholder :email, but it is missing from values given.',
69,
],
[
'Query expects placeholder :adaid, but it is missing from values given.',
73,
],
]);
}
}
5 changes: 5 additions & 0 deletions tests/default/UnresolvablePdoStatementRuleTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,11 @@ public function testSyntaxErrorInQueryRule(): void
13,
UnresolvableQueryException::RULE_TIP,
],
[
'Unresolvable Query: Cannot simulate parameter value for type: mixed.',
17,
UnresolvableQueryException::RULE_TIP,
],
]);
}
}
19 changes: 19 additions & 0 deletions tests/default/data/pdo-stmt-execute-error.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,4 +53,23 @@ public function conditionalSyntaxError(PDO $pdo)
$stmt = $pdo->prepare($query);
$stmt->execute(['adaid' => 1]);
}

public function errorsBind(PDO $pdo)
{
$stmt = $pdo->prepare('SELECT email, adaid FROM ada WHERE adaid = :adaid');
$stmt->bindValue('wrongParamName', 1);
$stmt->execute();

$stmt = $pdo->prepare('SELECT email, adaid FROM ada WHERE adaid = :adaid');
$stmt->bindValue(':wrongParamName', 1);
$stmt->execute();

$stmt = $pdo->prepare('SELECT email, adaid FROM ada WHERE adaid = :adaid and email = :email');
$stmt->bindValue(':adaid', 1);
$stmt->execute(); // wrong number of parameters

$stmt = $pdo->prepare('SELECT email, adaid FROM ada WHERE adaid = :adaid and email = :email');
$stmt->bindValue(':email', 'email@example.org');
$stmt->execute(); // wrong number of parameters
}
}
21 changes: 21 additions & 0 deletions tests/default/data/pdo-stmt-execute.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,16 @@ public function execute(PDO $pdo)
assertType('PDOStatement<array{email: string, 0: string, adaid: int<0, 4294967295>, 1: int<0, 4294967295>}'.$bothType.'>', $stmt);
}

public function executeWithBindCalls(PDO $pdo)
{
$stmt = $pdo->prepare('SELECT email, adaid FROM ada WHERE email = :test1 AND email = :test2');
$test = 1337;
$stmt->setFetchMode(PDO::FETCH_ASSOC);
$stmt->bindParam(':test1', $test);
$stmt->bindValue(':test2', 1001);
$stmt->execute();
}

public function errors(PDO $pdo)
{
$stmt = $pdo->prepare('SELECT email, adaid FROM ada WHERE adaid = :adaid');
Expand All @@ -58,5 +68,16 @@ public function errors(PDO $pdo)
assertType('PDOStatement', $stmt);
$stmt->execute(); // missing parameter
assertType('PDOStatement', $stmt);

$stmt = $pdo->prepare('SELECT email, adaid FROM ada WHERE adaid = :adaid');
assertType('PDOStatement', $stmt);
$stmt->bindValue(':wrongParamName', 1);
assertType('PDOStatement', $stmt);

$stmt = $pdo->prepare('SELECT email, adaid FROM ada WHERE adaid = :adaid');
assertType('PDOStatement', $stmt);
$stmt->bindValue(':wrongParamValue', 'hello world');
$stmt->execute();
assertType('PDOStatement', $stmt);
}
}
8 changes: 8 additions & 0 deletions tests/default/data/unresolvable-pdo-statement.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,10 @@ public function mixedParam(PDO $pdo, $mixed)
$query = 'SELECT email FROM ada WHERE gesperrt=:gesperrt';
$stmt = $pdo->prepare($query);
$stmt->execute([':gesperrt' => $mixed]);

$stmt = $pdo->prepare($query);
$stmt->bindValue(':gesperrt', $mixed);
$stmt->execute();
}

public function noErrorOnMixedQuery(PDO $pdo, $mixed)
Expand Down Expand Up @@ -38,5 +42,9 @@ public function noErrorOnStringValue(PDO $pdo, string $string)
$query = 'SELECT adaid FROM ada WHERE email=:email';
$stmt = $pdo->prepare($query);
$stmt->execute([':email' => '%|'.$string.'|%']);

$stmt = $pdo->prepare($query);
$stmt->bindValue(':email', '%|'.$string.'|%');
$stmt->execute();
Comment on lines +46 to +48
Copy link
Owner

@staabm staabm Feb 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can/should cover a case where some params are bound via bindValue or bindParam and others are given to execute at the same time.. I guess this is supported in pdo?

Copy link
Contributor Author

@xPaw xPaw Feb 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked at the pdo code, and it seemed like if you pass into execute, it empties any of the previously bound variables. I may have misunderstood though.

Copy link
Owner

@staabm staabm Feb 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would be cool if you could run a small example on your own and verify the thesis.

if its right, we might even create a PHPStan-Rule which errors on bindValue/.. calls, when parameters are passed to execute

Copy link
Contributor Author

@xPaw xPaw Feb 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it's the case:

	$st = Container::Database()->prepare( 'SELECT * FROM Apps WHERE AppID = :a OR AppID = :b' );
	$st->bindValue( ':a', 123 );
	$st->execute( [ ':b' => 456 ] ); // SQLSTATE[HY093]: Invalid parameter number

Copy link
Owner

@staabm staabm Feb 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, thx

#275

}
}