feat: Update ActiveQuery and ActiveRecord dynamic return type extensions for improved type inference and error handling; remove deprecated ActiveQueryObjectType and ActiveRecordObjectType classes.
#36
Conversation
…ensions for improved type inference and error handling; remove deprecated `ActiveQueryObjectType` and `ActiveRecordObjectType` classes.
WalkthroughThis update refactors and enhances PHPStan return type extensions for Yii2 Active Record, replacing custom type classes with PHPStan generics, improving type inference logic, and expanding test coverage. It also updates configuration files, removes obsolete stubs, and introduces comprehensive tests to validate dynamic return type inference for queries and relations. Changes
Sequence Diagram(s)sequenceDiagram
participant TestSuite
participant PHPStanExtension
participant YiiActiveRecord
participant PHPStanTypeSystem
TestSuite->>PHPStanExtension: Invoke method/chain (e.g., find()->asArray()->all())
PHPStanExtension->>YiiActiveRecord: Analyze method call and arguments
PHPStanExtension->>PHPStanTypeSystem: Extract model type, infer return type (using generics)
PHPStanTypeSystem-->>PHPStanExtension: Return inferred type (e.g., ActiveQuery<Model>, array<Model>, Model|null)
PHPStanExtension-->>TestSuite: Provide inferred type for assertion
Suggested labels
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #36 +/- ##
===========================================
Coverage 100.00% 100.00%
Complexity 60 60
===========================================
Files 1 1
Lines 91 91
===========================================
Hits 91 91 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
src/type/ActiveRecordDynamicMethodReturnTypeExtension.php (1)
89-96: 🛠️ Refactor suggestionPrefer
instanceoffor clarity & safety
$arg::class !== Arg::classworks but:
- It’s harder to read than
! $arg instanceof Arg.- It assumes
$argis an object; if a non-object sneaks in the expression will fatal-error before the null-check.- if ($arg === null || $arg::class !== Arg::class) { + if (! $arg instanceof Arg) {Same null-safety and intent, fewer pitfalls.
🧹 Nitpick comments (12)
tests/stub/MyActiveRecord.php (2)
32-34: PHPDoc genericself<string, mixed>is not a recognised PHPStan notation.
selfis already the concrete class; providing two generic parameters to it is meaningless and will be ignored (or flagged) by PHPStan.
Drop the parameters or use the properActiveQuery<self>pattern when you need generics.- * @return self<string, mixed>|null + * @return self|null
24-25: Dynamic property$flagought to be declared to avoid PHP 8.2 deprecation notices.The docblock advertises
@property bool $flag, but there is no typed property nor__get()implementation.
Either add a real property or remove the annotation to keep the stub noise-free.tests/stub/Category.php (1)
42-49: Minor: validation rules can omit defaults when using typed nullable property.Because
$parent_idis already nullable and initialised tonull, the'default'rule can be removed:- [['parent_id'], 'default', 'value' => null],Nit-level only.
tests/stub/User.php (1)
32-34: Public properties may shadow AR attributesDeclaring
$id,$name, and
Leaving them un-initialised or making themprotectedand relying on AR’s magic access is usually safer for stubs.- public int $id = 1; - public string $name = 'John Doe'; - public string $email = 'john@example.com'; + /** @var int */ + public $id; + + /** @var string */ + public $name; + + /** @var string */ + public $email;tests/type/ActiveQueryDynamicReturnTypeExtensionTest.php (1)
7-10: Remove unused import
yii\db\ActiveQueryis never referenced in this test class. Keeping the import can trip strict-rules linters.-use yii\db\ActiveQuery;src/type/ActiveRecordDynamicMethodReturnTypeExtension.php (1)
102-115: Handle multiple constant-string argumentsCurrently the first constant string is taken blindly:
$modelClass = $constantStrings[0]->getValue();If the analyser sees a union like
User::class|Category::class, onlyUseris used, losing precision.
Iterating and building a union (or aGenericObjectTypeper item merged viaTypeCombinator::union()) would keep inference exact.src/type/ActiveRecordDynamicStaticMethodReturnTypeExtension.php (2)
54-57: Docblock references deleted class
ActiveQueryObjectTypewas removed in this PR, yet the constructor doc still says “Creates a new instance of the ActiveQueryObjectType”. Update the comment to avoid confusion.
212-218: Custom query subclasses lose their identityWhen the return type is a generic
ActiveQuery, the extension correctly wraps the model type, but for subclasses you still replace the concrete class with plainActiveQuery::class.Consider:
class PostQuery extends ActiveQuery {} Post::find(); // should return GenericObjectType(PostQuery::class, [Post::class])Preserve
$classNames[0]instead of hard-coding.src/type/ActiveQueryDynamicMethodReturnTypeExtension.php (1)
462-472: Fluent chain drops concrete query subclass
preserveModelType()always returns a newActiveQuerygeneric when$calledOnTypeisn’t the generic root type.
Instead, construct the generic with the original class name to keep IDE autocompletion for custom query APIs.tests/fixture/data/types/ActiveQueryDynamicMethodReturnType.php (1)
12-34: Overly verbose docblockThe 20-line header repeats information already covered by the filename and test names.
Trimming it will keep fixtures lightweight.tests/fixture/data/types/ActiveRecordDynamicMethodReturnType.php (1)
12-33: Consider slimming down the headerLarge per-fixture docblocks add noise; a brief sentence is sufficient.
tests/fixture/data/types/ActiveRecordDynamicStaticMethodReturnType.php (1)
12-34: Reduce boilerplate in test headerKeeping fixtures minimal improves readability and diff clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
composer.json(1 hunks)extension.neon(1 hunks)phpstan.neon(2 hunks)src/type/ActiveQueryDynamicMethodReturnTypeExtension.php(5 hunks)src/type/ActiveQueryObjectType.php(0 hunks)src/type/ActiveRecordDynamicMethodReturnTypeExtension.php(5 hunks)src/type/ActiveRecordDynamicStaticMethodReturnTypeExtension.php(4 hunks)src/type/ActiveRecordObjectType.php(0 hunks)tests/fixture/data/types/ActiveQueryDynamicMethodReturnType.php(1 hunks)tests/fixture/data/types/ActiveRecordDynamicMethodReturnType.php(1 hunks)tests/fixture/data/types/ActiveRecordDynamicStaticMethodReturnType.php(1 hunks)tests/stub/Category.php(1 hunks)tests/stub/MyActiveRecord.php(1 hunks)tests/stub/MyController.php(0 hunks)tests/stub/User.php(1 hunks)tests/type/ActiveQueryDynamicReturnTypeExtensionTest.php(1 hunks)tests/type/ActiveRecordDynamicMethodReturnTypeExtensionTest.php(1 hunks)tests/type/ActiveRecordDynamicStaticMethodReturnTypeExtensionTest.php(1 hunks)
💤 Files with no reviewable changes (3)
- src/type/ActiveQueryObjectType.php
- tests/stub/MyController.php
- src/type/ActiveRecordObjectType.php
🧰 Additional context used
🧬 Code Graph Analysis (9)
tests/stub/Category.php (1)
tests/stub/User.php (2)
tableName(36-39)rules(41-48)
tests/stub/User.php (1)
tests/stub/Category.php (2)
tableName(37-40)rules(42-49)
tests/type/ActiveQueryDynamicReturnTypeExtensionTest.php (2)
tests/type/ActiveRecordDynamicMethodReturnTypeExtensionTest.php (3)
DataProvider(48-52)dataFileAsserts(36-41)getAdditionalConfigFiles(43-46)tests/type/ActiveRecordDynamicStaticMethodReturnTypeExtensionTest.php (3)
DataProvider(50-54)dataFileAsserts(36-43)getAdditionalConfigFiles(45-48)
tests/type/ActiveRecordDynamicMethodReturnTypeExtensionTest.php (2)
tests/type/ActiveQueryDynamicReturnTypeExtensionTest.php (3)
DataProvider(48-52)dataFileAsserts(36-41)getAdditionalConfigFiles(43-46)tests/type/ActiveRecordDynamicStaticMethodReturnTypeExtensionTest.php (3)
DataProvider(50-54)dataFileAsserts(36-43)getAdditionalConfigFiles(45-48)
tests/type/ActiveRecordDynamicStaticMethodReturnTypeExtensionTest.php (2)
tests/type/ActiveQueryDynamicReturnTypeExtensionTest.php (3)
DataProvider(48-52)dataFileAsserts(36-41)getAdditionalConfigFiles(43-46)tests/type/ActiveRecordDynamicMethodReturnTypeExtensionTest.php (3)
DataProvider(48-52)dataFileAsserts(36-41)getAdditionalConfigFiles(43-46)
tests/fixture/data/types/ActiveRecordDynamicMethodReturnType.php (3)
tests/stub/Category.php (1)
Category(31-50)tests/stub/MyActiveRecord.php (1)
MyActiveRecord(29-54)tests/stub/User.php (1)
User(30-49)
tests/fixture/data/types/ActiveQueryDynamicMethodReturnType.php (1)
tests/stub/MyActiveRecord.php (1)
MyActiveRecord(29-54)
tests/fixture/data/types/ActiveRecordDynamicStaticMethodReturnType.php (3)
tests/stub/Category.php (1)
Category(31-50)tests/stub/MyActiveRecord.php (1)
MyActiveRecord(29-54)tests/stub/User.php (1)
User(30-49)
src/type/ActiveQueryDynamicMethodReturnTypeExtension.php (4)
src/type/ActiveRecordDynamicStaticMethodReturnTypeExtension.php (2)
__construct(58-58)getClass(73-76)src/reflection/UserPropertiesClassReflectionExtension.php (1)
__construct(57-61)src/reflection/ComponentPropertyReflection.php (4)
__construct(46-50)getDocComment(111-116)getReadableType(129-132)getType(145-148)src/type/ActiveRecordDynamicMethodReturnTypeExtension.php (2)
isMethodSupported(127-130)getClass(62-65)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: phpunit / PHP 8.1-windows-latest
- GitHub Check: phpunit / PHP 8.3-windows-latest
- GitHub Check: phpunit / PHP 8.4-windows-latest
- GitHub Check: phpunit / PHP 8.2-windows-latest
- GitHub Check: phpunit-compatibility / PHP 8.4-windows-latest
- GitHub Check: phpunit-compatibility / PHP 8.3-windows-latest
- GitHub Check: phpunit-compatibility / PHP 8.2-windows-latest
- GitHub Check: phpunit-compatibility / PHP 8.1-windows-latest
🔇 Additional comments (5)
composer.json (1)
14-15: Avoid pinning todev-master; prefer a tagged constraint or explicit branch alias.Requiring
yiisoft/yii2atdev-masterforces every downstream consumer of this library intominimum-stability=dev, which is generally discouraged for published packages.
Consider one of the following instead:- "yiisoft/yii2": "dev-master" + "yiisoft/yii2": "^2.0.52 || ^2.2@dev" # keep dev only for 2.2 lineor add
"minimum-stability": "dev", "prefer-stable": trueso that consumers understand the intent.Would you double-check whether any feature used in the new extensions actually requires code that is only available on the
masterbranch?phpstan.neon (1)
30-32:✅ Verification successful
Verify that
tests/fixture/config/config.phpexists and is load-safe.A wrong path will make PHPStan silently ignore the bootstrap and the new return-type extensions will misbehave.
🏁 Script executed:
#!/bin/bash # Ensure the referenced config file actually exists test -f tests/fixture/config/config.php && echo "✓ config present" || echo "✗ missing config"Length of output: 84
🏁 Script executed:
#!/bin/bash # Verify that config.php is syntactically valid (load-safe) php -l tests/fixture/config/config.php && echo "✓ config.php syntax is valid" || echo "✗ syntax errors detected"Length of output: 168
phpstan.neon config_path verified
tests/fixture/config/config.phpexists- Syntax check passed (no errors)
extension.neon (1)
44-46: Service reorder looks fine.Moving
HeaderCollectionDynamicMethodReturnTypeExtensionfurther down does not affect autowiring; DI container order is preserved. No action required.tests/type/ActiveRecordDynamicMethodReturnTypeExtensionTest.php (1)
31-53: Looks goodClean, minimal test harness that follows the pattern used in the companion suites.
No issues found.tests/type/ActiveRecordDynamicStaticMethodReturnTypeExtensionTest.php (1)
31-55: Looks goodConsistent with the other type-inference tests and correctly wired to the shared
extension.neon.
…hecker.json` for enhanced type checking.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/type/ActiveQueryDynamicMethodReturnTypeExtension.php (1)
226-233: Model type extraction still ignores customActiveQuerysubclassesThe check is limited to
getClassName() === ActiveQuery::class, so a subclass such asPostQuerydegrades tomixed, breaking all downstream inference.
This was already pointed out in a previous review and has not been addressed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/type/ActiveQueryDynamicMethodReturnTypeExtension.php(5 hunks)tests/fixture/data/types/ActiveQueryDynamicMethodReturnType.php(1 hunks)tests/stub/Category.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/stub/Category.php
- tests/fixture/data/types/ActiveQueryDynamicMethodReturnType.php
… query subclasses.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
src/type/ActiveQueryDynamicMethodReturnTypeExtension.php (4)
393-398: Simplify guard for defaultasArray()argument
TheArg::classcomparison is redundant (it is always true) and obscures intent.- if (isset($methodCall->args[0]) === false || ($methodCall->args[0]::class === Arg::class) === false) { - return new ConstantBooleanType(true); - } - - return $scope->getType($methodCall->args[0]->value); + if ($methodCall->args === []) { + // `asArray()` default in Yii is `true`. + return new ConstantBooleanType(true); + } + + return $scope->getType($methodCall->args[0]->value);
467-469: Sameinstanceofissue when preserving fluent return type- if ($returnType::class === ThisType::class) { + if ($returnType instanceof ThisType) {
496-503: Fluent type preservation should keep custom query subclassesCurrently a non-generic
$calledOnType(e.g. an intersection) downgrades toActiveQuery, losing the concrete subclass. Re-use the original class when possible:- if ($modelType::class !== MixedType::class) { - return new GenericObjectType(ActiveQuery::class, [$modelType]); + if ($modelType::class !== MixedType::class) { + $queryClass = $calledOnType instanceof GenericObjectType + ? $calledOnType->getClassName() + : ActiveQuery::class; + + return new GenericObjectType($queryClass, [$modelType]); }
151-157: 🛠️ Refactor suggestionPrefer
instanceofover strict class‐string comparison
The current check fails if the return type is wrapped or proxied. Usinginstanceofis safer and idiomatic.- $returnType = $variants[0]->getReturnType(); - - return $returnType::class === ThisType::class; + $returnType = $variants[0]->getReturnType(); + + return $returnType instanceof ThisType;
🧹 Nitpick comments (1)
src/type/ActiveQueryDynamicMethodReturnTypeExtension.php (1)
222-228: Useinstanceofto detectGenericObjectType
Relying on class-string equality breaks when$calledOnTypeis an intersection or union containingGenericObjectType.- if ($calledOnType::class === GenericObjectType::class) { + if ($calledOnType instanceof GenericObjectType) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/type/ActiveQueryDynamicMethodReturnTypeExtension.php(5 hunks)tests/fixture/data/types/ActiveQueryDynamicMethodReturnType.php(1 hunks)tests/stub/Post.php(1 hunks)tests/stub/PostQuery.php(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/stub/Post.php
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/fixture/data/types/ActiveQueryDynamicMethodReturnType.php
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/type/ActiveQueryDynamicMethodReturnTypeExtension.php (2)
src/type/ActiveRecordDynamicStaticMethodReturnTypeExtension.php (2)
__construct(58-58)getClass(73-76)src/type/ActiveRecordDynamicMethodReturnTypeExtension.php (2)
isMethodSupported(127-130)getClass(62-65)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: phpunit / PHP 8.3-windows-latest
- GitHub Check: phpunit / PHP 8.2-windows-latest
- GitHub Check: phpunit-compatibility / PHP 8.3-windows-latest
- GitHub Check: phpunit-compatibility / PHP 8.2-windows-latest
- GitHub Check: phpunit-compatibility / PHP 8.1-windows-latest
🔇 Additional comments (1)
tests/stub/PostQuery.php (1)
15-21: Looks good – concise custom query helper
Implementation is minimal, type-safe, and matches Yii’s fluent API expectations.
No issues spotted.
There was a problem hiding this comment.
Pull Request Overview
This PR updates the dynamic return type extensions for Yii ActiveRecord and ActiveQuery to improve type inference and error handling, while also removing deprecated custom object types.
- Refactors type extensions to use native generic types and improves return type inference for both static and dynamic methods.
- Updates test stubs and fixture files to validate enhanced type analysis, and revises configuration files and dependency constraints.
- Removes deprecated classes (ActiveRecordObjectType and ActiveQueryObjectType) and adjusts extension configurations.
Reviewed Changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/* | Added and updated test cases for dynamic method and static method return types. |
| tests/stub/User.php, Post.php, etc. | Updated stubs for improved type inference and documentation. |
| src/type/* | Refactored dynamic return type extensions; removed deprecated object type classes. |
| phpstan.neon, extension.neon, composer.json | Updated configuration and dependency version constraints. |
| CHANGELOG.md | Updated changelog to reflect refactor and removal of deprecated classes. |
Summary by CodeRabbit
New Features
Category,Post, andPostQueryfor enhanced testing scenarios.UserandMyActiveRecordstubs to improve static analysis coverage.Bug Fixes
Refactor
Chores