Make ext/odbc default value handling more consistent

[kocsismate]

These changes are carved off from https://github.com/php/php-src/pull/12040/files. I noticed that there are some inconsistencies between odbc_fetch_object()/odbc_fetch_array(), odbc_fetch_into(), as well as odbc_fetch_row(), specifically in how they handle the $row parameter.

Now, I tried to align their behaviour the following way:

• I made null the default value. Previously, the default values were one of the following: -1, 0, or null...
• I made the 0 value behave the same way as the default. Previously, odbc_fetch_row() returned a false return value indicating there is no more rows when 0 was passed, but the rest of the functions returned the next available row instead. This change causes a slight BC break for odbc_fetch_row() though...
• When HAVE_SQL_EXTENDED_FETCH is not defined, the $row parameter is always ignored. Previously, some of the functions didn't accept it at all. I think this behavior was not ideal, it would be better to emit a warning or even better, a specific ValueError if the feature is not supported, but the parameter has any value but the default one (null). I can add this error if we can agree on it.

[kocsismate]

this looks like an omission. The result of the affected functions already used the same #ifdefs

[SakiTakamachi]

lgtm.

When HAVE_SQL_EXTENDED_FETCH is not defined, the $row parameter is always ignored. Previously, some of the functions didn't accept it at all. I think this behavior was not ideal, it would be better to emit a warning or even better, a specific ValueError if the feature is not supported, but the parameter has any value but the default one (null). I can add this error if we can agree on it.

I think you need to throw a value error for invalid numbers. In this case, values ​​less than or equal to 0 have no meaning.

By the way, this is outside the scope of this PR, but it seems that SQLExtendedFetch is deprecated in ODBC3.
https://learn.microsoft.com/sql/odbc/reference/syntax/sqlextendedfetch-function?view=sql-server-ver16

[NattyNarwhal]

Related to the comment you made earlier, I wonder how useful the whole EXTENDED_FETCH ifdef mess really is - could we remove it and simplify some stuff? Looking at php_odbc_includes.h, the only ODBC implementations that don't have it are Empress and Solid, neither of which I'm familiar with. Both of them look like embedded databases that are still commercially sold though. The question is if people are still using them with PHP, and if they're using an SQL/CLI interface directly instead of through an ODBC driver and driver manager.

[SakiTakamachi]

It would be a bit unkind to remove it just because it has few or no users.

In particular, since this has no impact on processing speed, I think should leave it in unless have a strong reason not to.

[NattyNarwhal]

That's fair, seeing as I care about a minority database (though I don't think many people are using Db2 directly via the ODBC extension). That ifdef cluster does seem to be a bit of a refactoring burden though.

[SakiTakamachi]

I see. What does the burden of refactoring involve? I found it a little hard to read as to what was inside the else block, is it something similar to that?

[NattyNarwhal]

It was based off of the code review comment left by Máté.

[kocsismate]

I think you need to throw a value error for invalid numbers. In this case, values ​​less than or equal to 0 have no meaning.

Yes, this would be the ideal case, but it would break existing code calling these functions by explicitly using the current default values (0, -1)... So that's why the maximum we can do for now is to emit a warning. However, this is probably also a bit fast pace, since the default values would have changed in the very same version... So I'm personally leaning towards not to do anything for invalid numbers in PHP 8.4. Instead, I'd add a TODO comment recommending to emit a diagnostic error in one of the next major/minor versions.

Related to the comment you made earlier, I wonder how useful the whole EXTENDED_FETCH ifdef mess really is - could we remove it and simplify some stuff?

Hmm, I'm honestly not sure whether anyone is using those DBs... But I agree with Saki that let's not get rid of those IFDEFs. At least I don't care about it enough ^^

[SakiTakamachi]

Yes, this would be the ideal case, but it would break existing code calling these functions by explicitly using the current default values (0, -1)... So that's why the maximum we can do for now is to emit a warning. However, this is probably also a bit fast pace, since the default values would have changed in the very same version... So I'm personally leaning towards not to do anything for invalid numbers in PHP 8.4. Instead, I'd add a TODO comment recommending to emit a diagnostic error in one of the next major/minor versions.

Ah, yes, that's certainly true. I agree to do nothing now.

[kocsismate]

I realized that it's still possible to warn in case of odbc_fetch_row() because null is already the default value since PHP 8.0 (9b50fd2#diff-36f2362bf160e8a01f19ba44e282f7a2f69b460d57a5b23d9e71c59af5de8810R1689).

[kocsismate]

I'm merging this now so that I can then rebase the resource to object PR on top of this. Should you find any problem with this approach I implemented, I'll take care of it in a followup.