Promote E_NOTICE to Error in PostgreSQL extension #6226
Conversation
We should add some (representative) tests then ^ |
Girgias
force-pushed
the
pgsql-notice-to-error
branch
from
f87c9a5 to
46c239e
Compare
ext/pgsql/pgsql.c
Outdated
| php_error_docref(NULL, E_NOTICE, "Invalid OID value passed"); | ||
| RETURN_FALSE; | ||
| zend_value_error("Invalid OID value passed"); | ||
| RETURN_THROWS(); |
There was a problem hiding this comment.
While you're here, indent this block one level?
ext/pgsql/pgsql.c
Outdated
| @@ -2322,25 +2323,26 @@ PHP_FUNCTION(pg_lo_create) | |||
| switch (Z_TYPE_P(oid)) { | |||
| case IS_STRING: | |||
| { | |||
| /* TODO: Use zend_is_numeric_string/subroutine? */ | |||
There was a problem hiding this comment.
No, this code is there to handle integer strings larger than zend_long. is_numeric_string would convert those to floats, which defeats the purpose of this code.
There was a problem hiding this comment.
Right, but this code is lot, so maybe make it a dedicated function in the future?
There was a problem hiding this comment.
Oh sure, we should extract the common pgsql code (later...)
ext/pgsql/pgsql.c
Outdated
| @@ -2525,6 +2531,7 @@ PHP_FUNCTION(pg_lo_open) | |||
| if ((pgsql_lofd = lo_open(pgsql, oid, pgsql_mode)) == -1) { | |||
| if (lo_unlink(pgsql, oid) == -1) { | |||
| efree(pgsql_lofp); | |||
| /* TODO: Throw error? */ | |||
There was a problem hiding this comment.
Nah, at least going by the message, this is an issue with the environment...
ext/pgsql/pgsql.c
Outdated
| php_error_docref(NULL, E_NOTICE, "Invalid OID value passed"); | ||
| RETURN_FALSE; | ||
| zend_value_error("Invalid OID value passed"); | ||
| RETURN_THROWS(); |
There was a problem hiding this comment.
Same here, this block is missing an indent.
ext/pgsql/pgsql.c
Outdated
| @@ -5532,7 +5546,7 @@ static inline int build_assignment_string(PGconn *pg_link, smart_str *querystr, | |||
|
|
|||
| ZEND_HASH_FOREACH_STR_KEY_VAL(ht, fld, val) { | |||
| if (fld == NULL) { | |||
| php_error_docref(NULL, E_NOTICE, "Expects associative array for values to be inserted"); | |||
| zend_value_error("Array of values must be an associated array of string keys"); | |||
There was a problem hiding this comment.
| zend_value_error("Array of values must be an associated array of string keys"); | |
| zend_value_error("Array of values must be an associative array with string keys"); |
ext/pgsql/pgsql.c
Outdated
| @@ -5358,7 +5373,7 @@ PHP_PGSQL_API int php_pgsql_insert(PGconn *pg_link, const char *table, zval *var | |||
|
|
|||
| ZEND_HASH_FOREACH_STR_KEY(Z_ARRVAL_P(var_array), fld) { | |||
| if (fld == NULL) { | |||
| php_error_docref(NULL, E_NOTICE, "Expects associative array for values to be inserted"); | |||
| zend_value_error("Array of values must be an associative array of string keys"); | |||
There was a problem hiding this comment.
| zend_value_error("Array of values must be an associative array of string keys"); | |
| zend_value_error("Array of values must be an associative array with string keys"); |
Girgias
force-pushed
the
pgsql-notice-to-error
branch
from
46c239e to
e327070
Compare
Some of these are obvious type/value errors which is surprising they were classified as E_NOTICE...
And because it is PGSQL none of these conditions are tested :-)