Fix UNKNOWN default values in ext/odbc #6154

kocsismate · 2020-09-17T17:37:06Z

No description provided.

nikic · 2020-09-18T08:51:37Z

AppVeyor failures are legitimate.

cmb69 · 2020-09-19T12:39:18Z

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 · 2020-09-21T10:08:55Z

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

kocsismate · 2020-09-21T12:41:11Z

@cmb69 And the changes you suggested worked :)

kocsismate · 2020-09-22T13:55:57Z

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

nikic · 2020-09-23T09:27:12Z

@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 · 2020-09-23T11:32:32Z

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 · 2020-09-23T18:00:59Z

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

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

kocsismate · 2020-09-24T08:16:19Z

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

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.

cmb69 · 2020-09-24T09:44:16Z

ext/odbc/php_odbc.c

 {
 	zval *pv_res, *tmp;
-	HashTable *pv_param_ht;
+	HashTable *pv_param_ht = NULL;


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.

cmb69 · 2020-09-24T09:45:24Z

ext/odbc/odbc.stub.php

 * @return resource|false
 */
-function odbc_exec($connection_id, string $query, int $flags = UNKNOWN) {}
+function odbc_exec($connection_id, string $query) {}


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

cmb69 · 2020-09-24T09:53:40Z

ext/odbc/php_odbc.c

 	SQLSMALLINT ctype;
   	odbc_result *result;
-	int numArgs = ZEND_NUM_ARGS(), i, ne;
+	int i, ne = 0;


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

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

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

…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

kocsismate force-pushed the odbc-defaults branch 2 times, most recently from dcc7ff1 to 348123d Compare September 17, 2020 17:44

kocsismate mentioned this pull request Sep 17, 2020

Improve remaining stubs with = UNKNOWN where possible php/php-tasks#24

Closed

kocsismate force-pushed the odbc-defaults branch 3 times, most recently from 575e372 to 4533257 Compare September 17, 2020 18:02

kocsismate force-pushed the odbc-defaults branch from 4533257 to abc0a50 Compare September 19, 2020 10:12

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

cmb69 approved these changes Sep 24, 2020

View reviewed changes

kocsismate added 3 commits September 24, 2020 18:37

Fix UNKNOWN default values in ext/odbc

16f0674

Fix tests

69d1388

Address review comments

26811b2

kocsismate force-pushed the odbc-defaults branch from 93994ac to 26811b2 Compare September 24, 2020 16:51

php-pulls closed this in 9b50fd2 Sep 24, 2020

kocsismate deleted the odbc-defaults branch September 24, 2020 20:18

jrfnl mentioned this pull request Oct 2, 2020

PHP 8.0 | RemovedFunctionParameters: account for more deprecated/removed function params PHPCompatibility/PHPCompatibility#1230

Merged

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

Fix UNKNOWN default values in ext/odbc #6154

Fix UNKNOWN default values in ext/odbc #6154

Uh oh!

Conversation

kocsismate commented Sep 17, 2020

Uh oh!

nikic commented Sep 18, 2020

Uh oh!

cmb69 commented Sep 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kocsismate commented Sep 21, 2020

Uh oh!

kocsismate commented Sep 21, 2020

Uh oh!

kocsismate commented Sep 22, 2020

Uh oh!

nikic commented Sep 23, 2020

Uh oh!

cmb69 commented Sep 23, 2020

Uh oh!

cmb69 commented Sep 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kocsismate commented Sep 24, 2020

Uh oh!

cmb69 left a comment

Choose a reason for hiding this comment

Uh oh!

cmb69 Sep 24, 2020

Choose a reason for hiding this comment

Uh oh!

kocsismate Sep 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmb69 Sep 24, 2020

Choose a reason for hiding this comment

Uh oh!

cmb69 Sep 24, 2020

Choose a reason for hiding this comment

Uh oh!

kocsismate Sep 24, 2020

Choose a reason for hiding this comment

Uh oh!

kocsismate Sep 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cmb69 commented Sep 19, 2020 •

edited

Loading

cmb69 commented Sep 23, 2020 •

edited

Loading

kocsismate Sep 24, 2020 •

edited

Loading

kocsismate Sep 24, 2020 •

edited

Loading