Skip to content

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

Merged
merged 6 commits into from
May 1, 2025
Merged

Conversation

DanCld
Copy link
Contributor

@DanCld DanCld commented Mar 21, 2025

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.

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.
@schengawegga
Copy link
Collaborator

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.

@DanCld DanCld marked this pull request as ready for review March 24, 2025 17:16
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
@rossnick
Copy link

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

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.

@DanCld DanCld requested a review from MarkMaldaba April 30, 2025 08:14
Copy link
Contributor

@MarkMaldaba MarkMaldaba left a 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.

@DanCld DanCld requested a review from MarkMaldaba April 30, 2025 10:47
Copy link
Contributor

@MarkMaldaba MarkMaldaba left a 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.

@rossnick
Copy link

I tested it (not that I'm more of a sysadmin than a dev), and I no longer have warnings with simple selects.

Copy link
Collaborator

@schengawegga schengawegga left a 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.
@DanCld DanCld requested a review from schengawegga April 30, 2025 16:27
@DanCld DanCld requested a review from schengawegga May 1, 2025 08:57
@schengawegga schengawegga merged commit a78e8de into pear:trunk May 1, 2025
@schengawegga schengawegga added this to the Version 1.12.3 milestone May 1, 2025
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