Follow-up GH-21434: Fixed the behavior when using FETCH_DEFAULT

[SakiTakamachi]

Follow up #21434

The changes in the above PR resolve most of the issue, but validation can still fail when FETCH_DEFAULT is combined with additional flags.

This happens because the default value already stores both the mode and its flags, and the implementation then attempts to combine it further with newly specified flags.

In the validation function, when FETCH_DEFAULT is specified, any flags provided alongside it are completely ignored, and validation is performed under the assumption that the existing default value will be used as-is.

The manual describes FETCH_DEFAULT as “Special value for using the current default fetch mode.” While it is somewhat unclear whether flags should be included in the default value, the current behavior does include flags as part of the default. Therefore, this change respects the existing implementation.

Specifically, when attempting to set FETCH_DEFAULT as a new default value, the update is skipped to avoid overwriting the current setting.

[iliaal]

@SakiTakamachi stmt->default_fetch_type = PDO_FETCH_BOTH a few lines up still fires unconditionally, so use_default leaves the stmt at FETCH_BOTH instead of the dbh default. That's what gh20214.phpt (hence test failure) is hitting on CI. Either skip that reset under use_default, or re-seed from stmt->dbh->default_fetch_type before returning.

[SakiTakamachi]

@iliaal
pdo_stmt_instantiate is called in two places in pdo_dbh.c, and in both cases the initial value is set as stmt->default_fetch_type = dbh->default_fetch_type;.

Therefore, setting the initial value here as stmt->default_fetch_type = PDO_FETCH_BOTH; is likely incorrect. This has been fixed to align with the call sites of pdo_stmt_instantiate, by initializing it as stmt->default_fetch_type = dbh->default_fetch_type;.

[Girgias]

Seems fine, but I haven't tried this on 8.5

[SakiTakamachi]

@Girgias

The added tests and the tests that are currently failing in CI passed locally on 8.5.

[iliaal]

Seems fine, but I haven't tried this on 8.5

For failing tests there is separate PR ( #21863 ) still needed even with this change for 8.5 & master, new test in those branches needs to be adjusted.

[SakiTakamachi]

@Girgias
CI for 8.5
#21867

[SakiTakamachi]

@iliaal

For failing tests there is separate PR ( #21863 ) still needed even with this change for 8.5 & master, new test in those branches needs to be adjusted.

No additional changes should be necessary.
The test expectations already match the current behavior and should not be modified.

[SakiTakamachi]

Hmm, the results differ from my local environment… It seems I might be missing something. I’ll double-check.

[iliaal]

Looked like it was need to me, but maybe I'm wrong, if it fixes things then I'll close the PR

[SakiTakamachi]

Ah, I resolved the conflict incorrectly when applying the patch to 8.5. I’ve fixed it and am running CI again.

[SakiTakamachi]

@iliaal

The CI failures on 8.5 and master are caused by adding the PDO::FETCH_PROPS_LATE flag to the default value using a bitwise OR in $stmt->setFetchMode($mode | PDO::FETCH_PROPS_LATE);.

The validation expects that when the default value is used, all flags should also come from the default value, and any flags passed as arguments should be ignored.

In other words, PDO::FETCH_PROPS_LATE should be completely ignored here, and the behavior is expected to be the same as using the default value as-is.

[iliaal]

I'll close the other PR