-
Notifications
You must be signed in to change notification settings - Fork 370
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
Fix for php 8.1+ #375
base: master
Are you sure you want to change the base?
Fix for php 8.1+ #375
Conversation
`PDO::quote(): Passing null to parameter j4mie#1 ($string) of type string is deprecated in idiorm.php on line 539`
$parameters = array_map(array(self::get_db($connection_name), 'quote'), $parameters); | ||
foreach ($parameters as &$parameter) { | ||
if (is_string($parameters) === true) { | ||
$parameter = [self::get_db($connection_name), "quote"]($parameter); |
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.
Probably it has to be:
if (is_string($parameter) === true) {
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.
yeah, u right but its still have no sense cz author dont care about project)) (in my separated version var has right typing)
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.
Hey @devjet
could you say what error is it fixing? it is not clear
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.
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.
Ahhh I see, it's a deprecated message only. Ok then it will be a problem on php 9 only, maybe you could inform it on your title, just to be clear it is fixing an deprecation message.
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.
Ahhh I see, it's a deprecated message only. Ok then it will be a problem on php 9 only, maybe you could inform it on your title, just to be clear it is fixing an deprecation message.
in full case it not only disabling deprecated message , this changes make more cleanest SQL code - null as NULL, boolean as BOOLEAN( e.g. TRUE
or FALSE
instead of '1'
and ''
)
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.
cz author dont care about project
Just to point out, that isn't true, he is replying when he is mentioned.
in full case it not only disabling deprecated message , this changes make more cleanest SQL code - null as NULL, boolean as BOOLEAN( e.g. TRUE or FALSE instead of '1' and '')
well if you want to "clean up" the code and introduce a different behaviour you definitely need to write test for it, and it would be nice if you could explain the benefits of this.
Output Deprecated: PDO::quote(): Passing null to parameter #1 ($string) of type string is deprecated in C:\VHOST\VHOST8\idiorm\vendor\j4mie\idiorm\idiorm.php on line 539
checking your message ... it is complain about null value for quote function ... in theory there are null values on $parameter
, But the question is : should $parameter
contain null values?
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.
heaven sake...
at the first: isuue created >2 year, PR >1 y. yeah, author dont care about proj
second: code upgrd for disable message about deprecated passing (read original issue)
thrid: if u dont what this code do - read the code, no explanation for it
P.S.
if u wanna to some rhetoric - go to some chat. i just pushed updates for issue solution, no more. this repo look like abandoned, so this discussion have no sense
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.
so this discussion have no sense
if you don't want move this forward, then it could be closed, couldn't it?
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.
@eerison idiorm has a problem with inserting null values
this is problem because one have to edit something in vendors directory to fix it.
This PR needs tests and in addition CI needs to be set up to run against the correct target versions of PHP for testing. |
Yeah if there isn't anything Testing this line ok, but maybe we should add phpstan deprecation on pipelines. Because as far as I understand it isn't fixing any issue, just a deprecation message. |
The test suite needs to be updated to target the version of php that this PR is attempting to fix. It also needs tests for the logic to change parameterisation of the query. Also, the switch to booleans with the values TRUE and FALSE seems like something that would break MS SQL support so that would need to be tested too. |
Btw, thank you for reply and your effort to maintain this repo. |
fix by issue