Update ext/odbc parameter names #6303
Conversation
There was a problem hiding this comment.
This is somewhat connected with #6294 and probably @kocsismate will have similar questions ... in particular whether it should be $stmt or $statement :)
very unclear: the nomenclature of "field"/"column". These are pretty mixed regarding the parameter names, but the function names use "column" only for meta information, and "field" for actual column values. Not sure if that distinction makes sense regarding the parameter names.
I ended up using $field for everything in pgsql, as nearly all APIs had "field" in the name (only one mention of "column"). So I would probably do the same here and stick the terminology this DB driver seems to favor.
| /** @param resource $result_id */ | ||
| function odbc_free_result($result_id): bool {} | ||
| /** @param resource $stmt */ | ||
| function odbc_free_result($stmt): bool {} | ||
|
|
||
| /** @return resource|false */ | ||
| function odbc_connect(string $dsn, string $user, string $password, int $cursor_option = SQL_CUR_USE_DRIVER) {} |
There was a problem hiding this comment.
| function odbc_connect(string $dsn, string $user, string $password, int $cursor_option = SQL_CUR_USE_DRIVER) {} | |
| function odbc_connect(string $dsn, string $user, string $password, int $cursor_type = SQL_CUR_USE_DRIVER) {} |
More in line with $fetch_type above?
There was a problem hiding this comment.
$cursor_type appears to be misleading; this is not about SQL_CURSOR_FORWARD_ONLY, SQL_CURSOR_KEYSET_DRIVEN, SQL_CURSOR_DYNAMIC or SQL_CURSOR_STATIC, but rather whether the ODBC cursor library (a compat layer for minimal drivers, which is deprecated at least on Windows) should be used. I don't have a better idea, so would be okay with $cursor_type).
|
I have no strong opinion on Sticking with |
cmb69
force-pushed
the
cmb/odbc-params
branch
from
095218e to
ebec18f
Compare
There was a problem hiding this comment.
Only have one note, otherwise this looks good to me. @kocsismate ?
ext/odbc/odbc.stub.php
Outdated
| /** @param resource $result_id */ | ||
| function odbc_field_name($result_id, int $field_number): string|false {} | ||
| /** @param resource $statement */ | ||
| function odbc_field_name($statement, int $field_number): string|false {} |
There was a problem hiding this comment.
Elsewhere we do not make a distinction between $field_number and $field_name and just use $field (or $column depending on DB) for both. Whether it's a number or a name (a lot of APIs also accept both) is determined by the type.
(If we want to keep the distinction here, we should $field_number -> $field_num to keep the abbreviation consistent.)
There was a problem hiding this comment.
I'm not sure here. Only odbc_field_num() accepts a name; all other odbc_field_*() functions expect the number (which looks strange to me). (ah, odbc_result() accepts either) But since we have the type, just $field looks good to me.
Some general notes:
$odbc, which appears to be the general convention$stmt. That is likely super controversial, because the method names contain "result" as well, but in ODBC parlance these are actually "statement"/"stmt", and "result" is misleading, sinceodbc_preparereturns a prepared statement, and not a result (set).