Fix GH-20214: PDO::FETCH_DEFAULT unexpected behavior with setFetchMode#21434
Fix GH-20214: PDO::FETCH_DEFAULT unexpected behavior with setFetchMode#21434iliaal wants to merge 3 commits intophp:PHP-8.4from
Conversation
iliaal
force-pushed
the
fix/gh-20214-pdo-fetch-default
branch
from
40d5d5a to
72d1b29
Compare
SakiTakamachi
left a comment
SakiTakamachi
left a comment
There was a problem hiding this comment.
Thanks for the fixes!
I’ve left a few minor comments, but I agree with the overall approach.
|
Good catches. The flags issue was a real bug. I moved the resolution after flags extraction so caller-supplied flags like flags = mode & PDO_FETCH_FLAGS;
if ((mode & ~PDO_FETCH_FLAGS) == PDO_FETCH_USE_DEFAULT) {
mode = stmt->dbh->default_fetch_type | flags;
}Moved the test to |
|
@iliaal |
…ment::setFetchMode When setFetchMode(PDO::FETCH_DEFAULT) is called, mode=0 (PDO_FETCH_USE_DEFAULT) gets stored as the statement's default fetch type. Later, do_fetch() tries to resolve PDO_FETCH_USE_DEFAULT by reading stmt->default_fetch_type, which is also 0 — circular reference that on 8.4 silently fell through to FETCH_BOTH and on master throws a ValueError. Resolve PDO_FETCH_USE_DEFAULT to the connection-level default early in pdo_stmt_setup_fetch_mode(), before flags extraction and the mode switch, so the rest of the function processes the actual fetch mode. Closes phpGH-20214
Extract flags before resolving PDO_FETCH_USE_DEFAULT so caller-supplied flags (e.g. PDO_FETCH_GROUP) are preserved. Move test from pdo_sqlite to ext/pdo/tests/ for cross-driver coverage. Add test cases for query() with second argument and flags preservation.
iliaal
requested review from
Girgias,
TimWolla,
adoy,
arnaud-lb,
derickr,
devnexen,
dstogov,
kocsismate and
ndossche
as code owners
iliaal
force-pushed
the
fix/gh-20214-pdo-fetch-default
branch
from
2d59faa to
40868e4
Compare
@SakiTakamachi I ported the code to 8.4 all the PDO tests pass, however FreeBSD seems to abort with infra error and in other environments un-related ssl (flaky??) tests seem to be failing which are not at all related to this change. |
iliaal
force-pushed
the
fix/gh-20214-pdo-fetch-default
branch
from
7196192 to
742500d
Compare
Firebird requires SELECT ... FROM table syntax; bare SELECT 'val' AS col is not valid. Create a test table to make the test cross-driver compatible.
iliaal
force-pushed
the
fix/gh-20214-pdo-fetch-default
branch
from
742500d to
9fbac37
Compare
|
@SakiTakamachi looks to be ready for 8.4 inclusion, unfortunately there seems to be some CI/CD issue with unrelated openssl test causing failure on [ci/circleci: arm], tried re-triggering the CI to no avail... |
2 participants
When
setFetchMode(PDO::FETCH_DEFAULT)is called,pdo_stmt_setup_fetch_mode()storesPDO_FETCH_USE_DEFAULT(0) as the statement's default fetch type. Later,do_fetch()tries to resolvePDO_FETCH_USE_DEFAULTby readingstmt->default_fetch_type— which is also 0, creating a circular reference. On 8.4 this silently fell through toFETCH_BOTH; on master it throws aValueError.The fix resolves
PDO_FETCH_USE_DEFAULTto the connection-level default (stmt->dbh->default_fetch_type) early inpdo_stmt_setup_fetch_mode(), before flags extraction and the mode switch. The connection default is guaranteed to never bePDO_FETCH_USE_DEFAULT(enforced byPDO::setAttribute), so no circular resolution is possible.Not sure if this should be rebased to the PHP-8.4 branch since the bug affects 8.4 as well (silently returns wrong fetch mode there).
Fixes #20214