ext/pgsql: php_pgsql_convert converts E_NOTICE to TypeError exceptions. #11238

devnexen · 2023-05-13T15:32:19Z

No description provided.

Girgias · 2023-05-15T09:05:43Z

ext/pgsql/pgsql.c

-					php_error_docref(NULL, E_NOTICE, "Expects NULL or IPv4 or IPv6 address string for '%s' (%s)", Z_STRVAL_P(type), ZSTR_VAL(field));
+					zend_type_error("Expects IPv4, IPv6 or null for PostgreSQL '%s' (%s)", Z_STRVAL_P(type), ZSTR_VAL(field));


This feels more like a ValueError for the IPv4/6 addresses

Girgias · 2023-05-15T09:06:34Z

ext/pgsql/tests/bug77047.phpt

-    var_dump(array_pop($row));
+	var_dump(array_pop($row));


We use space indents for PHP test files, (but tabs for C source files, confusing yes I know)

Girgias · 2023-05-15T09:09:04Z

ext/pgsql/pgsql.c

-								php_error_docref(NULL, E_NOTICE, "Detected invalid value (%s) for PostgreSQL %s field (%s)", Z_STRVAL_P(val), Z_STRVAL_P(type), ZSTR_VAL(field));
+								zend_value_error("Detected invalid value (%s) for PostgreSQL '%s' field (%s)", Z_STRVAL_P(val), Z_STRVAL_P(type), ZSTR_VAL(field));


This probably could be improved to say that the issue is to convert to a PostgreSQL bool?

Aside, the handling of IS_NULL seems slightly strange....

Girgias · 2023-05-15T09:09:43Z

ext/pgsql/pgsql.c

-					php_error_docref(NULL, E_NOTICE, "Expects string, null, long or boolelan value for PostgreSQL '%s' (%s)", Z_STRVAL_P(type), ZSTR_VAL(field));
+					zend_type_error("Expects string, null long or boolean value for PostgreSQL '%s' (%s)", Z_STRVAL_P(type), ZSTR_VAL(field));


@kocsismate what is the standard phrasing for type errors again? And how do you think we could adjust it for these case?

First of all, the currently executed function name should be at the beginning (e.g. pg_convert():). Then
probably the field type should be mentioned, like Field "%s" must be of type string|long|bool|null, %s given

nielsdos · 2023-05-18T00:10:49Z

ext/pgsql/tests/bug77047.phpt

-pg_insert($db, "bug77047", array("t" => NULL));
-pg_insert($db, "bug77047", array("t" => ""));
+try {
+    pg_insert($db, "bug77047", array("t" => "13:31"));


~~I think you're only testing one case now, because when pg_insert (expectedly) fails, it will execute the catch and not execute the other pg_insert calls.~~
I guess it doesn't really matter here, but maybe you want the test to check if there are no errors thrown that shouldn't be thrown after the first failure. In which case you might not want to put a big try-catch around all calls.

Girgias · 2023-05-25T19:13:37Z

ASAN Failure is expected but somehow it is still marked as FAIL...

Girgias

Nits about using the standard type name boolean to bool and double to float, and needing to split one error case into 2 ideally.

Girgias · 2023-05-29T09:07:19Z

ext/pgsql/pgsql.c

 							}
 							else {
-								php_error_docref(NULL, E_NOTICE, "Detected invalid value (%s) for PostgreSQL %s field (%s)", Z_STRVAL_P(val), Z_STRVAL_P(type), ZSTR_VAL(field));
+								zend_value_error("%s(): Field \"%s\" must be of type boolean, %s (%s) given", get_active_function_name(), ZSTR_VAL(field), Z_STRVAL_P(type), Z_STRVAL_P(val));


Suggested change

zend_value_error("%s(): Field \"%s\" must be of type boolean, %s (%s) given", get_active_function_name(), ZSTR_VAL(field), Z_STRVAL_P(type), Z_STRVAL_P(val));

zend_value_error("%s(): Field \"%s\" must be of type bool, %s (%s) given", get_active_function_name(), ZSTR_VAL(field), Z_STRVAL_P(type), Z_STRVAL_P(val));

We use bool instead of boolean for standard types

Girgias · 2023-05-29T09:07:56Z

ext/pgsql/pgsql.c

 				PGSQL_CONV_CHECK_IGNORE();
 				if (err) {
-					php_error_docref(NULL, E_NOTICE, "Expects NULL, string, long or double value for pgsql '%s' (%s)", Z_STRVAL_P(type), ZSTR_VAL(field));
+					zend_type_error("%s(): Field \"%s\" must be of type string|null|long|double, %s given", get_active_function_name(), ZSTR_VAL(field), Z_STRVAL_P(type));


We use float instead of double as the standard type

Suggested change

zend_type_error("%s(): Field \"%s\" must be of type string|null|long|double, %s given", get_active_function_name(), ZSTR_VAL(field), Z_STRVAL_P(type));

zend_type_error("%s(): Field \"%s\" must be of type string|null|long|float, %s given", get_active_function_name(), ZSTR_VAL(field), Z_STRVAL_P(type));

Girgias · 2023-05-29T09:09:12Z

ext/pgsql/pgsql.c

 				PGSQL_CONV_CHECK_IGNORE();
 				if (err) {
-					php_error_docref(NULL, E_NOTICE, "Expects NULL or IPv4 or IPv6 address string for '%s' (%s)", Z_STRVAL_P(type), ZSTR_VAL(field));
+					zend_value_error("Field \"%s\" must be of type IPv4|IPv6|null, given %s", ZSTR_VAL(field), Z_STRVAL_P(type));


As this is a value error instead of a type error I think may need to be split into two different cases:
One that handles the types and the second one that checks that a string is a valid IP address.

Girgias

Okay sorry for the previous crappy reviews.

I just had a proper look at what this code does, and it performs implicit type juggling to fit the PostgreSQL field types, as such we shouldn't be saying that we accept all those types when realistically we only want to support the target type.

One other thing that probably should be done is to point out when a float is passed for a field that expects an int but is not compatible (i.e. has a fractional part) by emitting a deprecation notice in line with PHP's standard behaviour.

Like I said in a comment, having a test that triggers all those conditions so that one can easily see what the error message being outputted is would be greatly helpful in making sense of what we should be doing.

Girgias · 2023-05-29T20:29:33Z

ext/pgsql/pgsql.c

+					if (err == 2) {
+						zend_value_error("%s(): Field \"%s\" must be of type IPv4|IPv6|null, given %s", get_active_function_name(), ZSTR_VAL(field), Z_STRVAL_P(val));
+					} else {
+						zend_type_error("%s(): Field \"%s\" must be of type IPv4|IPv6|null, given %s", get_active_function_name(), ZSTR_VAL(field), Z_STRVAL_P(type));
+					}


Suggested change

if (err == 2) {

zend_value_error("%s(): Field \"%s\" must be of type IPv4|IPv6|null, given %s", get_active_function_name(), ZSTR_VAL(field), Z_STRVAL_P(val));

} else {

zend_type_error("%s(): Field \"%s\" must be of type IPv4|IPv6|null, given %s", get_active_function_name(), ZSTR_VAL(field), Z_STRVAL_P(type));

}

if (err == 2) {

zend_value_error("%s(): Field \"%s\" must be a valid IPv4 or IPv6 address string, \"%s\" given", get_active_function_name(), ZSTR_VAL(field), Z_STRVAL_P(val));

} else {

zend_type_error("%s(): Field \"%s\" must be of type string|null, given %s", get_active_function_name(), ZSTR_VAL(field), zend_zval_value_name(val));

}

As far as I can see the "type" zval should have nothing to do here. I think ideally it would be good to have a new test that tries to hit everyone of these type errors and value errors as it would be easier to see and correct the error messages.

Girgias · 2023-05-29T20:31:00Z

ext/pgsql/pgsql.c

 				PGSQL_CONV_CHECK_IGNORE();
 				if (err) {
-					php_error_docref(NULL, E_NOTICE, "Expects NULL, string, long or double value for '%s' (%s)", Z_STRVAL_P(type), ZSTR_VAL(field));
+					zend_type_error("%s(): Field \"%s\" must be of type string|null|long|float, %s given", get_active_function_name(), ZSTR_VAL(field), Z_STRVAL_P(type));


Only just saw that "long" is used instead of the standard "int" PHP type.

Also it wants the time as an int so...

Moreover, there is the issue again about not actually providing the type of the actual passed value

Suggested change

zend_type_error("%s(): Field \"%s\" must be of type string|null|long|float, %s given", get_active_function_name(), ZSTR_VAL(field), Z_STRVAL_P(type));

zend_type_error("%s(): Field \"%s\" must be of type int|null, %s given", get_active_function_name(), ZSTR_VAL(field), zend_zval_value_name(val));

Girgias · 2023-05-29T20:36:58Z

ext/pgsql/pgsql.c

 				PGSQL_CONV_CHECK_IGNORE();
 				if (err) {
-					php_error_docref(NULL, E_NOTICE, "Expects NULL, string, long or double value for pgsql '%s' (%s)", Z_STRVAL_P(type), ZSTR_VAL(field));
+					zend_type_error("%s(): Field \"%s\" must be of type string|null|long|float, %s given", get_active_function_name(), ZSTR_VAL(field), Z_STRVAL_P(type));


Actually this field types just wants am int|null and does some type juggling to convert other types to int. however this maybe should follow the normal PHP semantics

Girgias · 2023-05-29T20:39:52Z

ext/pgsql/pgsql.c

 							}
 							else {
-								php_error_docref(NULL, E_NOTICE, "Detected invalid value (%s) for PostgreSQL %s field (%s)", Z_STRVAL_P(val), Z_STRVAL_P(type), ZSTR_VAL(field));
+								zend_value_error("%s(): Field \"%s\" must be of type bool, %s (%s) given", get_active_function_name(), ZSTR_VAL(field), Z_STRVAL_P(type), Z_STRVAL_P(val));


Suggested change

zend_value_error("%s(): Field \"%s\" must be of type bool, %s (%s) given", get_active_function_name(), ZSTR_VAL(field), Z_STRVAL_P(type), Z_STRVAL_P(val));

zend_value_error("%s(): Field \"%s\" must be of type bool, invalid PostgreSQL string boolean value \"%s\" given", get_active_function_name(), ZSTR_VAL(field), Z_STRVAL_P(val));

Erf I don't know what to do with this error message as what it wants is a valid PostgreSQL bool and we are parsing the string here...

Girgias · 2023-05-29T20:40:44Z

ext/pgsql/pgsql.c

 				PGSQL_CONV_CHECK_IGNORE();
 				if (err) {
-					php_error_docref(NULL, E_NOTICE, "Expects NULL, string, long or double value for PostgreSQL '%s' (%s)", Z_STRVAL_P(type), ZSTR_VAL(field));
+					zend_type_error("%s(): Field \"%s\" must be of type string|null|long|float, %s given", get_active_function_name(), ZSTR_VAL(field), Z_STRVAL_P(type));


Same here, it seems it only wants a float. But then this might require us to split the logic for NUMERIC and MONEY fields

Girgias · 2023-05-29T20:41:45Z

ext/pgsql/pgsql.c

 				PGSQL_CONV_CHECK_IGNORE();
 				if (err) {
-					php_error_docref(NULL, E_NOTICE, "Expects NULL, string, long or double value for PostgreSQL '%s' (%s)", Z_STRVAL_P(type), ZSTR_VAL(field));
+					zend_type_error("%s(): Field \"%s\" must be of type string|null|long|float, %s given", get_active_function_name(), ZSTR_VAL(field), Z_STRVAL_P(type));


Suggested change

zend_type_error("%s(): Field \"%s\" must be of type string|null|long|float, %s given", get_active_function_name(), ZSTR_VAL(field), Z_STRVAL_P(type));

zend_type_error("%s(): Field \"%s\" must be of type string|null, %s given", get_active_function_name(), ZSTR_VAL(field), Z_STRVAL_P(type));

Similarly here, it does implicit type juggling to end up with a string.

Girgias

Final bits of review comments and then I think this is goot to land. Thank you!

Girgias · 2023-06-05T13:03:01Z

ext/pgsql/pgsql.c

 				PGSQL_CONV_CHECK_IGNORE();
 				if (err) {
-					php_error_docref(NULL, E_NOTICE, "Expects string, null, long or boolelan value for PostgreSQL '%s' (%s)", Z_STRVAL_P(type), ZSTR_VAL(field));
+					zend_type_error("%s(): Field \"%s\" must be of type string|null|long|bool, %s given", get_active_function_name(), ZSTR_VAL(field), Z_STRVAL_P(type));


Suggested change

zend_type_error("%s(): Field \"%s\" must be of type string|null|long|bool, %s given", get_active_function_name(), ZSTR_VAL(field), Z_STRVAL_P(type));

zend_type_error("%s(): Field \"%s\" must be of type string|null|int|bool, %s given", get_active_function_name(), ZSTR_VAL(field), Z_STRVAL_P(type));

Girgias · 2023-06-05T13:05:09Z

ext/pgsql/pgsql.c

 				PGSQL_CONV_CHECK_IGNORE();
 				if (err) {
-					php_error_docref(NULL, E_NOTICE, "Expects NULL, string, long or double value for PostgreSQL '%s' (%s)", Z_STRVAL_P(type), ZSTR_VAL(field));
+					zend_type_error("%s(): Field \"%s\" must be of type %s|null, %s given", get_active_function_name(), (data_type == PG_MONEY ? "money" : "float"), ZSTR_VAL(field), Z_STRVAL_P(type));


As int seems to be a reasonable, valid one too. Not sure if we should use money as a type but at this point it's better than what we have I think.

Suggested change

zend_type_error("%s(): Field \"%s\" must be of type %s|null, %s given", get_active_function_name(), (data_type == PG_MONEY ? "money" : "float"), ZSTR_VAL(field), Z_STRVAL_P(type));

zend_type_error("%s(): Field \"%s\" must be of type %s|int|null, %s given", get_active_function_name(), (data_type == PG_MONEY ? "money" : "float"), ZSTR_VAL(field), Z_STRVAL_P(type));

Girgias · 2023-06-05T13:06:06Z

ext/pgsql/pgsql.c

 				PGSQL_CONV_CHECK_IGNORE();
 				if (err) {
-					php_error_docref(NULL, E_NOTICE, "Expects NULL, string, long or double value for PostgreSQL '%s' (%s)", Z_STRVAL_P(type), ZSTR_VAL(field));
+					zend_type_error("%s(): Field \"%s\" must be of type string|null|long|float, %s given", get_active_function_name(), ZSTR_VAL(field), Z_STRVAL_P(type));


Suggested change

zend_type_error("%s(): Field \"%s\" must be of type string|null|long|float, %s given", get_active_function_name(), ZSTR_VAL(field), Z_STRVAL_P(type));

zend_type_error("%s(): Field \"%s\" must be of type string|null, %s given", get_active_function_name(), ZSTR_VAL(field), Z_STRVAL_P(type));

Girgias · 2023-06-05T13:06:42Z

ext/pgsql/tests/bug71998.phpt

+    try {
+    	@pg_insert($db, 'tmp_statistics', $data);
+    } catch (\ValueError $e) {
+	echo $e->getMessage() . PHP_EOL;


Nit: indent

github-actions bot added the Extension: pgsql label May 13, 2023

devnexen requested a review from Girgias May 13, 2023 16:14

Girgias reviewed May 15, 2023

View reviewed changes

nielsdos reviewed May 18, 2023

View reviewed changes

devnexen force-pushed the pgsql_convert_update branch from 1d78c4c to 7c9c2e4 Compare May 18, 2023 11:56

ext/pgsql: php_pgsql_convert converts E_NOTICE to TypeError exceptions.

b722b99

devnexen force-pushed the pgsql_convert_update branch from 7c9c2e4 to 596ff5b Compare May 26, 2023 20:23

Girgias requested changes May 29, 2023

View reviewed changes

changes from reviews

3e816a8

devnexen force-pushed the pgsql_convert_update branch from 596ff5b to 3e816a8 Compare May 29, 2023 12:13

Girgias requested changes May 29, 2023

View reviewed changes

Girgias approved these changes Jun 5, 2023

View reviewed changes

update error messages

6e04505

devnexen force-pushed the pgsql_convert_update branch from 392c1f0 to 6e04505 Compare June 5, 2023 13:44

Girgias approved these changes Jun 5, 2023

View reviewed changes

devnexen closed this in 16a63d7 Jun 5, 2023

		php_error_docref(NULL, E_NOTICE, "Expects NULL or IPv4 or IPv6 address string for '%s' (%s)", Z_STRVAL_P(type), ZSTR_VAL(field));
		zend_type_error("Expects IPv4, IPv6 or null for PostgreSQL '%s' (%s)", Z_STRVAL_P(type), ZSTR_VAL(field));

		php_error_docref(NULL, E_NOTICE, "Detected invalid value (%s) for PostgreSQL %s field (%s)", Z_STRVAL_P(val), Z_STRVAL_P(type), ZSTR_VAL(field));
		zend_value_error("Detected invalid value (%s) for PostgreSQL '%s' field (%s)", Z_STRVAL_P(val), Z_STRVAL_P(type), ZSTR_VAL(field));

		php_error_docref(NULL, E_NOTICE, "Expects string, null, long or boolelan value for PostgreSQL '%s' (%s)", Z_STRVAL_P(type), ZSTR_VAL(field));
		zend_type_error("Expects string, null long or boolean value for PostgreSQL '%s' (%s)", Z_STRVAL_P(type), ZSTR_VAL(field));

	zend_value_error("%s(): Field \"%s\" must be of type boolean, %s (%s) given", get_active_function_name(), ZSTR_VAL(field), Z_STRVAL_P(type), Z_STRVAL_P(val));
	zend_value_error("%s(): Field \"%s\" must be of type bool, %s (%s) given", get_active_function_name(), ZSTR_VAL(field), Z_STRVAL_P(type), Z_STRVAL_P(val));

	zend_type_error("%s(): Field \"%s\" must be of type string\|null\|long\|double, %s given", get_active_function_name(), ZSTR_VAL(field), Z_STRVAL_P(type));
	zend_type_error("%s(): Field \"%s\" must be of type string\|null\|long\|float, %s given", get_active_function_name(), ZSTR_VAL(field), Z_STRVAL_P(type));

Uh oh!

ext/pgsql: php_pgsql_convert converts E_NOTICE to TypeError exceptions. #11238

ext/pgsql: php_pgsql_convert converts E_NOTICE to TypeError exceptions. #11238

Uh oh!

Conversation

devnexen commented May 13, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nielsdos May 18, 2023 • 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!

Girgias commented May 25, 2023

Uh oh!

Girgias left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Girgias left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Girgias left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nielsdos May 18, 2023 •

edited

Loading