Skip to content

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

Merged
Girgias merged 1 commit into
php:masterfrom
Girgias:pgsql-zpp-refacto
Nov 5, 2025
Merged

ext/pgsql: Refactor parameter parsing to remove manual usage of zend_wrong_parameters_count_error()#20068
Girgias merged 1 commit into
php:masterfrom
Girgias:pgsql-zpp-refacto

Conversation

@Girgias

@GirgiasGirgias commented Oct 5, 2025

Copy link
Copy Markdown
Member

Extract pgsql changes from #20066

ndossche
ndossche requested changes Oct 5, 2025
View reviewed changes

@ndosschendossche left a comment

Copy link
Copy Markdown
Member

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

Girgias commented Oct 5, 2025

Copy link
Copy Markdown
MemberAuthor

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

@Girgias
Girgias requested a review from ndosscheOctober 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 ownerOctober 5, 2025 14:16
ndossche
ndossche requested changes Oct 5, 2025
View reviewed changes

@ndosschendossche left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

More readable indeed

Comment threadext/pgsql/pgsql.c
Comment threadext/pgsql/pgsql.c Outdated
Comment threadext/pgsql/pgsql.c
Comment threadext/pgsql/pgsql.c
Comment threadext/pgsql/pgsql.c
Comment threadext/pgsql/pgsql.c
@Girgias
Girgias requested a review from ndosscheOctober 12, 2025 22:54
@GirgiasGirgias mentioned this pull request Oct 12, 2025
Merged
ndossche
ndossche approved these changes Oct 13, 2025
View reviewed changes

@ndosschendossche left a comment

Copy link
Copy Markdown
Member

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.

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

object_init_ex(return_value, ce);

@ndosschendosscheOct 13, 2025

Copy link
Copy Markdown
Member

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.

@devnexendevnexenOct 13, 2025

Copy link
Copy Markdown
Member

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.

@GirgiasGirgiasOct 13, 2025

Copy link
Copy Markdown
MemberAuthor

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 ndosscheNovember 4, 2025 00:32
@ndossche

ndossche commented Nov 4, 2025

Copy link
Copy Markdown
Member

Please rebase for the test failure to double check.

ndossche
ndossche approved these changes Nov 4, 2025
View reviewed changes

@ndosschendossche left a comment

Copy link
Copy Markdown
Member

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:masterNov 5, 2025
10 checks passed
@Girgias
Girgias deleted the pgsql-zpp-refacto branch November 5, 2025 22:33
@iluuu1994

iluuu1994 commented Nov 6, 2025

Copy link
Copy Markdown
Member

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

Girgias commented Nov 6, 2025•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

Yeah, I think I figured out the issue.

See #20407 for the fix

@GirgiasGirgias 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 freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ndosschendosschendossche approved these changes
@devnexendevnexenAwaiting requested review from devnexendevnexen 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