Skip to content

ODBC fetch refactoring #19848

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 6 commits into from
Sep 29, 2025
Merged

ODBC fetch refactoring #19848

merged 6 commits into from
Sep 29, 2025

Conversation

@NattyNarwhal
Copy link
Member

@NattyNarwhal NattyNarwhal commented Sep 15, 2025 •
edited
Loading

  • Merge php_odbc_fetch_into into php_odbc_fetch_hash.
    • The implementation was nearly identical, so refactor into a common implementation with some differences in how the result set array is returned.
    • odbc_fetch_into is kind of weird. I don't know if it'd be better to deprecate it and have a more "symmetrical" API that works like odbc_fetch_array.
  • Converts php_odbc_fetch_hash to use ZPP.
    • Opportunistic since I was there anyways.
  • Convert SQLExtendedFetch calls to SQLFetchScroll
  • Convert result type constants to an enum
  • Merge php_odbc_fetch_row into php_odbc_fetch_hash

@NattyNarwhal NattyNarwhal marked this pull request as ready for review September 15, 2025 20:17
@NattyNarwhal
Copy link
Member Author

ARM failure unrelated.

FWIW, if this misses 8.5, I'll probably continue some further refactoring in this PR.

@NattyNarwhal
Merge odbc_fetch_into into odbc_fetch_hash
8a1d8cf 
Now that we can assume fetch_hash exists, there's a lot of redundancy in
these functions. Merge their implementations, and smooth over the
differences in how they handle returning their result set as an array.
@NattyNarwhal
Convert php_odbc_fetch_hash to ZPP
453033a
@NattyNarwhal
Use SQLFetchScroll instead of SQLExtendedFetch
5d91ab4 
Fixes phpGH-19522
@NattyNarwhal
Convert result type constants to an enum
a23cbc3
@NattyNarwhal
Implement odbc_fetch_row in terms of php_odbc_fetch_hash
99bb325 
These are also doing extremely similar jobs, but with slightly different
behaviours for the return value (in this case, none, as it's tended to
be used with odbc_result). Unify this too.

The $row value deprecation for 0/-1 is only handled for odbc_fetch_row;
it's too late to do so for PHP 8.5. Should probably unify it for PHP 8.6.
@NattyNarwhal
Rename php_odbc_fetch_hash to remove _hash
770ce9b 
Since this is a much more shared fetch function now.
Girgias
Girgias reviewed Sep 21, 2025
View reviewed changes
Copy link
Member

@Girgias Girgias left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Can you remove the deprecation? As that would then be a purely refactory PR.

As for the deprecation you would need RM approval and I don't know how they feel about it being this close to RC1 and might be difficult to convince.

@NattyNarwhal
Copy link
Member Author

NattyNarwhal commented Sep 21, 2025 •
edited
Loading

Can you remove the deprecation? As that would then be a purely refactory PR.

As for the deprecation you would need RM approval and I don't know how they feel about it being this close to RC1 and might be difficult to convince.

I changed the condition in the inner function to only trigger when it's called with the parameters odbc_fetch_row would call it with, so this shouldn't change the deprecation. (That said, for 8.6, we should probably actually get around to deprecating it for the other functions too, but as you say, not now.)

Girgias
Girgias approved these changes Sep 21, 2025
View reviewed changes
Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

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

The commit title "Convert php_odbc_fetch_hash to ZPP" is misleading, as you are not converting the function to ZPP but to fast ZPP as it was already using ZPP to parse and validate arguments.

@NattyNarwhal
Copy link
Member Author

@php/release-managers-85 Your call if it's OK to get this in for RC1.

@DanielEScherzer
Copy link
Member

@php/release-managers-85 Your call if it's OK to get this in for RC1.

Sorry, didn't see this in time and 8.5 has branched, so I guess it is now moot

@NattyNarwhal
Copy link
Member Author

No problem, I think I might merge into master then if it looks good. It's possible to backport into the RCs if you want it in there anyways.

@DanielEScherzer
Copy link
Member

If this is just refactoring we shouldn't backport

@NattyNarwhal
Copy link
Member Author

It does replace the use of a deprecated ODBC function, but that's probably not enough for a backport.

@NattyNarwhal NattyNarwhal merged commit 294a080 into php:master Sep 29, 2025
9 checks passed
@NattyNarwhal
Copy link
Member Author

Merging into 8.6/master since this missed 8.5.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Girgias Girgias Girgias approved these changes

Assignees

No one assigned

Labels

Extension: odbc

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Replace SQLExtendedFetch with SQLFetchScroll in ext/odbc

3 participants

@NattyNarwhal @DanielEScherzer @Girgias