Skip to content

ext/pdo: Refactor PDOStatement::fetchAll() #17347

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

Merged
merged 3 commits into from
Jan 7, 2025
Merged

ext/pdo: Refactor PDOStatement::fetchAll() #17347

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fde518b
ext/pdo: Move common PDO bug test and enhance it
Girgias Jan 3, 2025
d38ba8b
ext/pdo: Refactor PDOStatement::fetchAll()
Girgias Jan 3, 2025
b70b899
[skip ci] Remove useless comment
Girgias Jan 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
262 changes: 134 additions & 128 deletions ext/pdo/pdo_stmt.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -708,14 +708,38 @@ static void do_fetch_opt_finish(pdo_stmt_t *stmt, int free_ctor_agrs) /* {{{ */
}
/* }}} */

static bool pdo_do_key_pair_fetch(pdo_stmt_t *stmt, enum pdo_fetch_orientation ori, zend_long offset, HashTable *container)
{
if (!do_fetch_common(stmt, ori, offset)) {
return false;
}
if (stmt->column_count != 2) {
/* TODO: Error? */
pdo_raise_impl_error(stmt->dbh, stmt, "HY000", "PDO::FETCH_KEY_PAIR fetch mode requires the result set to contain exactly 2 columns.");
return false;
}

zval key, val;
fetch_value(stmt, &key, 0, NULL);
fetch_value(stmt, &val, 1, NULL);

if (Z_TYPE(key) == IS_LONG) {
zend_hash_index_update(container, Z_LVAL(key), &val);
} else {
convert_to_string(&key);
zend_symtable_update(container, Z_STR(key), &val);
}
zval_ptr_dtor(&key);
return true;
}

/* perform a fetch.
* Stores values into return_value according to HOW. */
static bool do_fetch(pdo_stmt_t *stmt, zval *return_value, enum pdo_fetch_type how, enum pdo_fetch_orientation ori, zend_long offset, zval *return_all) /* {{{ */
static bool do_fetch(pdo_stmt_t *stmt, zval *return_value, enum pdo_fetch_type how, enum pdo_fetch_orientation ori, zend_long offset, zval *group_key) /* {{{ */
{
int flags, idx, old_arg_count = 0;
zend_class_entry *ce = NULL, *old_ce = NULL;
zval grp_val, *pgrp, retval, old_ctor_args = {{0}, {0}, {0}};
int colno;
zval retval, old_ctor_args = {{0}, {0}, {0}};
int i = 0;

if (how == PDO_FETCH_USE_DEFAULT) {
Expand All @@ -725,23 +749,54 @@ static bool do_fetch(pdo_stmt_t *stmt, zval *return_value, enum pdo_fetch_type h
how = how & ~PDO_FETCH_FLAGS;

if (!do_fetch_common(stmt, ori, offset)) {
return 0;
return false;
}

if (how == PDO_FETCH_BOUND) {
RETVAL_TRUE;
return 1;
}

if ((flags & PDO_FETCH_GROUP) && stmt->fetch.column == -1) {
colno = 1;
} else {
colno = stmt->fetch.column;
return true;
}

if (how == PDO_FETCH_LAZY) {
get_lazy_object(stmt, return_value);
return 1;
return true;
}

/* When fetching a column we only do one value fetch, so handle it separately */
Copy link
Member

Choose a reason for hiding this comment

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

This could still be in the switch, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because of the weird special case with PDO_FETCH_GROUP (which probably should also take into account FETCH_UNIQUE now that I think about it) I'd rather leave this outside the switch statement.

A follow-up I think to do is instead of the code setting stmt->fetch.column == -1 prior to calling do_fetch() it should set stmt->fetch.column == 0 and let the new grouping code handle it. Probably requires having a "common" colno variable however.

if (how == PDO_FETCH_COLUMN) {
int colno = stmt->fetch.column;

if ((flags & PDO_FETCH_GROUP) && stmt->fetch.column == -1) {
colno = 1;
}

if (colno < 0 ) {
zend_value_error("Column index must be greater than or equal to 0");
return false;
}

if (colno >= stmt->column_count) {
zend_value_error("Invalid column index");
return false;
}

if (flags == PDO_FETCH_GROUP && stmt->fetch.column == -1) {
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this the same as what you have on line 769?

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried to group this but I was getting failures I didn't really get. I would like to keep this for a follow-up.

fetch_value(stmt, return_value, 1, NULL);
} else if (flags == PDO_FETCH_GROUP && colno) {
fetch_value(stmt, return_value, 0, NULL);
} else {
fetch_value(stmt, return_value, colno, NULL);
}

if (group_key) {
if (flags == PDO_FETCH_GROUP && stmt->fetch.column > 0) {
fetch_value(stmt, group_key, colno, NULL);
} else {
fetch_value(stmt, group_key, 0, NULL);
}
convert_to_string(group_key);
}
return true;
}

RETVAL_FALSE;
Expand All @@ -752,47 +807,13 @@ static bool do_fetch(pdo_stmt_t *stmt, zval *return_value, enum pdo_fetch_type h
case PDO_FETCH_BOTH:
case PDO_FETCH_NUM:
case PDO_FETCH_NAMED:
if (!return_all) {
if (!group_key) {
array_init_size(return_value, stmt->column_count);
} else {
array_init(return_value);
}
break;

case PDO_FETCH_KEY_PAIR:
if (stmt->column_count != 2) {
/* TODO: Error? */
pdo_raise_impl_error(stmt->dbh, stmt, "HY000", "PDO::FETCH_KEY_PAIR fetch mode requires the result set to contain exactly 2 columns.");
return 0;
}
if (!return_all) {
array_init(return_value);
}
break;

case PDO_FETCH_COLUMN:
if (colno < 0 ) {
zend_value_error("Column index must be greater than or equal to 0");
return false;
}

if (colno >= stmt->column_count) {
zend_value_error("Invalid column index");
return false;
}

if (flags == PDO_FETCH_GROUP && stmt->fetch.column == -1) {
fetch_value(stmt, return_value, 1, NULL);
} else if (flags == PDO_FETCH_GROUP && colno) {
fetch_value(stmt, return_value, 0, NULL);
} else {
fetch_value(stmt, return_value, colno, NULL);
}
if (!return_all) {
return 1;
}
break;

case PDO_FETCH_OBJ:
object_init_ex(return_value, ZEND_STANDARD_CLASS_DEF_PTR);
break;
Expand Down Expand Up @@ -889,18 +910,10 @@ static bool do_fetch(pdo_stmt_t *stmt, zval *return_value, enum pdo_fetch_type h
EMPTY_SWITCH_DEFAULT_CASE();
}

if (return_all && how != PDO_FETCH_KEY_PAIR) {
if (flags == PDO_FETCH_GROUP && how == PDO_FETCH_COLUMN && stmt->fetch.column > 0) {
fetch_value(stmt, &grp_val, colno, NULL);
} else {
fetch_value(stmt, &grp_val, i, NULL);
}
convert_to_string(&grp_val);
if (how == PDO_FETCH_COLUMN) {
i = stmt->column_count; /* no more data to fetch */
} else {
i++;
}
if (group_key) {
fetch_value(stmt, group_key, i, NULL);
convert_to_string(group_key);
i++;
}

for (idx = 0; i < stmt->column_count; i++, idx++) {
Expand All @@ -912,22 +925,6 @@ static bool do_fetch(pdo_stmt_t *stmt, zval *return_value, enum pdo_fetch_type h
zend_symtable_update(Z_ARRVAL_P(return_value), stmt->columns[i].name, &val);
break;

case PDO_FETCH_KEY_PAIR:
{
zval tmp;
fetch_value(stmt, &tmp, ++i, NULL);

if (Z_TYPE(val) == IS_LONG) {
zend_hash_index_update((return_all ? Z_ARRVAL_P(return_all) : Z_ARRVAL_P(return_value)), Z_LVAL(val), &tmp);
} else {
convert_to_string(&val);
zend_symtable_update((return_all ? Z_ARRVAL_P(return_all) : Z_ARRVAL_P(return_value)), Z_STR(val), &tmp);
}
zval_ptr_dtor(&val);
return 1;
}
break;

case PDO_FETCH_USE_DEFAULT:
case PDO_FETCH_BOTH:
zend_symtable_update(Z_ARRVAL_P(return_value), stmt->columns[i].name, &val);
Expand Down Expand Up @@ -1048,10 +1045,7 @@ static bool do_fetch(pdo_stmt_t *stmt, zval *return_value, enum pdo_fetch_type h
pdo_raise_impl_error(stmt->dbh, stmt, "HY000", "could not call user-supplied function");
return 0;
} else {
if (return_all) {
zval_ptr_dtor(return_value); /* we don't need that */
ZVAL_COPY_VALUE(return_value, &retval);
} else if (!Z_ISUNDEF(retval)) {
if (!Z_ISUNDEF(retval)) {
ZVAL_COPY_VALUE(return_value, &retval);
}
}
Expand All @@ -1064,26 +1058,19 @@ static bool do_fetch(pdo_stmt_t *stmt, zval *return_value, enum pdo_fetch_type h
break;
}

if (return_all) {
if ((flags & PDO_FETCH_UNIQUE) == PDO_FETCH_UNIQUE) {
zend_symtable_update(Z_ARRVAL_P(return_all), Z_STR(grp_val), return_value);
} else {
zval grp;
if ((pgrp = zend_symtable_find(Z_ARRVAL_P(return_all), Z_STR(grp_val))) == NULL) {
array_init(&grp);
zend_symtable_update(Z_ARRVAL_P(return_all), Z_STR(grp_val), &grp);
} else {
ZVAL_COPY_VALUE(&grp, pgrp);
}
zend_hash_next_index_insert(Z_ARRVAL(grp), return_value);
}
zval_ptr_dtor_str(&grp_val);
}

return 1;
}
/* }}} */


// TODO Error on the following cases:
// Using any fetch flag with PDO_FETCH_KEY_PAIR
// Combining PDO_FETCH_UNIQUE and PDO_FETCH_GROUP
// Using PDO_FETCH_UNIQUE or PDO_FETCH_GROUP outside of fetchAll()
// Combining PDO_FETCH_PROPS_LATE with a fetch mode different than PDO_FETCH_CLASS
// Improve error detection when combining PDO_FETCH_USE_DEFAULT with
// Reject PDO_FETCH_INTO mode with fetch_all as no support
// Reject $pdo->setAttribute(PDO::ATTR_DEFAULT_FETCH_MODE, value); bypass
static bool pdo_stmt_verify_mode(pdo_stmt_t *stmt, zend_long mode, uint32_t mode_arg_num, bool fetch_all) /* {{{ */
{
int flags = mode & PDO_FETCH_FLAGS;
Expand Down Expand Up @@ -1155,6 +1142,17 @@ PHP_METHOD(PDOStatement, fetch)
RETURN_THROWS();
}

int fetch_mode = how & ~PDO_FETCH_FLAGS;
if (fetch_mode == PDO_FETCH_KEY_PAIR) {
array_init_size(return_value, 1);
bool success = pdo_do_key_pair_fetch(stmt, ori, off, Z_ARRVAL_P(return_value));
if (!success) {
zval_dtor(return_value);
PDO_HANDLE_STMT_ERR();
RETURN_FALSE;
}
return;
}
if (!do_fetch(stmt, return_value, how, ori, off, NULL)) {
PDO_HANDLE_STMT_ERR();
RETURN_FALSE;
Expand Down Expand Up @@ -1234,12 +1232,10 @@ PHP_METHOD(PDOStatement, fetchColumn)
PHP_METHOD(PDOStatement, fetchAll)
{
zend_long how = PDO_FETCH_USE_DEFAULT;
zval data, *return_all = NULL;
zval *arg2 = NULL;
zend_class_entry *old_ce;
zval old_ctor_args, *ctor_args = NULL;
bool error = false;
int flags, old_arg_count;
uint32_t old_arg_count;

ZEND_PARSE_PARAMETERS_START(0, 3)
Z_PARAM_OPTIONAL
Expand All @@ -1253,6 +1249,9 @@ PHP_METHOD(PDOStatement, fetchAll)
RETURN_THROWS();
}

int fetch_mode = how & ~PDO_FETCH_FLAGS;
int flags = how & PDO_FETCH_FLAGS;

old_ce = stmt->fetch.cls.ce;
ZVAL_COPY_VALUE(&old_ctor_args, &stmt->fetch.cls.ctor_args);
old_arg_count = stmt->fetch.cls.fci.param_count;
Expand All @@ -1261,7 +1260,7 @@ PHP_METHOD(PDOStatement, fetchAll)

/* TODO Would be good to reuse part of pdo_stmt_setup_fetch_mode() in some way */

switch (how & ~PDO_FETCH_FLAGS) {
switch (fetch_mode) {
case PDO_FETCH_CLASS:
/* Figure out correct class */
if (arg2) {
Expand Down Expand Up @@ -1328,7 +1327,7 @@ PHP_METHOD(PDOStatement, fetchAll)
}
stmt->fetch.column = Z_LVAL_P(arg2);
} else {
stmt->fetch.column = how & PDO_FETCH_GROUP ? -1 : 0;
stmt->fetch.column = flags & PDO_FETCH_GROUP ? -1 : 0;
}
break;

Expand All @@ -1343,34 +1342,47 @@ PHP_METHOD(PDOStatement, fetchAll)
}
}

flags = how & PDO_FETCH_FLAGS;

if ((how & ~PDO_FETCH_FLAGS) == PDO_FETCH_USE_DEFAULT) {
if (fetch_mode == PDO_FETCH_USE_DEFAULT) {
flags |= stmt->default_fetch_type & PDO_FETCH_FLAGS;
how |= stmt->default_fetch_type & ~PDO_FETCH_FLAGS;
fetch_mode = stmt->default_fetch_type & ~PDO_FETCH_FLAGS;
how = fetch_mode | flags;
}

PDO_STMT_CLEAR_ERR();
if ((how & PDO_FETCH_GROUP) || how == PDO_FETCH_KEY_PAIR ||
(how == PDO_FETCH_USE_DEFAULT && stmt->default_fetch_type == PDO_FETCH_KEY_PAIR)
) {
array_init(return_value);
return_all = return_value;
}
if (!do_fetch(stmt, &data, how | flags, PDO_FETCH_ORI_NEXT, /* offset */ 0, return_all)) {
error = true;

zval data, group_key;

array_init(return_value);

if (fetch_mode == PDO_FETCH_KEY_PAIR) {
while (pdo_do_key_pair_fetch(stmt, PDO_FETCH_ORI_NEXT, /* offset */ 0, Z_ARRVAL_P(return_value)));
PDO_HANDLE_STMT_ERR();
return;
}

if (!error) {
if ((how & PDO_FETCH_GROUP) || how == PDO_FETCH_KEY_PAIR ||
(how == PDO_FETCH_USE_DEFAULT && stmt->default_fetch_type == PDO_FETCH_KEY_PAIR)
) {
while (do_fetch(stmt, &data, how | flags, PDO_FETCH_ORI_NEXT, /* offset */ 0, return_all));
} else {
array_init(return_value);
do {
zend_hash_next_index_insert_new(Z_ARRVAL_P(return_value), &data);
} while (do_fetch(stmt, &data, how | flags, PDO_FETCH_ORI_NEXT, /* offset */ 0, NULL));
// Need to handle the "broken" PDO_FETCH_GROUP|PDO_FETCH_UNIQUE fetch case
//if (flags == PDO_FETCH_GROUP || flags == PDO_FETCH_UNIQUE) {
if (flags & PDO_FETCH_GROUP || flags & PDO_FETCH_UNIQUE) {
while (do_fetch(stmt, &data, how | flags, PDO_FETCH_ORI_NEXT, /* offset */ 0, &group_key)) {
ZEND_ASSERT(Z_TYPE(group_key) == IS_STRING);
Copy link
Member

Choose a reason for hiding this comment

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

Instead of assertion wouldn't it make more sense to have the convert_to_string here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think something I would prefer to do is to pass a reference to a zend_string * to do_fetch() in a follow-up so that it is guaranteed to be a zend_string anyway.

if ((flags & PDO_FETCH_UNIQUE) == PDO_FETCH_UNIQUE) {
zend_symtable_update(Z_ARRVAL_P(return_value), Z_STR(group_key), &data);
} else {
zval *group_ptr = zend_symtable_find(Z_ARRVAL_P(return_value), Z_STR(group_key));
zval group;
if (group_ptr == NULL) {
group_ptr = &group;
array_init(group_ptr);
zend_symtable_update(Z_ARRVAL_P(return_value), Z_STR(group_key), group_ptr);
}
zend_hash_next_index_insert(Z_ARRVAL_P(group_ptr), &data);
}
zval_ptr_dtor_str(&group_key);
}
} else {
while (do_fetch(stmt, &data, how, PDO_FETCH_ORI_NEXT, /* offset */ 0, NULL)) {
zend_hash_next_index_insert_new(Z_ARRVAL_P(return_value), &data);
}
}

Expand All @@ -1381,13 +1393,7 @@ PHP_METHOD(PDOStatement, fetchAll)
ZVAL_COPY_VALUE(&stmt->fetch.cls.ctor_args, &old_ctor_args);
stmt->fetch.cls.fci.param_count = old_arg_count;

/* on no results, return an empty array */
if (error) {
PDO_HANDLE_STMT_ERR();
if (Z_TYPE_P(return_value) != IS_ARRAY) {
array_init(return_value);
}
}
PDO_HANDLE_STMT_ERR();
}
/* }}} */

Expand Down
Loading
Loading