Skip to content

Fix UNKNOWN default values in ext/odbc #6154

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
Closed
wants to merge 3 commits into from
Closed

Fix UNKNOWN default values in ext/odbc #6154

wants to merge 3 commits into from

Conversation

kocsismate
Copy link
Member

No description provided.

@kocsismate kocsismate force-pushed the odbc-defaults branch 2 times, most recently from dcc7ff1 to 348123d Compare September 17, 2020 17:44
@kocsismate kocsismate mentioned this pull request Sep 17, 2020
Closed
@kocsismate kocsismate force-pushed the odbc-defaults branch 3 times, most recently from 575e372 to 4533257 Compare September 17, 2020 18:02
@nikic
Copy link
Member

nikic commented Sep 18, 2020

AppVeyor failures are legitimate.

@cmb69
Copy link
Member

cmb69 commented Sep 19, 2020 •
edited
Loading

The last 3 parameters of odbc_columns() and odbc_tables() should not be nullable and should default to an empty string. At least, that lets the original odbc_columns_001.phpt and odbc_tables_001.phpt pass. Not sure about the new test clauses in these PHPTs; these may not have worked for PHP 7 as well.

PS: odbc_columns($conn) is supposed to succeed (i.e. returns true). The strange test output is because the result resource isn't properly destroyed without explicitly calling odbc_free_result() or otherwise freeing it before calling odbc_columns() again (same for odbc_tables()).

@kocsismate
Copy link
Member Author

@cmb69 Thanks for the info! I'll give this PR a try sooner or later! :)

@kocsismate kocsismate force-pushed the odbc-defaults branch 2 times, most recently from 0ecddba to 93994ac Compare September 21, 2020 12:39
@kocsismate
Copy link
Member Author

@cmb69 And the changes you suggested worked :)

@kocsismate
Copy link
Member Author

@cmb69 @nikic Can I ask you to review this? :)

@nikic
Copy link
Member

nikic commented Sep 23, 2020

@cmb69 above says:

The last 3 parameters of odbc_columns() and odbc_tables() should not be nullable and should default to an empty string.

The changes look reasonable to me, but I'm not familiar with the underlying APIs, so can't say whether the removed argument count restrictions make sense.

@cmb69
Copy link
Member

cmb69 commented Sep 23, 2020

I'm trying to figure out how these APIs are supposed to be used. Looks pretty arcane to me.

Anyway, just noticed that odbc_data_source() may also return null, and in that case currently fails (debug builds):

php-src/ext/odbc/php_odbc.c

Lines 1269 to 1271 in 9623756

/* System has no data sources, no error. Signal it by returning NULL,
not false. */
RETURN_NULL();

php-src/ext/odbc/odbc.stub.php

Line 27 in 9623756

function odbc_data_source($connection_id, int $fetch_type): array|false {}

@cmb69
Copy link
Member

cmb69 commented Sep 23, 2020 •
edited
Loading

Not unrelated: https://bugs.php.net/bug.php?id=78470. Will fix ASAP. PR #6200

And sorry for hi-jacking this PR, @kocsismate!

@kocsismate
Copy link
Member Author

The last 3 parameters of odbc_columns() and odbc_tables() should not be nullable and should default to an empty string.

I think this was later clarified by @cmb69 in the P.S. paragraph. Or did I mistunderstand that the null defaults are still correct?

And sorry for hi-jacking this PR

Absolutely no problem :)

cmb69
cmb69 approved these changes Sep 24, 2020
View reviewed changes
Copy link
Member

@cmb69 cmb69 left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! Looks good to me, but see my comments.

Regarding odbc_tables() etc.: the null default values are actually more close to what we had in PHP 7, so these are fine (not sure if there'd be a difference to passing empty strings, but using null might still be useful conceptionally).

Regarding Nikita's comment: that is about odbc_procedures() and odbc_procedurecolumns() which formerly required 1 or 4/5 parameters. It seems to be fine to pass any number of arguments, though; basically, these functions are the pendant of odbc_tables() and odbc_tablecolumns(), respectively, and these functions already supported that.

ext/odbc/php_odbc.c Outdated
@@ -964,7 +964,7 @@ PHP_FUNCTION(odbc_prepare)
PHP_FUNCTION(odbc_execute)
{
zval *pv_res, *tmp;
HashTable *pv_param_ht;
HashTable *pv_param_ht = NULL;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
HashTable *pv_param_ht = NULL;
HashTable *pv_param_ht = (HashTable *) &zend_empty_array;

This way we could drop the check for !pv_param_ht below, and it also looks cleaner wrt. having [] as default.

Copy link
Member Author

@kocsismate kocsismate Sep 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

oh, nice!

ext/odbc/odbc.stub.php
@@ -31,14 +31,14 @@ function odbc_data_source($connection_id, int $fetch_type): array|false {}
* @param resource $connection_id
* @return resource|false
*/
function odbc_exec($connection_id, string $query, int $flags = UNKNOWN) {}
function odbc_exec($connection_id, string $query) {}
Copy link
Member

Choose a reason for hiding this comment

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

I agree that it makes sense to drop this unused parameter, but there should be a note about that in UPGRADING.

ext/odbc/php_odbc.c Outdated
@@ -974,26 +974,20 @@ PHP_FUNCTION(odbc_execute)
unsigned char otype;
SQLSMALLINT ctype;
odbc_result *result;
int numArgs = ZEND_NUM_ARGS(), i, ne;
int i, ne = 0;
Copy link
Member

Choose a reason for hiding this comment

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

The initialization appears to be superfluous, but doesn't hurt.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure why I added it. So I'll get rid of it.

Copy link
Member Author

@kocsismate kocsismate Sep 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Oh, it has just come to my mind: since the !pv_param_ht check below would short-circuit the initialization of ne in the next condition, I had to assign it a default value, because it's used inside the "then" branch.

it's not the case anymore, so I can safely get rid of it for sure.

UPDATE: I've just rebased to master + pushed a new commit

@php-pulls php-pulls closed this in 9b50fd2 Sep 24, 2020
@kocsismate kocsismate deleted the odbc-defaults branch September 24, 2020 20:18
jrfnl added a commit to PHPCompatibility/PHPCompatibility that referenced this pull request Oct 2, 2020
@jrfnl
PHP 8.0 | RemovedFunctionParameters: handle changed odbc_exec() funct…
f0cee38 
…ion signature

> The unused flags parameter of `odbc_exec()` has been removed.

Note: per the commit, this also applies to the `odbc_do()` alias of `odbc_exec()`.

Refs:
* https://github.com/php/php-src/blob/0a84fba0deb1c1b75770a436c4236dc56e6d0463/UPGRADING#L400
* php/php-src#6154
* php/php-src@9b50fd2
@jrfnl jrfnl mentioned this pull request Oct 2, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cmb69 cmb69 cmb69 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kocsismate @nikic @cmb69