Add support for yii\web\User Components (Identity Property)
#32
Conversation
Added support for IdentityClass Components implemented by yii\web\User. This is acheived by using genric types in yii\web\User (stubs for now). Need to update inline comments (PHPDocs). Need to write tests. Need to upstream the addition of generic type in yii\web\User to Yii.
wrote basic tests need to update docs
|
""" WalkthroughThis update introduces enhanced handling and richer type representation of Yii application components, especially the Changes
Sequence Diagram(s)sequenceDiagram
participant App as Yii Application
participant ServiceMap
participant UserComponent
participant MyUser
App->>ServiceMap: processComponents(config)
ServiceMap->>ServiceMap: Store full component definitions
App->>ServiceMap: getComponentClassById('customUser')
ServiceMap-->>App: Return User::class
App->>ServiceMap: getComponentPropertyById('customUser', 'identityClass')
ServiceMap-->>App: Return 'MyUser'
App->>UserComponent: Create User<T=MyUser>
UserComponent->>MyUser: Use as identity
Possibly related PRs
Poem
✨ Finishing Touches
🪧 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 ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #32 +/- ##
==============================================
- Coverage 100.00% 89.36% -10.64%
- Complexity 53 61 +8
==============================================
Files 1 1
Lines 79 94 +15
==============================================
+ Hits 79 84 +5
- Misses 0 10 +10 ☔ 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: 2
🧹 Nitpick comments (2)
tests/ServiceMapTest.php (1)
120-124: Fix inconsistent test comment.The assertion expects
User::classbut the comment claims it should resolve to'MyUser::class'. Based on the ServiceMap implementation, the assertion is correct since it returns the component's class (User), not the identityClass (MyUser).Apply this diff to fix the comment:
- 'ServiceMap should resolve user component id \'customInitializedUser\' to \'MyUser::class\'.', + 'ServiceMap should resolve user component id \'customInitializedUser\' to \'User::class\'.',tests/stub/MyUser.php (1)
10-36: LGTM! Complete IdentityInterface implementation for testing.The
MyUserclass properly implements all requiredIdentityInterfacemethods with appropriate stub return values. The implementation follows Yii conventions by extendingActiveRecord.Consider adding return type declarations to improve type safety:
- public static function findIdentity($id) + public static function findIdentity($id): ?static { return new static(); } - public static function findIdentityByAccessToken($token, $type = null) + public static function findIdentityByAccessToken($token, $type = null): ?static { return new static(); } - public function getId() + public function getId(): string|int { return 1; } - public function getAuthKey() + public function getAuthKey(): ?string { return ''; } - public function validateAuthKey($authKey) + public function validateAuthKey($authKey): bool { return true; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
CHANGELOG.md(1 hunks)README.md(3 hunks)extension.neon(1 hunks)phpunit.xml.dist(1 hunks)src/ServiceMap.php(5 hunks)src/reflection/ApplicationPropertiesClassReflectionExtension.php(3 hunks)src/reflection/UserPropertiesClassReflectionExtension.php(1 hunks)stubs/BaseObject.stub(1 hunks)stubs/Component.stub(1 hunks)stubs/Configurable.stub(1 hunks)stubs/IdentityInterface.stub(1 hunks)stubs/User.stub(1 hunks)tests/ServiceMapTest.php(2 hunks)tests/fixture/config.php(2 hunks)tests/stub/MyUser.php(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
tests/ServiceMapTest.php (1)
src/ServiceMap.php (1)
getComponentClassById(121-137)
tests/fixture/config.php (2)
tests/stub/MyActiveRecord.php (1)
MyActiveRecord(34-59)tests/stub/MyUser.php (1)
MyUser(10-36)
src/reflection/ApplicationPropertiesClassReflectionExtension.php (2)
src/ServiceMap.php (2)
getComponentClassById(121-137)getComponentPropertyById(156-176)src/reflection/ComponentPropertyReflection.php (1)
ComponentPropertyReflection(37-271)
🪛 PHPStan (2.1.15)
src/ServiceMap.php
166-166: Variable property access on object.
(property.dynamicName)
336-336: Property yii2\extensions\phpstan\ServiceMap::$components (array<string, array<string, mixed>|object>) does not accept array<string, array|object>.
(assign.propertyType)
🪛 GitHub Check: codecov/patch
src/ServiceMap.php
[warning] 136-136: src/ServiceMap.php#L136
Added line #L136 was not covered by tests
[warning] 151-151: src/ServiceMap.php#L151
Added line #L151 was not covered by tests
[warning] 156-156: src/ServiceMap.php#L156
Added line #L156 was not covered by tests
[warning] 158-158: src/ServiceMap.php#L158
Added line #L158 was not covered by tests
[warning] 160-161: src/ServiceMap.php#L160-L161
Added lines #L160 - L161 were not covered by tests
[warning] 164-166: src/ServiceMap.php#L164-L166
Added lines #L164 - L166 were not covered by tests
[warning] 168-168: src/ServiceMap.php#L168
Added line #L168 was not covered by tests
[warning] 171-172: src/ServiceMap.php#L171-L172
Added lines #L171 - L172 were not covered by tests
[warning] 175-175: src/ServiceMap.php#L175
Added line #L175 was not covered by tests
🪛 GitHub Check: phpstan / PHP 8.4-ubuntu-latest
src/ServiceMap.php
[failure] 166-166:
Variable property access on object.
[failure] 336-336:
Property yii2\extensions\phpstan\ServiceMap::$components (array<string, array<string, mixed>|object>) does not accept array<string, array|object>.
🔇 Additional comments (20)
phpunit.xml.dist (1)
24-24: Formatting alignment update
Indentation of<directory suffix=".php">./src/type</directory>was adjusted for consistency. No behavioral change.stubs/Configurable.stub (1)
1-5: NewConfigurableinterface stub
This empty interface correctly introducesyii\base\Configurablefor stubbing purposes.stubs/IdentityInterface.stub (1)
1-5: NewIdentityInterfacestub
Defines theyii\web\IdentityInterfaceas expected for generic constraint inUserstub.CHANGELOG.md (1)
10-10: Changelog updated with new enhancement entry
Entry for Enh #32 correctly reflects support foryii\web\Useridentity property.extension.neon (1)
8-13:✅ Verification successful
Verify stub file paths in configuration
Ensure that the listedstubFilesexist in thestubs/directory to avoid loading errors.
🏁 Script executed:
#!/bin/bash # Check that each stub file declared in extension.neon exists for file in stubs/Configurable.stub stubs/BaseObject.stub stubs/Component.stub stubs/IdentityInterface.stub stubs/User.stub; do if [[ ! -f "$file" ]]; then echo "ERROR: Missing stub file: $file" exit 1 fi done echo "All stub files are present."Length of output: 871
All
stubFilesentries verified
All files listed understubFilesinextension.neonexist in thestubs/directory. No missing stub files detected.stubs/BaseObject.stub (1)
1-5: LGTM! Well-structured stub file.The stub correctly defines the
BaseObjectclass in the appropriate namespace and implements theConfigurableinterface as expected for Yii's component hierarchy.stubs/Component.stub (1)
1-5: LGTM! Correct inheritance hierarchy.The stub properly establishes the Component class extending BaseObject, which aligns with Yii's component architecture and supports the type inference improvements for the User component.
README.md (1)
17-17: LGTM! Good formatting improvements.The whitespace cleanup and parameter reorganization improves readability and maintains consistency throughout the documentation.
Also applies to: 21-21, 23-23, 26-26, 142-144
tests/ServiceMapTest.php (2)
15-15: LGTM! Appropriate import for test assertions.The import of
yii\web\Usersupports the new test assertions for User component resolution.
115-119: LGTM! Correct test assertion for User component.The test properly validates that the
customUsercomponent resolves toUser::classas expected.stubs/User.stub (1)
1-11: LGTM! Well-structured generic User stub.The stub correctly implements the generic type pattern with
@template T of IdentityInterfaceand properly declares theidentityproperty as nullable?T. This enables precise typing when the actual identity class is configured.tests/fixture/config.php (2)
5-7: LGTM! Proper imports for the new User component functionality.The imports correctly add the necessary classes for testing User components with custom identity classes.
18-22: LGTM! Comprehensive test configuration for User components.The configuration properly defines both array-based and object-instantiated User components with the
MyUseridentity class. This provides good coverage for testing different component initialization patterns.src/reflection/UserPropertiesClassReflectionExtension.php (2)
74-76: LGTM! Proper delegation to annotation-based property reflection.The simplified approach correctly delegates the
identityproperty resolution to the annotations extension, which will use the generic type information from the User stub.
107-108: LGTM! Streamlined property existence check.The method correctly delegates to native and annotation-based property checks, which aligns with the simplified reflection approach.
src/reflection/ApplicationPropertiesClassReflectionExtension.php (2)
98-107: LGTM! Special handling for User components is well-implemented.The logic correctly retrieves the
identityClassfrom the service map and creates aGenericObjectTypefor theUsercomponent, which aligns perfectly with the PR objective of improving type support for theyii\web\Usercomponent'sidentityproperty.
16-20: Clean use of import aliases.Using explicit aliases for Yii base classes improves code readability and makes the class references consistent throughout the file.
src/ServiceMap.php (3)
139-154: LGTM! Clean getter implementation.The method provides a simple and clean way to retrieve the full component definition. The return type annotation is accurate and the implementation is straightforward.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 151-151: src/ServiceMap.php#L151
Added line #L151 was not covered by tests
304-311: Good documentation update reflecting new functionality.The PHPDoc comment accurately describes the enhanced functionality for processing both component definitions and user-component identity class definitions.
121-137:✅ Verification successful
Good implementation, but missing test coverage.
The method correctly handles both object instances and array definitions with a 'class' key. The logic is sound and follows defensive programming practices with appropriate null checks.
However, I noticed that lines 136 in this method lack test coverage. Consider adding unit tests to cover all branches of this method.
🏁 Script executed:
#!/bin/bash # Check test coverage for getComponentClassById method rg -A 10 "getComponentClassById" tests/Length of output: 2371
🏁 Script executed:
#!/bin/bash # Display the top section of the ServiceMapTest to review component definitions sed -n '1,200p' tests/ServiceMapTest.phpLength of output: 8101
🏁 Script executed:
#!/bin/bash # Display fixture configuration to inspect component definitions sed -n '1,200p' tests/fixture/config.phpLength of output: 1640
getComponentClassById is fully covered by existing tests
ServiceMapTest.php’s testItLoadsServicesAndComponents already exercises every branch of
getComponentClassById:
- Null definition (
nonExistentComponent) ⇒ returns null- Object definitions (
customInitializedComponent,customInitializedUser) ⇒get_class()- Array definitions with
'class'key (customComponent,customUser) ⇒ returns the class string- Array definitions without
'class'(assetManager) ⇒ returns nullNo additional unit tests are required for lines 121–137.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 136-136: src/ServiceMap.php#L136
Added line #L136 was not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/ServiceMap.php (1)
334-334:⚠️ Potential issueFix type mismatch in component assignment.
Static analysis correctly flags a type inconsistency. The assignment uses a generic
arraytype but the property expectsarray<string, mixed>.This is the same issue flagged in previous reviews. Apply this diff to fix the type annotation on the property declaration:
- * @phpstan-var array<string,array<string,mixed>> + * @phpstan-var array<string,array<array-key,mixed>>This change aligns the property type with the actual array structure being assigned while maintaining type safety.
🧰 Tools
🪛 PHPStan (2.1.15)
334-334: Property yii2\extensions\phpstan\ServiceMap::$components (array<string, array<string, mixed>>) does not accept array<string, array>.
(assign.propertyType)
🪛 GitHub Check: phpstan / PHP 8.4-ubuntu-latest
[failure] 334-334:
Property yii2\extensions\phpstan\ServiceMap::$components (array<string, array<string, mixed>>) does not accept array<string, array>.
🧹 Nitpick comments (1)
src/ServiceMap.php (1)
160-173: Method implementation is correct but lacks test coverage.The
getComponentPropertyByIdmethod correctly retrieves property values from component definitions. The implementation handles the null cases appropriately and uses array access which is safe for the array-based component definitions.Note: The static analysis hints indicate this method lacks test coverage, which should be addressed in the test suite.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 160-160: src/ServiceMap.php#L160
Added line #L160 was not covered by tests
[warning] 162-162: src/ServiceMap.php#L162
Added line #L162 was not covered by tests
[warning] 164-165: src/ServiceMap.php#L164-L165
Added lines #L164 - L165 were not covered by tests
[warning] 168-169: src/ServiceMap.php#L168-L169
Added lines #L168 - L169 were not covered by tests
[warning] 172-172: src/ServiceMap.php#L172
Added line #L172 was not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/ServiceMap.php(5 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
src/ServiceMap.php
[warning] 132-132: src/ServiceMap.php#L132
Added line #L132 was not covered by tests
[warning] 147-147: src/ServiceMap.php#L147
Added line #L147 was not covered by tests
[warning] 160-160: src/ServiceMap.php#L160
Added line #L160 was not covered by tests
[warning] 162-162: src/ServiceMap.php#L162
Added line #L162 was not covered by tests
[warning] 164-165: src/ServiceMap.php#L164-L165
Added lines #L164 - L165 were not covered by tests
[warning] 168-169: src/ServiceMap.php#L168-L169
Added lines #L168 - L169 were not covered by tests
[warning] 172-172: src/ServiceMap.php#L172
Added line #L172 was not covered by tests
🪛 PHPStan (2.1.15)
src/ServiceMap.php
334-334: Property yii2\extensions\phpstan\ServiceMap::$components (array<string, array<string, mixed>>) does not accept array<string, array>.
(assign.propertyType)
🪛 GitHub Check: phpstan / PHP 8.4-ubuntu-latest
src/ServiceMap.php
[failure] 334-334:
Property yii2\extensions\phpstan\ServiceMap::$components (array<string, array<string, mixed>>) does not accept array<string, array>.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: phpunit / PHP 8.2-windows-latest
- GitHub Check: phpunit-compatibility / PHP 8.3-windows-latest
- GitHub Check: phpunit-compatibility / PHP 8.1-windows-latest
- GitHub Check: phpunit / PHP 8.4-windows-latest
🔇 Additional comments (4)
src/ServiceMap.php (4)
14-14: LGTM: Import addition supports new functionality.The import of
yii\web\Useris appropriate for the enhanced user component type analysis functionality being added.
122-133: LGTM: Method correctly handles new component structure.The updated
getComponentClassByIdmethod properly extracts the class name from the new component definition structure. The null checks and string validation are appropriate.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 132-132: src/ServiceMap.php#L132
Added line #L132 was not covered by tests
147-150: LGTM: New method provides access to full component definitions.The
getComponentByIdmethod correctly returns the complete component definition, enabling richer type analysis as intended by the PR objectives.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 147-147: src/ServiceMap.php#L147
Added line #L147 was not covered by tests
328-329: LGTM: Object handling preserves component properties.The modification to store object properties with
get_object_vars()and add the class name is a good approach that maintains consistency with the array-based storage while preserving all object state.
| @@ -72,7 +72,7 @@ | |||
| /** | |||
| * Component definitions map for Yii application analysis. | |||
| * | |||
| * @phpstan-var string[] | |||
| * @phpstan-var array<string,array<string,mixed>> | |||
There was a problem hiding this comment.
Fix type annotation to match actual usage.
The type annotation has been changed to remove |object but the code still processes objects on lines 328-329. This creates a mismatch between the declared type and actual usage.
Apply this diff to fix the type annotation:
- * @phpstan-var array<string,array<string,mixed>>
+ * @phpstan-var array<string,array<string,mixed>|object>Or alternatively, if objects should no longer be stored directly, the object processing logic needs to be updated.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| * @phpstan-var array<string,array<string,mixed>> | |
| * @phpstan-var array<string,array<string,mixed>|object> |
🤖 Prompt for AI Agents
In src/ServiceMap.php at line 75, the type annotation was changed to remove
'|object' but the code on lines 328-329 still processes objects, causing a
mismatch. To fix this, either revert the type annotation to include '|object' to
reflect actual usage or update the code on lines 328-329 to stop processing
objects directly, ensuring consistency between the annotation and the code
behavior.
Added support for
yii\web\UserComponents.This is achieved by using generic types in
yii\web\User(stubs for now).Need to update inline comments (PHPDocs).
Need to upstream the addition of generic type in
yii\web\Userto Yii.Before:
The Problem
As per the yii2 docs.
As of now,
Yii::$app->__user_component_name__->identityafter successful login will return\yii\web\IdentityInterface, irrespective of what is defined in$config['components']['__user_component_name__']['identityClass'].Expected Outcome
Yii::$app->__user_component_name__->identityafter successful login should return instance of class mentioned in$config['components']['__user_component_name__']['identityClass'].After:
Yii::$app->__user_component_name__->identityafter successful login will return whatever class mentioned in$config['components']['__user_component_name__']['identityClass']which should be implementingyii\web\IdentityInterfaceHow it works:
T of yii\web\IdentityInterfaceis added to theyii\web\Userclass in the class level.yii\web\IdentityInterfaceinyii\web\Useris replaced with generic typeT.yii\web\Useris accessed inyii\web\Application, a class property of generic object type with classyii\web\Userand generic typeTis set toidentityClassdefined in$config['components']['__user_component_name__']['identityClass']is returned.identityproperty ofyii\web\Useris accessed, the property reflection is derived from the PHPDoc annotation inyii\web\Useris returned.T of yii\web\IdentityInterfaceofyii\web\Useris set to the$config['components']['__user_component_name__']['identityClass']earlier.Summary by CodeRabbit
New Features
identityproperty.Bug Fixes
Documentation
Tests
Chores