-
Notifications
You must be signed in to change notification settings - Fork 37
Fix: PHP 8.1 pgsql change of return values #43
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
Conversation
As per https://php.watch/versions/8.1/PgSQL-resource, PostgreSQL extension has moved away from resource objects and is now returning opaque classes (PgSQL\Connection, Result, Lob). These classes no longer provide default conversions to (int), so a different approach is required, while maintaining compatibility to older PHP versions.
Sourcecode changes looks good to me. I haven´t got any postgre database at the moment, but if there is anybody who can approve the funcionality, i will merge this PR in the master branch. |
My previous commit did not check for the right result structure before raising an exception. While looking into this, I have found a deprecation issue with Postgresql server versions >= 12 that needed fixing as well (adsrc column was removed). Context and recommended replacement: https://www.postgresql.org/docs/12/release-12.html#:~:text=obsolete%20pg_attrdef.-,adsrc,-column%20(Peter%20Eisentraut
My dev team looked at the code, and it's in line with what was found when we first encountered the problem with pear and PHP >= 8.1. So look's good enough for me ! |
DB/pgsql.php
Outdated
|
||
if (function_exists('pg_version')) { | ||
$pg_ver = pg_version($this->connection); | ||
$this->pg_major_server_version = intval($pg_ver['server'] ?? 0); |
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.
The null coalescing operator requires PHP 7 or above, so can't be used here.
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.
Minor nitpick, but I'm otherwise happy with this from a code-review perspective.
I don't have any way of testing the code, and I don't feel I have the authority to approve even if I did, but from what I can see this looks good to me.
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.
Approved from a code-review perspective (inspection only).
I do NOT have the ability to run and test this right now.
I tested it (not that I'm more of a sysadmin than a dev), and I no longer have warnings with simple selects. |
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.
Please make changes for backwards compatibility to PHP5.
Thank you.
This completes the decision tree related to handling pg_() internal functions return types and function availability based on PHP versions. The statement is not normally expected to be executed with any current or historical PHP version, but having it seems like a good idea, at least for completeness.
As per https://php.watch/versions/8.1/PgSQL-resource, PostgreSQL
extension has moved away from resource objects and is now
returning opaque classes (PgSQL\Connection, Result, Lob).
These classes no longer provide default conversions to (int), so
a different approach is required, while maintaining compatibility
to older PHP versions.