close
The Wayback Machine - https://web.archive.org/web/20210428221536/https://github.com/php/php-src/pull/6801
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 bug #80892 PDO::PARAM_INT on pdo_pgsql #6801

Closed
wants to merge 1 commit into from
Closed

Conversation

@mbeccati
Copy link
Contributor

@mbeccati mbeccati commented Mar 23, 2021

Created PR for review - I will commit to the appropriate branches when ready.

@mbeccati
Copy link
Contributor Author

@mbeccati mbeccati commented Apr 12, 2021

Merged in 340a067

@mbeccati mbeccati closed this Apr 12, 2021
@mbeccati mbeccati deleted the mbeccati:bug80892 branch Apr 12, 2021
@taylorotwell
Copy link

@taylorotwell taylorotwell commented Apr 28, 2021

@mbeccati @nikic

Hey all - we got a PR related to this change on Laravel: laravel/framework#37168

In summary, previously if you had a boolean column on a Postgres table, you could bind an integer value (0 or 1 typically) in the where condition against that column using PDO::PARAM_INT and everything was fine. However, you will now receive an error.

That's OK I guess, although fairly unexpected and large breaking change on a patch release?

In addition, if we try to adjust Laravel to bind all booleans using PDO::PARAM_BOOL that breaks applications that were previously doing something like querying a smallint column by binding a integer casted boolean value using PDO::PARAM_INT - since you can't bind a PDO::PARAM_BOOL binding when querying a smallint column. I don't think that is really PHP's problem because we were automatically casting booleans to integers when making Postgres queries and always binding using PDO::PARAM_INT.

But, it does put us in a pretty tough spot, since if we don't merge this PR change, people can no longer perform simple boolean queries in Laravel using PHP 8.0.5, while if we do merge it, we break all applications querying smallint columns via booleans - and we don't have a good way of determining what they are actually intended to do. Admittedly, it seems like querying a smallint column by passing true or false to represent 1 or 0 is not exactly the best approach to querying "boolean like" columns in Postgres, but people do it. 🤷

Ultimately, if this PR stands, we would just need to remove this convenience boolean to integer casting when working with Postgres and people could no longer pass booleans in our query builders when querying if a smallint column is 0 or 1 - and that's generally fine I guess - but just wanted to check with y'all first on if you consider this breaking change of not being being able to bind a PDO::PARAM_INT binding against a boolean column as too large of a breaking change to let pass on a patch release.

Any thoughts?

/cc @driesvints

if (PDO_PARAM_TYPE(param->param_type) == PDO_PARAM_LOB) {
if (PDO_PARAM_TYPE(param->param_type) == PDO_PARAM_INT) {
/* we need to check if the number requires bigints */
long long val = strtoll(Z_STRVAL_P(parameter), NULL, 10);

This comment has been minimized.

@nikic

nikic Apr 28, 2021
Member

Without having looked into Taylor's issue yet, this code is clearly broken :( If the original value was null or bool, then it will stay as such, while this code assumes the value is always a string. Passing a null to a PARAM_INT binding causes a segfault now (bool doesn't for some reason).

This comment has been minimized.

@mbeccati

mbeccati Apr 28, 2021
Author Contributor

Indeed I thought I had better coverage :(

@mbeccati
Copy link
Contributor Author

@mbeccati mbeccati commented Apr 28, 2021

Thanks @taylorotwell - I will have a better look tomorrow. Also cc-ing @derickr @carusogabriel as RMs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants