-
Notifications
You must be signed in to change notification settings - Fork 77
#475: Improve error message for unsupported named parameters #476
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.
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
base: master
Are you sure you want to change the base?
#475: Improve error message for unsupported named parameters #476
Conversation
This probably need a few doc/pod updates and one or more tests. |
c782330
to
cc09167
Compare
@dveeden This is my first attempt working on a perl module so if you don't mind a couple of questions...
I couldn't understand why "looks_like_number(1)" failed here https://github.com/perl5-dbi/DBD-mysql/actions/runs/14923375602/job/41923094232#step:10:49 but I changed the code to put the new check under the failure which was originally being triggered. When I |
Also note that MySQL might make it easier to add named attributes in the future: |
Previously just 'Illegal parameter number' was logged, make this a more helpful message. ``` [25-05-06 20:46:04.2629] Slim::Schema::Storage::throw_exception (122) Error: DBI Exception: DBD::mysql::st bind_param failed: named parameters are unsupported: :album [for Statement " SELECT contributor_album.role AS role, contributors.name AS name, contributors.id AS id FROM contributor_album JOIN contributors ON contributors.id = contributor_album.contributor WHERE contributor_album.album = :album AND contributor_album.role IN (5,1) ORDER BY contributor_album.role, contributors.namesort "] ```
cc09167
to
32b7fed
Compare
If a named parameter is passed to bind_param() ensure the expected error message is reported.
@dveeden I've added a new test that the expected error message is reported if This is the output from the test:
I had seen the documentation about named attributes but after some research I realised that its actually a bit misleading. The feature provides a way to annotate a query with, for example, the name of script which invoked the query. |
plan tests => 11; | ||
|
||
SKIP: { | ||
skip 'SET @@auto_increment_offset needs MySQL >= 5.0.2', 2 unless $dbh->{mysql_serverversion} >= 50002; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DBD::mysql needs MySQL 8.0 or newer, so this shouldn't be needed. But on the other hand... it doesn't hurt either.
} | ||
else | ||
{ | ||
do_error(sth, JW_ERR_ILLEGAL_PARAM_NUM, "<param> could not be coerced to a C string", NULL); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a test for this error?
What if there are multiple params. Does it indicate which one is the issue? Could/should it?
@@ -1725,6 +1725,10 @@ parameter value and C<execute> the statement again, with other values | |||
unchanged. The attribute remains properly populated after the C<finish> | |||
method is called, with the values from the last execution. | |||
|
|||
MySQL does not support named place holders in C<bind_param>. If a | |||
string is passed to C<bind_param> as the parameter index then a | |||
"named parameters are unsupported" error is reported. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"named parameters are unsupported" error is reported. | |
`JW_ERR_ILLEGAL_PARAM_NUM` with a message of "named parameters are unsupported" error is reported. |
btw. I think JW
stands for Jim Winstead. See git log
Previously just 'Illegal parameter number' was logged, make this a more helpful message.