Skip to content
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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fix for php 8.1+ #375

wants to merge 2 commits into from

Conversation

DarkVss
Copy link

@DarkVss DarkVss commented Apr 3, 2023

fix by issue

`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);
Copy link

@devjet devjet Aug 15, 2023

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) {

Copy link
Author

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)

Copy link

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

Copy link
Author

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

check original comment for PR ⬆️ or read this issue

Copy link

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.

Copy link
Author

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 '')

Copy link

@eerison eerison Mar 26, 2024

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?

Copy link
Author

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

Copy link

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?

Copy link

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.

@treffynnon
Copy link
Collaborator

This PR needs tests and in addition CI needs to be set up to run against the correct target versions of PHP for testing.

@eerison
Copy link

eerison commented Mar 26, 2024

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.

https://github.com/phpstan/phpstan-deprecation-rules

@treffynnon
Copy link
Collaborator

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.

@eerison
Copy link

eerison commented Mar 26, 2024

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.

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.

4 participants