ext/pdo: Refactor PDOStatement::fetchAll() #17347
Conversation
Girgias
force-pushed
the
pdo-fetchall-refactoring
branch
from
a4d01eb to
9078169
Compare
Girgias
force-pushed
the
pdo-fetchall-refactoring
branch
2 times, most recently
from
c02adac to
3b5043d
Compare
This also refactors the internal do_fetch() function to stop doing wonky stuff to handle grouping, which is a feature of fetchAll Handle PDO_FETCH_KEY_PAIR on its own as GROUP and UNIQUE flags can interfere with it
Girgias
force-pushed
the
pdo-fetchall-refactoring
branch
from
3b5043d to
d38ba8b
Compare
| return false; | ||
| } | ||
|
|
||
| if (flags == PDO_FETCH_GROUP && stmt->fetch.column == -1) { |
There was a problem hiding this comment.
Isn't this the same as what you have on line 769?
There was a problem hiding this comment.
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.
| //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); |
There was a problem hiding this comment.
Instead of assertion wouldn't it make more sense to have the convert_to_string here?
There was a problem hiding this comment.
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.
| return true; | ||
| } | ||
|
|
||
| /* When fetching a column we only do one value fetch, so handle it separately */ |
There was a problem hiding this comment.
This could still be in the switch, no?
There was a problem hiding this comment.
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.
2 participants
This also refactors the internal do_fetch() function to stop doing wonky stuff to handle grouping, which is a feature of fetchAll
Handle PDO_FETCH_KEY_PAIR on its own as GROUP and UNIQUE flags can interfere with it