Repository navigation
ResultSet Refactoring for Mago - #201
Merged
Merged
Conversation
AbstractResultSet::current() ended with `is_array($data) ? $data : null` while declaring `array|object|null`. The guard exists to turn `false`, meaning the results are exhausted, into `null`, and it took objects with it. Pdo\Result::isBuffered() is always false, so a PDO result never takes the `buffer === -1` path that returns the row untouched, and the guard saw every row. Under FETCH_OBJ and FETCH_LAZY every row became null, with no exception and no warning. FETCH_BOUND is unchanged on purpose: that mode carries its row in variables bound by reference and returns only true, so null is correct. Five further defects followed from the same fact, that a row need not be an array: - initialize() measured the first element of an array rather than the array, so an array of objects raised an uncaught TypeError from count(). - initialize() called getIterator() once, but that method returns Traversable and may hand back another aggregate. It now walks the chain and rejects one that never bottoms out in an Iterator. - The iteration methods reached the data source directly, so calling them before initialize() raised an Error. They now report an uninitialised result set. - HydratingResultSet::toArray() handed non-objects to the hydrator. The @throws tag promised an exception that was never raised; it is now. - ResultSet::current() and RowPrototypeResultSet::current() declared return types that excluded objects the parent can return. ResultSetFetchModeTest drives a real SQLite result through the PDO driver across every entry in Result::VALID_FETCH_MODES: nine modes return rows, FETCH_BOUND returns null while its bound columns fill, and three throw. A single FETCH_OBJ case would have missed FETCH_LAZY entirely. The three that throw are adapter defects raised separately as php-db#198 and php-db#199. src/ResultSet goes from 90 baselined findings to 36. The remainder are the mixed family that follows from a row being mixed, complexity metrics, array shapes no truthful annotation can narrow, and a Mago stub that treats DOMNodeList and its kin as Traversable but not Iterator. No public signature is narrowed and nothing is sealed, so the change is binary compatible for downstream consumers.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
simon-mundy
added
bug
simon-mundy
changed the title
The previous commit widened ResultSet::current() to array|object|null and RowPrototypeResultSet::current() likewise, so that object rows could pass through untouched. That made both declarations weaker than the contract each class actually offers: a ResultSet hands back ArrayObject, and a RowPrototypeResultSet hands back its prototype. Both keep their original narrow return types. A row that already satisfies the declared type is handed back untouched, preserving instance identity. Any other object row is reduced to an array and used to fill a clone of the prototype, so FETCH_OBJ now yields an ArrayObject rather than a stdClass. HydratingResultSet did the same thing behind a ?object declaration: it tested is_array() and returned null for everything else, so object rows were still discarded there. It now hydrates them on the same terms. AbstractResultSet::rowToArray() performs the reduction. A row carrying its values as elements rather than properties, ArrayObject being the common case, is read through its iterator. Everything else goes through get_object_vars(), which sees only public properties from outside the row's scope; a plain cast would instead yield mangled keys for anything private. A row that exposes nothing either way is rejected rather than emptied. PDORow is the case that matters: FETCH_LAZY resolves columns through __get(), so both get_object_vars() and an array cast return an empty array. Converting it would drop every column, which is a quieter version of the defect this branch set out to fix, so FETCH_LAZY now raises RuntimeException::forUnconvertibleRow() through the three result sets that fill a prototype. It continues to work on AbstractResultSet, which promises nothing about the row's type. HydratingResultSet::current() gained enough work to cross the halstead threshold, so the conversion and hydration move into hydrateRow(). src/ResultSet is at 100% line coverage and 35 baselined findings.
Every array row fills the prototype and every object row is either already a prototype or reduced to an array that does, so the method could only ever return a prototype or null. The declared `array` arm was unreachable. This is the one return type in the component that narrows against 0.6.x. A subclass overriding current() with the wider declaration would no longer satisfy covariance.
ArrayResultSet had no current() override, so it inherited array|object|null from the base class and could hand back an object despite being the result set that exists to yield arrays. It now declares ?array and reduces an object row through rowToArray(), on the same terms as the three prototype-filling result sets. Five cases cover the method: an array row, an object row, a row carrying its values as elements, a row that exposes nothing, and an exhausted data source. Also removes the inline comments and docblock prose added earlier in this branch. The @throws and @return tags stay, since mago analyze reports unhandled-thrown-type and imprecise-type without them.
The case was added in f394207 with no consumer. Nothing read it, so a caller selecting it silently got Array behaviour: the constructor accepted it, getReturnType() reported it, current() checked only for ArrayObject and let everything else fall through to the raw row. Every observable signal said it worked. Prototype is the successor name for ArrayObject. It follows the rename already in flight elsewhere in the class, where TYPE_ARRAYOBJECT gives way to the enum and setArrayObjectPrototype() to setRowPrototype(); for ResultSet the ArrayObject and the row prototype are the same object. Both cases now fill the prototype. The decision is resolved once in the constructor rather than on each current() call, since the return type cannot change after construction. Both properties are readonly so that a later setter cannot leave the derived flag stale. Per-row cost drops to a bool read, below the enum comparison it replaces. The match is exhaustive. A fourth case with no decision here raises UnhandledMatchError at construction, before any row is touched, rather than falling through current() unnoticed. returnTypeProvider() covers every case and asserts its own completeness against ResultSetReturnType::cases(), so the same omission fails the suite too.
- current() moves to each concrete set; mapRow() shapes the row - buffering moves behind RowBuffer, with __clone to unshare it - ObjectResultSet serves FETCH_OBJ and FETCH_LAZY rows - rows are typed through a TRow template - the data source narrows to ?Iterator
PHP rejects `self` inside an intersection type, so the capability interfaces cannot take the rule's advice. Drops the three stale baseline entries.
The mysqli Result closes its statement once a fetch finds no row, so the valid() that followed current() fetched again from a closed statement and threw on an empty result. A null or false row now ends the read on its own, which also restores the 0.6.x handling of a null row inside array data.
Iterating a ResultSet cost about 25 method calls per row against 7 in 0.6.x, nearly all of it RowBuffer bookkeeping that had nothing to do once buffering was settled. - RowBuffer folds back into AbstractResultSet as a RowBufferState and an array of held rows; rowBuffer() and __clone() go - $readsDirectly and $storesRows, derived from the state by setBufferState(), give valid(), current() and next() a direct branch for each common state - mapRow() reads plain arrays first and clones the row prototype property rather than calling its getter, as laminas-db did
A buffer held the rows mapRow() had shaped, so every pass handed out the same objects: a change made to a row in one pass showed up in the next, a row prototype set after a pass was ignored, and a buffer of ArrayObjects cost around 15 times the memory of the raw arrays plus cycle-collector scans. The buffer now holds the rows as the data source returned them and maps them on each read. ResultSet and RowPrototypeResultSet behave as they did in 0.6.x; HydratingResultSet now hydrates a fresh object on every pass instead of returning the same instance, which is documented.
mapRow() read the private row prototype and hydrator properties directly, so a subclass overriding getRowPrototype() or getHydrator() no longer decided how its rows were built. Each set now asks its getters once per data source and keeps the answer, which honours the overrides without a method call on every row. A buffered HydratingResultSet hydrated every row again on each pass, handing out new objects, so changes made to an entity in one pass were lost in the next. Result sets can now opt into holding the rows mapRow() built; HydratingResultSet does, and returns the same entities on every pass as laminas-db and earlier PhpDb releases did. ResultSet and RowPrototypeResultSet still build fresh rows from raw ones.
…he new hooks currentRow() only reaches its last branch on the first read of an unbuffered set: Storing with a data source always takes the buffered branch, and every other settled state reads directly. The buffering code left in that branch could not run, which left a line uncovered. The tests now name the methods they exercise, so the new hooks count towards coverage: holdRow(), holdsMappedRows() and resetResolvedConfiguration(), plus RowPrototypeResultSet's constructor, getter and setter.
- Resolve each row prototype in its own statement before cloning it. The formatter strips the parentheses from clone ($x ??= ...), and the analyzer then misreads the expression. - HOLDS_MAPPED_ROWS replaces the holdsMappedRows() hook as a protected bool constant read through static::. Its @var bool stops the analyzer taking the base class's false as the only possible value. - HydratingResultSet no longer overrides resetResolvedConfiguration(), which kept it within the method limit. It asks its getters on the first row and again after setHydrator() or setRowPrototype(). - Annotate the held row currentRow() returns as TRow. - The lint baseline allows a third Halstead finding in AbstractResultSet, for initialize().
- The fetch mode guidance says FETCH_NUM, FETCH_BOTH and FETCH_KEY_PAIR fail in HydratingResultSet, whose hydrators reject integer keys. - The AbstractResultSet API block shows the current signatures, mapRow() and the HOLDS_MAPPED_ROWS and resetResolvedConfiguration() extension points, in place of current() and toArray(). - The ResultSetReturnType listing includes Prototype, and the ResultSet constructor shows its real defaults. - The toArray() sample catches ValueError rather than RuntimeException, and notes that catch (\Exception) misses it. - What a Buffer Holds says when each set asks its getters again.
- Storing gets separate branches for raw and mapped rows, so a buffered read no longer calls holdRow() or reads HOLDS_MAPPED_ROWS per row. - valid() has its own Storing branch, and its last path drops a held-row lookup that could never succeed. - next() counts how far it has advanced the data source rather than comparing its key() with the position, so a source keyed other than 0, 1, 2... is advanced instead of repeating its first row. - ResultSet and RowPrototypeResultSet resolve the row prototype when a data source or prototype is set, not on each row.
ValueError extended PHP's \ValueError, an Error, so catch (\Exception) missed a refused row. UnexpectedValueException extends the SPL class of the same name, which catch (\RuntimeException) and catch (\Exception) both reach, and joins ExceptionHierarchyTest. Each forRowThatIs...() constructor now has its own template constant, as the convention requires; the messages are unchanged. PhpDb\Exception\ValueError had no other user and is removed.
tyrsson
approved these changes
Oct 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Clears the Mago findings in
src/ResultSetby fixing the typing problems behind them, most of which came fromAbstractResultSet::current()doing both buffering and row normalisation.Changes
current().currentRow()handles buffering and a new abstractmapRow()shapes the row.PhpDb\ResultSet\Exception\UnexpectedValueExceptioninstead of becomingnull.ObjectResultSetforFETCH_OBJandFETCH_LAZYrows.$buffersentinel is replaced by a privateRowBufferStateenum, with a fast path per state.HydratingResultSetkeeps returning the same objects on every buffered pass, opted into throughHOLDS_MAPPED_ROWS.Breaking changes
current()andmapRow().parent::current(),rowToArray()and$bufferare gone.FETCH_OBJ,FETCH_LAZYandFETCH_BOUNDrows throw on the row-data sets.getReturnType()returns the enum, so=== self::TYPE_ARRAYchecks are always false.$dataSourceandgetDataSource()are typed?Iterator.mago lint,mago analyzeand the unit tests pass.