Skip to content

Conversation

@xPaw
Copy link
Contributor

@xPaw xPaw commented Feb 8, 2022

I think this adds the right pieces.

refs #199

@xPaw
Copy link
Contributor Author

xPaw commented Feb 8, 2022

While adding tests, it seems like it's finding bind calls from previous pdo statements, so the code isn't entirely correct. It needs to stop once it hits prepare or something.

@staabm
Copy link
Owner

staabm commented Feb 8, 2022

For inspiration, a few real world examples can be taken from
https://github.com/shopware/shopware/search?q=bindParam

@xPaw
Copy link
Contributor Author

xPaw commented Feb 10, 2022

I think I fixed it. I manually constructed ConstantArrayType out of the collected bind calls.

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

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

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.

Copy link
Owner

@staabm staabm left a comment

Choose a reason for hiding this comment

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

i love it

@staabm staabm merged commit 6a03ce1 into staabm:main Feb 10, 2022
Comment on lines +46 to +48
$stmt = $pdo->prepare($query);
$stmt->bindValue(':email', '%|'.$string.'|%');
$stmt->execute();
Copy link
Owner

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

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

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

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

Choose a reason for hiding this comment

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

Cool, thx

#275

@xPaw xPaw deleted the pdo-bind-param branch February 10, 2022 11:52
@staabm staabm mentioned this pull request Feb 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants