PDO Unconditional errors #6212

Girgias · 2020-09-25T04:02:53Z

A couple of unrelated refactoring changes that I made should be able to land on their own if it makes the PR too noisy.

Used only PDO MySQL for tests so hopefully not too many more drivers complain.

nikic

Can you please apply the bug fix separately (together with corresponding test changes)?

ext/pdo/pdo_stmt.c

nikic · 2020-09-25T08:24:34Z

And please also apply at least "Inverse logic in do_fetch() to reduce a level of indentation" separately, as the indentation changes makes it hard to understand the diff.

ext/pdo/pdo_dbh.stub.php

nikic

First pass ... this is huge

ext/pdo/pdo_dbh.c

ext/pdo/pdo_stmt.c

Girgias · 2020-09-28T10:37:34Z

Pushing review changed before the ZPP variadic revert which I'm doing now

pdo_stmt_verify_mode() never returns FAILURE (aka -1) and thus this path was never hit

Promote a couple of confirmed cases to unconditional error Fix error messages

… the ODBC driver And ammend PDO::quote() test to take into account drivers which do not support the feature

nikic

This looks pretty good

ext/pdo/pdo_dbh.c

ext/pdo/pdo_stmt.c

nikic · 2020-09-28T13:11:57Z

ext/pdo/pdo_stmt.c

@@ -1963,6 +2009,8 @@ PHP_METHOD(PDOStatement, debugDumpParams)
 	ZEND_PARSE_PARAMETERS_NONE();

 	PHP_STMT_GET_OBJ;
+
+	/* TODO: Change to assertion? */


I'm not confident it can't fail, e.g. >&- is used.

ext/pdo_mysql/tests/pdo_mysql_stmt_nextrowset.phpt

ext/pdo_sqlite/tests/bug_44159_sqlite_version.phpt

nikic · 2020-09-28T13:20:00Z

ext/pdo/pdo_stmt.c

@@ -62,6 +63,7 @@ static inline int rewrite_name_to_position(pdo_stmt_t *stmt, struct pdo_bound_pa
 				param->name = zend_string_init(name, strlen(name), 0);
 				return 1;
 			}
+			/* TODO Error? */
 			pdo_raise_impl_error(stmt->dbh, stmt, "HY093", "parameter was not defined");


So this happens if you use an out of bounds parameter number? I think generally it would make sense for things like this to throw, but we need to be careful that native & emulated prepared statements have the same behavior. I don't know whether both go through this codepath.

I think those trigger if the param name/number is not present. I would imagine they both go through this code path as this is something internal to PDO which is not exposed, as all PDO bind param/value methods go through this. Or do emulated prepared statements somehow override the PDO methods?

Indentation Error message Use variadic number position for pdo_stmt_setup_fetch_mode() param cehcks Use values from *args directly instead of preassigning them in pdo_stmt_setup_fetch_mode() Drop todo comment

ext/pdo_sqlite/tests/bug_44159_sqlite_version.phpt

Indent (again) Drop unnecessary cehck Improve title + SKIPIF for PDO SQLite test

nikic · 2020-09-28T14:45:30Z

ext/pdo/pdo_dbh.c

-				);
-				PDO_HANDLE_DBH_ERR();
-				RETURN_FALSE;
+				zend_type_error("PDO::ATTR_STATEMENT_CLASS ctor_args must be ?array, %s given",


Suggested change

zend_type_error("PDO::ATTR_STATEMENT_CLASS ctor_args must be ?array, %s given",

zend_type_error("PDO::ATTR_STATEMENT_CLASS ctor_args must be of type ?array, %s given",

nikic · 2020-09-28T14:46:35Z

ext/pdo/pdo_dbh.c

-						"ctor_args must be an array"
-					);
-					PDO_HANDLE_DBH_ERR();
+					zend_type_error("PDO::ATTR_STATEMENT_CLASS ctor_args must be ?array, %s given",


Suggested change

zend_type_error("PDO::ATTR_STATEMENT_CLASS ctor_args must be ?array, %s given",

zend_type_error("PDO::ATTR_STATEMENT_CLASS ctor_args must be of type ?array, %s given",

nikic · 2020-09-28T14:49:30Z

ext/pdo/pdo_stmt.c

+			/* Figure out correct class */
+			if (arg2) {
+				if (Z_TYPE_P(arg2) != IS_STRING) {
+					zend_argument_type_error(2, "must be string, %s given", zend_zval_type_name(arg2));


Suggested change

zend_argument_type_error(2, "must be string, %s given", zend_zval_type_name(arg2));

zend_argument_type_error(2, "must be of type string, %s given", zend_zval_type_name(arg2));

nikic · 2020-09-28T14:49:57Z

ext/pdo/pdo_stmt.c

+			if (arg2) {
+				// Reuse convert_to_long(arg2); ?
+				if (Z_TYPE_P(arg2) != IS_LONG) {
+					zend_argument_type_error(2, "must be int, %s given", zend_zval_type_name(arg2));


Suggested change

zend_argument_type_error(2, "must be int, %s given", zend_zval_type_name(arg2));

zend_argument_type_error(2, "must be of type int, %s given", zend_zval_type_name(arg2));

nikic · 2020-09-28T14:50:31Z

ext/pdo/pdo_stmt.c

+				return false;
+			}
+			if (Z_TYPE(args[0]) != IS_LONG) {
+				zend_argument_type_error(arg1_arg_num, "must be int, %s given", zend_zval_type_name(&args[0]));


Suggested change

zend_argument_type_error(arg1_arg_num, "must be int, %s given", zend_zval_type_name(&args[0]));

zend_argument_type_error(arg1_arg_num, "must be of type int, %s given", zend_zval_type_name(&args[0]));

nikic · 2020-09-28T14:50:56Z

ext/pdo/pdo_stmt.c

+					return false;
+				}
+				if (Z_TYPE(args[0]) != IS_STRING) {
+					zend_argument_type_error(arg1_arg_num, "must be string, %s given", zend_zval_type_name(&args[0]));


Suggested change

zend_argument_type_error(arg1_arg_num, "must be string, %s given", zend_zval_type_name(&args[0]));

zend_argument_type_error(arg1_arg_num, "must be of type string, %s given", zend_zval_type_name(&args[0]));

nikic · 2020-09-28T14:51:10Z

ext/pdo/pdo_stmt.c

-						retval = FAILURE;
-					} else if (Z_TYPE(args[1]) == IS_ARRAY && zend_hash_num_elements(Z_ARRVAL(args[1]))) {
-						ZVAL_ARR(&stmt->fetch.cls.ctor_args, zend_array_dup(Z_ARRVAL(args[1])));
+						zend_argument_type_error(constructor_arg_num, "must be ?array, %s given",


Suggested change

zend_argument_type_error(constructor_arg_num, "must be ?array, %s given",

zend_argument_type_error(constructor_arg_num, "must be of type ?array, %s given",

nikic · 2020-09-28T14:51:21Z

ext/pdo/pdo_stmt.c

-			if (SUCCESS == retval) {
-				ZVAL_COPY(&stmt->fetch.into, &args[0]);
+			if (Z_TYPE(args[0]) != IS_OBJECT) {
+				zend_argument_type_error(arg1_arg_num, "must be object, %s given", zend_zval_type_name(&args[0]));


Suggested change

zend_argument_type_error(arg1_arg_num, "must be object, %s given", zend_zval_type_name(&args[0]));

zend_argument_type_error(arg1_arg_num, "must be object, %s given", zend_zval_type_name(&args[0]));

Suggested change

zend_argument_type_error(arg1_arg_num, "must be object, %s given", zend_zval_type_name(&args[0]));

zend_argument_type_error(arg1_arg_num, "must be of type object, %s given", zend_zval_type_name(&args[0]));

nikic · 2020-09-28T14:51:41Z

ext/pdo/pdo_stmt.c

@@ -1857,18 +1891,27 @@ PHP_METHOD(PDOStatement, setFetchMode)
 	zval *args = NULL;
 	uint32_t num_args = 0;

+	/* Support null for constructor arguments for BC */


Comment now a bit out of place

nikic · 2020-09-28T14:52:25Z

ext/pdo/pdo_dbh.c

 #define PDO_LONG_PARAM_CHECK \
 	if (Z_TYPE_P(value) != IS_LONG && Z_TYPE_P(value) != IS_STRING && Z_TYPE_P(value) != IS_FALSE && Z_TYPE_P(value) != IS_TRUE) { \
-		pdo_raise_impl_error(dbh, NULL, "HY000", "attribute value must be an integer"); \
-		PDO_HANDLE_DBH_ERR(); \
+		zend_type_error("Attribute value must be int for selected attribute, %s given", zend_zval_type_name(value)); \


Suggested change

zend_type_error("Attribute value must be int for selected attribute, %s given", zend_zval_type_name(value)); \

zend_type_error("Attribute value must be of type int for selected attribute, %s given", zend_zval_type_name(value)); \

nikic

LG assuming CI happy.

Girgias requested a review from kocsismate September 25, 2020 04:03

kocsismate mentioned this pull request Sep 25, 2020

Warning to Error (TypeError/ValueError) in extensions php/php-tasks#18

Closed

71 tasks

nikic reviewed Sep 25, 2020

View reviewed changes

ext/pdo/pdo_stmt.c Outdated Show resolved Hide resolved

Girgias force-pushed the pdo-unconditional-error branch from 19bc7ab to 1f276da Compare September 25, 2020 13:48

Girgias mentioned this pull request Sep 25, 2020

Fix logic error in PDO #6215

Closed

Girgias force-pushed the pdo-unconditional-error branch 5 times, most recently from ebdc73b to 9d79ad6 Compare September 26, 2020 17:50

nikic reviewed Sep 27, 2020

View reviewed changes

ext/pdo/pdo_dbh.stub.php Outdated Show resolved Hide resolved

Girgias force-pushed the pdo-unconditional-error branch from b03e59e to 32b0b47 Compare September 27, 2020 22:37

nikic reviewed Sep 28, 2020

View reviewed changes

Girgias force-pushed the pdo-unconditional-error branch from 5e67d6d to 0ae6ecb Compare September 28, 2020 12:05

Girgias added 14 commits September 28, 2020 14:10

Fix a logical bug in PDO

2435c87

pdo_stmt_verify_mode() never returns FAILURE (aka -1) and thus this path was never hit

Column index must be >= 0 (just 1 case) and todo comment.

e280e5e

Refactor PDO setFetchMode

96770fa

Drop ZPP check in PDO Mysqli test...

c5a4c3e

Make certain error conditions always error in PDO::fetchAll()

4c75cae

Unconditonal error for values less than a minimum

2cf849b

Add todo comments

220d89f

ValueError for negative column index

b2bd8b6

Assertion + always throw on invalid Fetch mode

156ec13

ValueError for incorrect Fetch Flag usage

7bcf852

Make invalid user callable throw a type error

d92274a

More TODOs

4028d9f

Todo everything...

1615a93

ValueError for empty query in PDO::exec()

2f9bb0e

Girgias added 16 commits September 28, 2020 14:10

Add a TODO comment

7716b91

Redfine todo comment

8e9fa15

Introduce ValueError for empty bind param names

1725e31

Fix stubs after making uninitialized object return Error

79e9444

Introduce ValueError for empty string arguments

64349d3

Revert ValueError for empty username

a52ce29

Make fetch argument variadic

d94521b

Fix test after making fetch args variadic

ee37a1d

Review for pdo_dbh.c

89b233c

Promote a couple of confirmed cases to unconditional error Fix error messages

Drop todo comments in pdo_sql_parser.re

978b43d

Revert + add test for empty string in PDO::qutoe()

909651e

Drop ValueError for empty lastInsertId()

86959ee

Accept strings values again for integer attribute values

8944242

Review for pdo_stmt.c

a0eedd8

Promote a couple of confirmed cases to unconditional error Fix error messages

Revert ZPP to use variadics

2fd259f

Do not advertise support for a PDO feature if it's not implemented by…

c0d2d8b

… the ODBC driver And ammend PDO::quote() test to take into account drivers which do not support the feature

nikic reviewed Sep 28, 2020

View reviewed changes

ext/pdo_mysql/tests/pdo_mysql_stmt_nextrowset.phpt Show resolved Hide resolved

nikic reviewed Sep 28, 2020

View reviewed changes

ext/pdo_sqlite/tests/bug_44159_sqlite_version.phpt Outdated Show resolved Hide resolved

nikic reviewed Sep 28, 2020

View reviewed changes

Another round of review

e4a13d0

Indentation Error message Use variadic number position for pdo_stmt_setup_fetch_mode() param cehcks Use values from *args directly instead of preassigning them in pdo_stmt_setup_fetch_mode() Drop todo comment

Girgias force-pushed the pdo-unconditional-error branch from 0ae6ecb to e4a13d0 Compare September 28, 2020 13:43

nikic reviewed Sep 28, 2020

View reviewed changes

ext/pdo_sqlite/tests/bug_44159_sqlite_version.phpt Outdated Show resolved Hide resolved

Minor review

080465f

Indent (again) Drop unnecessary cehck Improve title + SKIPIF for PDO SQLite test

nikic reviewed Sep 28, 2020

View reviewed changes

Fix type error messages

c949e24

nikic approved these changes Sep 28, 2020

View reviewed changes

php-pulls closed this in a5cf828 Sep 28, 2020

Girgias deleted the pdo-unconditional-error branch September 28, 2020 17:52

Girgias mentioned this pull request Sep 28, 2020

Another pass making some failure states unconditional erros in PDO #6227

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PDO Unconditional errors #6212

PDO Unconditional errors #6212

Girgias commented Sep 25, 2020

nikic left a comment

nikic commented Sep 25, 2020

nikic left a comment

Girgias commented Sep 28, 2020

nikic left a comment

nikic Sep 28, 2020

nikic Sep 28, 2020

Girgias Sep 28, 2020

nikic Sep 28, 2020

nikic Sep 28, 2020

nikic Sep 28, 2020

nikic Sep 28, 2020

nikic Sep 28, 2020

nikic Sep 28, 2020

nikic Sep 28, 2020

nikic Sep 28, 2020

nikic Sep 28, 2020

nikic Sep 28, 2020

nikic left a comment

	zend_type_error("PDO::ATTR_STATEMENT_CLASS ctor_args must be ?array, %s given",
	zend_type_error("PDO::ATTR_STATEMENT_CLASS ctor_args must be of type ?array, %s given",

	zend_argument_type_error(2, "must be string, %s given", zend_zval_type_name(arg2));
	zend_argument_type_error(2, "must be of type string, %s given", zend_zval_type_name(arg2));

	zend_argument_type_error(2, "must be int, %s given", zend_zval_type_name(arg2));
	zend_argument_type_error(2, "must be of type int, %s given", zend_zval_type_name(arg2));

	zend_argument_type_error(arg1_arg_num, "must be int, %s given", zend_zval_type_name(&args[0]));
	zend_argument_type_error(arg1_arg_num, "must be of type int, %s given", zend_zval_type_name(&args[0]));

	zend_argument_type_error(constructor_arg_num, "must be ?array, %s given",
	zend_argument_type_error(constructor_arg_num, "must be of type ?array, %s given",

	zend_type_error("Attribute value must be int for selected attribute, %s given", zend_zval_type_name(value)); \
	zend_type_error("Attribute value must be of type int for selected attribute, %s given", zend_zval_type_name(value)); \

PDO Unconditional errors #6212

PDO Unconditional errors #6212

Conversation

Girgias commented Sep 25, 2020

nikic left a comment

Choose a reason for hiding this comment

nikic commented Sep 25, 2020

nikic left a comment

Choose a reason for hiding this comment

Girgias commented Sep 28, 2020

nikic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikic left a comment

Choose a reason for hiding this comment