Add multi-variant overloads for PDOStatement::fetchAll() in function signature map#5630
Conversation
VincentLanglet
left a comment
There was a problem hiding this comment.
Add PHP8+ test with
$pdo = new PDO(
dsn: "mysql:host=servername;dbname=dbname;charset=utf8mb4",
username: "username",
password: "password"
);
$dbQuery = $pdo->prepare("SELECT * FROM table;");
$dbQuery->fetchAll(mode: PDO::FETCH_DEFAULT);
$dbQuery->fetchAll(
mode: PDO::FETCH_COLUMN,
column: 1
);
$dbQuery->fetchAll(
mode: PDO::FETCH_CLASS,
class: "class"
);
Name arguments should work correctly
|
Done. I've added named argument tests for Regarding the reviewer's suggestion to also test |
VincentLanglet
left a comment
There was a problem hiding this comment.
fetchAll() uses overloaded signatures — the available named parameters change depending on which $mode you pass.
So the four signature
PDOStatement::fetchAll(int $mode = PDO::FETCH_DEFAULT): array
PDOStatement::fetchAll(int $mode = PDO::FETCH_COLUMN, int $column): array
PDOStatement::fetchAll(int $mode = PDO::FETCH_CLASS, string $class, ?array $constructorArgs): array
PDOStatement::fetchAll(int $mode = PDO::FETCH_FUNC, callable $callback): array
```
should be usable with the named parameters (for all parameters).
|
Here's a summary of what was done to address the review: Problem: The reviewer requested that Changes made:
|
VincentLanglet
left a comment
There was a problem hiding this comment.
Should we also/instead improve ParametersAcceptorSelector::selectFromArgs in order to support this method ? Like it's the case for
ArrayMapArgVisitor
CurlSetOptArgVisitor
ArrayFilterArgVisitor
ImplodeArgVisitor
and more ?
|
Both pass cleanly. There are no code changes needed for this review comment — it's a design question that should be answered in the PR discussion. The reviewer's question "Should we also/instead improve
Since there are no code changes to make and nothing to push, the response to this review is explanatory. The tests and static analysis all pass. |
VincentLanglet
left a comment
There was a problem hiding this comment.
FILE: src/Reflection/SignatureMap/Php8SignatureMapProvider.php
FOUND 1 ERROR AFFECTING 1 LINE
298 | ERROR | [x] Function array_slice() should not be referenced via a
| | fallback global name, but via a use statement.
| | (SlevomatCodingStandard.Namespaces.ReferenceUsedNamesOnly.ReferenceViaFallbackGlobalName)
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
|
Fixed the coding standard violation by adding |
6e601fb to
da44eea
Compare
…n signature map - Replace the single `PDOStatement::fetchAll` signature (which combined all parameter types into one union) with proper multi-variant overloads matching `PDO::query` and `PDOStatement::setFetchMode` patterns - Add variant '1 for FETCH_COLUMN mode (int $mode, int $colno) - Add variant '2 for FETCH_CLASS mode (int $mode, string $classname, ?array $ctorargs) - Add variant '3 for FETCH_FUNC mode (int $mode, callable $callback) - Update both base functionMap.php and functionMap_php80delta.php - Verified PDO::query and PDOStatement::setFetchMode already had correct overloads
da44eea to
5e20c36
Compare
|
|
||
| class Foo | ||
| { | ||
| public function test(\PDOStatement $stmt): void |
There was a problem hiding this comment.
in the docs on https://www.php.net/manual/en/class.pdostatement.php
I can see
can we make more tests out of that?
There was a problem hiding this comment.
Added tests for PDOStatement implementing IteratorAggregate and Traversable as the reviewer requested:
foreachiteration overPDOStatement- Calling
getIterator()onPDOStatement - Passing
PDOStatementtoTraversableandIteratorAggregatetypehints
All tests pass (12044 tests, 79632 assertions) and make phpstan reports no errors.
…able Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… Traversable" This reverts commit 5f47180.
Summary
PDOStatement::fetchAll()had a single function map entry withfetch_argumenttyped asint|string|callable, which caused false positives when passing constructor arguments (an array) as the third parameter inFETCH_CLASSmode. The fix adds proper multi-variant overloads.Changes
resources/functionMap.php: Replaced singlePDOStatement::fetchAllentry with 4 variants:(int $how)— for simple fetch modes likeFETCH_ASSOC(int $fetch_column, int $colno)— forFETCH_COLUMN(int $fetch_class, string $classname, ?array $ctorargs)— forFETCH_CLASS(int $fetch_func, callable $callback)— forFETCH_FUNCresources/functionMap_php80delta.php: Same overloads witharrayreturn type (instead ofarray|false)tests/PHPStan/Rules/Methods/data/bug-5509.phpRoot cause
The function map had a single signature where the second parameter was typed as
int|string|callable— a union of all possible types across different fetch modes. When callingfetchAll(PDO::FETCH_CLASS, StdClass::class, [new StdClass]), the array constructor argument didn't match any type in the variadic expansion ofint|string|callable. The fix follows the established pattern already used byPDO::queryandPDOStatement::setFetchMode, which have separate overload variants for each fetch mode.Analogous cases checked
PDO::query— already has proper overloads (variants '1, '2, '3)PDOStatement::setFetchMode— already has proper overloads (variants '1, '2, '3)PDOStatement::fetch— has fixed parameter types (int, int, int), no mode-dependent types neededTest
tests/PHPStan/Rules/Methods/data/bug-5509.php: Tests all valid calling patterns offetchAll()—FETCH_CLASSwith class name and constructor args,FETCH_CLASSwith just class name,FETCH_COLUMNwith column number,FETCH_FUNCwith callable, no args, and just mode.Fixes phpstan/phpstan#5509