ext/pdo: Refactor PDO::FETCH_CLASS to not rely on a FCI and use a HashTable for ctor_arg #17427
Conversation
Girgias
force-pushed
the
pdo-stmt-remove-fci-usage
branch
3 times, most recently
from
702f741 to
2bad75c
Compare
Girgias
force-pushed
the
pdo-stmt-remove-fci-usage
branch
from
2bad75c to
81490b2
Compare
Girgias
force-pushed
the
pdo-stmt-remove-fci-usage
branch
from
81490b2 to
5711354
Compare
Girgias
force-pushed
the
pdo-stmt-remove-fci-usage
branch
from
5711354 to
808b10e
Compare
Girgias
force-pushed
the
pdo-stmt-remove-fci-usage
branch
from
808b10e to
4791772
Compare
ext/pdo/pdo_stmt.c
Outdated
| zval_ptr_dtor(return_value); | ||
| RETVAL_FALSE; |
There was a problem hiding this comment.
Okay let's just make it consistently not use it then as it's a big refactor anyway it seems like a good opportunity to do so.
Girgias
force-pushed
the
pdo-stmt-remove-fci-usage
branch
from
4791772 to
83b8a92
Compare
ext/pdo/pdo_stmt.c
Outdated
| } | ||
| } else if (default_fetch_mode == PDO_FETCH_CLASS) { | ||
| if (stmt->fetch.cls.ctor_args != NULL) { | ||
| zend_hash_release(stmt->fetch.cls.ctor_args); |
There was a problem hiding this comment.
This can be:
| zend_hash_release(stmt->fetch.cls.ctor_args); | |
| zend_array_release(stmt->fetch.cls.ctor_args); |
as they're real arrays, not storing any persistent or non-zval stuff
There was a problem hiding this comment.
Also: can ctor_args be cyclic? If so, then this is not enough
Girgias
force-pushed
the
pdo-stmt-remove-fci-usage
branch
from
1a63e7b to
1c52e37
Compare
…hTable for ctor_arg To call the constructor we now only store the CE and a HashTable for the arguments. This reduces the size of the _pdo_stmt_t struct from 320 bytes to 232 bytes. Moreover, this now means that the constructor argument array follows the usual CUFA semantics. This change is a BC break, as string keys now act like named arguments. Moreover, the automatic wrapping of by-value arguments for by-ref parameters has been dropped, and the usual E_WARNING is now emitted in those cases. The do_fetch() is heavily refactored to simplify the execution flow, which also makes it easier to understand. Additionally we add a new bitflag in_fetch to prevent modification of the fetch flags by userland when PDO is fetching from the DB.
Girgias
force-pushed
the
pdo-stmt-remove-fci-usage
branch
from
08342ff to
441f9de
Compare
| emitted. | ||
| To pass a variable by-ref to a constructor argument use the general | ||
| array value reference assignment: $ctor_args = [&$valByRef] | ||
| . Attempting to modify a PDOStatement during a call to PDO::fetch(), |
There was a problem hiding this comment.
Could this be worded better? I assume you mean that the default fetch mode cannot be changed, right? It seems like I can still make other changes to the object, e.g. execute it again. Also, it's not immediately clear what is meant by "during a call".
Reposting the comment for better visibility.
There was a problem hiding this comment.
Also, while it's now forbidden to change the default fetch mode, the actual fetch mode can still be changed via the parameter to fetch/fetchAll.
I wonder if there exists a driver that allows the default fetch mode to be changed via PDOStatement::setAttribute.
What exactly is the reason behind introduction of this error?
Commits should be reviewed in order.
This removes the last usage of
zend_fcall_info_args_ex()and basically supersedes #9725, which would allow merging #9723.This also aligns the behaviour of the
ctor_argumentsto be have like CUFA.Some follow-up ideas: