Skip to content

ext/pgsql: Refactor parameter parsing to remove manual usage of zend_wrong_parameters_count_error() #20068

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Nov 5, 2025
Merged

ext/pgsql: Refactor parameter parsing to remove manual usage of zend_wrong_parameters_count_error() #20068

merged 1 commit into from
Nov 5, 2025
+93 −72

Conversation

@Girgias
Copy link
Member

@Girgias Girgias commented Oct 5, 2025

Extract pgsql changes from #20066

ndossche
ndossche requested changes Oct 5, 2025
View reviewed changes
Copy link
Member

@ndossche ndossche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, see 0a3442f and a previous commit of me.

@Girgias
Copy link
Member Author

Girgias commented Oct 5, 2025

Blerg, really can't wait to get rid of this jank...

@Girgias Girgias force-pushed the pgsql-zpp-refacto branch from 8a745ec to 346df2d Compare October 5, 2025 12:18
@Girgias Girgias requested a review from ndossche October 5, 2025 12:22
@Girgias Girgias marked this pull request as ready for review October 5, 2025 14:16
@Girgias Girgias requested a review from devnexen as a code owner October 5, 2025 14:16
ndossche
ndossche requested changes Oct 5, 2025
View reviewed changes
Copy link
Member

@ndossche ndossche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More readable indeed

@Girgias Girgias requested a review from ndossche October 12, 2025 22:54
@Girgias Girgias mentioned this pull request Oct 12, 2025
Open
ndossche
ndossche approved these changes Oct 13, 2025
View reviewed changes
Copy link
Member

@ndossche ndossche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but please see my question.
Also leaving final review for @devnexen as he has worked more on this code than I have.

ext/pgsql/pgsql.c Outdated
zval dataset;
php_pgsql_fetch_hash(&dataset, result, row, row_is_null, PGSQL_ASSOC);

object_init_ex(return_value, ce);
Copy link
Member

@ndossche ndossche Oct 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may be a pre-existing issue: but can't you make this object_init_ex call fail by providing an uninstantiable class (e.g. an interface, abstract class, ...). In that case, the code below may break.

Copy link
Member

@devnexen devnexen Oct 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that sounds possible and very much a bug fix to me.

Copy link
Member Author

@Girgias Girgias Oct 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess first thing is to fix the bug, then rebase on top of it

@Girgias Girgias requested a review from ndossche November 4, 2025 00:32
@ndossche
Copy link
Member

ndossche commented Nov 4, 2025

Please rebase for the test failure to double check.

ndossche
ndossche approved these changes Nov 4, 2025
View reviewed changes
Copy link
Member

@ndossche ndossche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is right

@Girgias Girgias merged commit 129f6e5 into php:master Nov 5, 2025
10 checks passed
@Girgias Girgias deleted the pgsql-zpp-refacto branch November 5, 2025 22:33
@iluuu1994
Copy link
Member

iluuu1994 commented Nov 6, 2025

Is this the cause for this failure on Windows? https://github.com/php/php-src/actions/runs/19121938021/job/54644131975 It happens only on master, and happened in two Windows builds, so it doesn't look like random chance.

@Girgias
Copy link
Member Author

Girgias commented Nov 6, 2025 •
edited
Loading

Yeah, I think I figured out the issue.

See #20407 for the fix

Girgias added a commit to Girgias/php-src that referenced this pull request Nov 6, 2025
@Girgias
Fix pg_fetch_object() when an error occurs in subroutine
81404a2 
This is a follow-up of phpGH-20068
@Girgias Girgias mentioned this pull request Nov 6, 2025
Merged
Girgias added a commit to Girgias/php-src that referenced this pull request Nov 6, 2025
@Girgias
Fix pg_fetch_object() when an error occurs in subroutine
8963663 
This is a follow-up of phpGH-20068
Girgias added a commit that referenced this pull request Nov 6, 2025
@Girgias
Fix pg_fetch_object() when an error occurs in subroutine (#20407)
0953e89 
This is a follow-up of GH-20068
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ndossche ndossche ndossche approved these changes

@devnexen devnexen Awaiting requested review from devnexen devnexen is a code owner

Assignees

No one assigned

Labels

Extension: pgsql

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Girgias @ndossche @iluuu1994 @devnexen