Visitar URL original
Adding named parameters support to sql requests by lolo3129 · Pull Request #731 · flightphp/core · GitHub
Skip to content

Adding named parameters support to sql requests - #731

Open
lolo3129 wants to merge 1 commit into
flightphp:masterfrom
lolo3129:master
Open

lolo3129 wants to merge 1 commit into
flightphp:masterfrom
lolo3129:master

Conversation

@lolo3129

Copy link
Copy Markdown

there is a bug when using named parameters in sql request.
see added tests in the PR
function processInStatementSql reset array keys and drop values without this suggested fix

@ambrose5773

ambrose5773 commented Nov 14, 2026 •

Copy link
Copy Markdown
Collaborator

Hey, thanks for jumping in on this one! Getting named params to work inside IN(...) is a really nice addition, and the tests for the key-reset bug in processInStatementSql are great to have.

I kicked off CI and everything passes (including all the existing positional IN(?) tests, which is awesome for backwards compat) except one: testInStatementWithMixedParams1 blows up on every PHP version with SQLSTATE[HY000]: General error: 25 column index out of range (SimplePdo.php:252). Run here.

Looking at it, that test does id > ? but passes 'id' => 0 as a named key, so there's no positional value for the ? to bind to, right? PDO doesn't really let you mix ? and :name in one statement either, so I'm not sure that case can work as written. Would you agree it makes sense to either fix the test so the params line up, or drop the mixed case (maybe throw a clear exception for it instead)?

Thanks again for helping maintain Flight, really appreciate it!

@lolo3129

Copy link
Copy Markdown
Author

hello,
Thanks for checking !

You're right, PDO cannot really mix params.
We can indeed throw an exception on that one. another option is to transform all named parameters to unamed ones, but may be a bit more complex as it can be shuffled

I don't really know if it's possible to change the PR ?
or if that move need a new one ?

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