Fix passing generics to BatchQueryResult - #20856
Conversation
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📜 Recent review details⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2026-04-21T21:24:32.138ZApplied to files:
🔇 Additional comments (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughDocblock generics for BatchQueryResult were generalized: ChangesBatchQueryResult Generic Type Annotations
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #20856 +/- ##
=========================================
Coverage 80.15% 80.15%
Complexity 11536 11536
=========================================
Files 374 374
Lines 30213 30213
=========================================
Hits 24218 24218
Misses 5995 5995 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@framework/db/ActiveQuery.php`:
- Around line 75-78: The batch() docblock broadened the key type from int to
array-key incorrectly; keep BatchQueryResult key type as int for batch() to
reflect that BatchQueryResult::next() assigns integer keys (see BatchQueryResult
class and its _key property and next() else-branch where _key is
auto-incremented). Revert or change the PHPDoc for batch() to
BatchQueryResult<int, T[]> (leave each() as BatchQueryResult<array-key, T>), so
callers know batch() yields integer keys only and types remain accurate. Ensure
the docblock mention of batch() and the template params are updated accordingly.
- Around line 75-78: The docblock for ActiveQuery::batch() wrongly widens the
key type to "array-key"; change the `@method` annotation back to use integer keys
(BatchQueryResult<int, T[]>) because BatchQueryResult::next() always sets $_key
to 0,1,2,...; leave the each() annotation as-is. Update the `@method` line
referencing batch($batchSize = 100, $db = null) in ActiveQuery.php to use
BatchQueryResult<int, T[]> so callers destructuring the key keep the correct int
type and the contract matches BatchQueryResult::next().
In `@framework/db/BatchQueryResult.php`:
- Around line 32-34: The docblock template for TValue in BatchQueryResult is
using a vacuous bound ("@template TValue of mixed = mixed"); update the docblock
to remove the redundant "of mixed" and declare it as "@template TValue = mixed"
(keeping the existing "@template TKey of array-key = array-key" and the
"@implements \Iterator<TKey, TValue>" line intact) so the annotation is concise
and semantically equivalent.
- Around line 32-33: The docblock uses a vacuous constraint for the TValue
template; update the class/file docblock in BatchQueryResult.php by replacing
the line "@template TValue of mixed = mixed" with the simplified "@template
TValue = mixed" (leave the existing "@template TKey of array-key = array-key"
line intact) so PHPDoc/PHPStan remains correct but clearer.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5facaeeb-5718-4d95-840b-c578680af0ec
📒 Files selected for processing (3)
framework/db/ActiveQuery.phpframework/db/BatchQueryResult.phpphpstan-baseline.neon
💤 Files with no reviewable changes (1)
- phpstan-baseline.neon
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: PHP 8
- GitHub Check: MSSQL tests / PHP 8.1-mssql-2022-latest
- GitHub Check: MSSQL tests / PHP 8.4-mssql-2022-latest
- GitHub Check: MSSQL tests / PHP 8.0-mssql-2022-latest
- GitHub Check: MSSQL tests / PHP 8.3-mssql-2022-latest
- GitHub Check: SQLite tests / PHP 8.2-ubuntu-22.04
- GitHub Check: MSSQL tests with coverage / PHP 7.4-mssql-2019-latest
- GitHub Check: MSSQL tests with coverage / PHP 8.5-mssql-2022-latest
- GitHub Check: MySQL tests / PHP 8.3-mysql-latest
- GitHub Check: MySQL tests / PHP 8.1-mysql-latest
- GitHub Check: PostgreSQL tests with coverage / PHP 7.4-pgsql-12
- GitHub Check: SQLite tests / PHP 8.0-ubuntu-22.04
- GitHub Check: SQLite tests / PHP 8.1-ubuntu-22.04
- GitHub Check: PostgreSQL tests with coverage / PHP 7.4-pgsql-15
- GitHub Check: SQLite tests with coverage / PHP 7.4-ubuntu-22.04
- GitHub Check: Oracle tests with coverage / PHP 8.5-oracle-slim-faststart
- GitHub Check: Oracle tests with coverage / PHP 7.4-oracle-slim-faststart
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-04-21T21:24:32.138Z
Learnt from: terabytesoftw
Repo: yiisoft/yii2 PR: 20829
File: framework/db/mssql/QueryBuilder.php:678-683
Timestamp: 2026-04-21T21:24:32.138Z
Learning: In yiisoft/yii2, follow the existing PHPStan configuration: `empty()` is explicitly prohibited. When reviewing PHP code, do not recommend replacing strict checks like `$var === null || $var === []` (or similar null/empty-array logic) with `empty($var)`, since that would conflict with the codebase’s `empty()` policy.
Applied to files:
framework/db/BatchQueryResult.phpframework/db/ActiveQuery.php
🔇 Additional comments (3)
framework/db/BatchQueryResult.php (3)
204-220: LGTM —key()andcurrent()return type annotations are correct.The
TKey|nullreturn onkey()accurately reflects both the null-after-reset case and the templated key type.current()returningTValueis consistent with$_valuebeing typedmixedgivenTValue's upper bound ismixed.
87-89:$_keyannotation is correct.
@var TKey|nullaccurately reflects both the null-initialised/reset state and the templated key type, consistent with the assignments innext()andreset().
204-220:key()andcurrent()return annotations are correct.
@return TKey|nullonkey()is accurate:_keyis null afterreset()and typed asTKeyduring iteration.@return TValueoncurrent()is consistent withTValue's upper bound ofmixedmatching$_value'smixedtype.
2 participants
Uh oh!
There was an error while loading. Please reload this page.