fix: Enhance property reflection in UserPropertiesClassReflectionExtension to support identity class resolution and improve type inference for user component properties.
#34
Conversation
…ension` to support identity class resolution and improve type inference for user component properties.
|
""" WalkthroughThe updates introduce enhanced handling and testing of Yii user component property reflection and configuration. Service map functionality is extended to provide raw component definitions by ID or class. The reflection extension now supports multiple user properties and improved type inference. Test fixtures and stubs are updated to support these changes, and relevant documentation is adjusted. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Case
participant ServiceMap as ServiceMap
participant Reflection as UserPropertiesClassReflectionExtension
Test->>ServiceMap: getComponentDefinitionById('user')
ServiceMap-->>Test: returns component config array
Test->>ServiceMap: getComponentDefinitionByClassName('yii\\web\\User')
ServiceMap-->>Test: returns component config array
Reflection->>ServiceMap: getComponentDefinitionById('user')
ServiceMap-->>Reflection: returns user component config
Reflection->>Reflection: getIdentityClass() extracts 'identityClass'
Reflection-->>Test: returns property reflection with inferred type
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (18)
✨ 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 ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #34 +/- ##
===========================================
Coverage 100.00% 100.00%
- Complexity 53 60 +7
===========================================
Files 1 1
Lines 79 91 +12
===========================================
+ Hits 79 91 +12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
terabytesoftw
changed the title
UserPropertiesClassReflectionExtension to support identity class resolution and improve type inference for user component properties.UserPropertiesClassReflectionExtension to support identity class resolution and improve type inference for user component properties.
![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
🧹 Nitpick comments (3)
CHANGELOG.md (1)
9-9: Minor style improvement: Use hyphen in compound adjective.Consider using "bleeding-edge" instead of "bleeding edge" when it modifies the following noun.
-- Enh #29: Enable strict rules and bleeding edge analysis, and update `README.md` with strict configuration examples (@terabytesoftw) +- Enh #29: Enable strict rules and bleeding-edge analysis, and update `README.md` with strict configuration examples (@terabytesoftw)🧰 Tools
🪛 LanguageTool
[uncategorized] ~9-~9: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...ftw) - Enh #29: Enable strict rules and bleeding edge analysis, and updateREADME.mdwith s...(EN_COMPOUND_ADJECTIVE_INTERNAL)
src/ServiceMap.php (2)
79-85: Docblock type too broad – tighten to nested array.
$componentsDefinitionsalways stores an array for each component ID, never a scalar.
Refine the PHPStan type to improve inference.- * @phpstan-var array<string, mixed> + * @phpstan-var array<string, array<array-key, mixed>>
149-155: Minor: redundantis_array()guard.
$componentsDefinitions[$id]is written exclusively with an array value inprocessComponents; the extrais_array()check is unnecessary noise.Not critical – consider during clean-up.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
CHANGELOG.md(1 hunks)src/ServiceMap.php(7 hunks)src/reflection/UserPropertiesClassReflectionExtension.php(4 hunks)tests/ServiceMapTest.php(2 hunks)tests/fixture/config.php(2 hunks)tests/stub/MyController.php(1 hunks)tests/stub/User.php(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/fixture/config.php (1)
tests/stub/User.php (1)
User(10-36)
🪛 LanguageTool
CHANGELOG.md
[uncategorized] ~9-~9: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...ftw) - Enh #29: Enable strict rules and bleeding edge analysis, and update README.md with s...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: phpunit-compatibility / PHP 8.3-windows-latest
- GitHub Check: phpunit-compatibility / PHP 8.1-windows-latest
- GitHub Check: phpunit-compatibility / PHP 8.2-ubuntu-latest
- GitHub Check: phpunit-compatibility / PHP 8.4-windows-latest
🔇 Additional comments (7)
tests/stub/MyController.php (1)
94-94: LGTM! Good test coverage for the new user property reflection.The addition of
$id = Yii::$app->user->id;appropriately tests the enhanced property reflection for the user component's ID property, complementing the existing user component usage patterns in the test controller.tests/fixture/config.php (2)
6-6: LGTM! Proper import for the User stub class.The import correctly references the User stub class needed for the identity class configuration.
17-20: LGTM! Well-structured user component configuration.The user component configuration correctly sets up the identity class that enables the enhanced property reflection functionality. The configuration follows Yii2 conventions and properly references the stub User class.
CHANGELOG.md (2)
7-7: LGTM! Correct categorization of bug fixes.The changes from "Enh" to "Bug" properly categorize these entries as bug fixes rather than enhancements.
Also applies to: 10-10
11-11: LGTM! Accurate documentation of the property reflection enhancement.The changelog entry clearly describes the UserPropertiesClassReflectionExtension improvements for identity class resolution and type inference.
tests/ServiceMapTest.php (2)
15-15: LGTM! Appropriate import for User class.The import correctly references the User class needed for the component definition tests.
115-132: LGTM! Comprehensive test coverage for component definition retrieval.The new assertions thoroughly test the component definition functionality:
- Positive case: Retrieving definition for existing
yii\web\Userclass- Negative case: Handling non-existent component class
- ID-based retrieval: Getting definition by component ID 'user'
The expected values correctly match the configuration in
tests/fixture/config.php, ensuring the ServiceMap properly exposes component definitions includingidentityClass.
…ssReflectionExtension` and update `User` class method signatures to remove type hints.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/reflection/UserPropertiesClassReflectionExtension.php (3)
143-143:⚠️ Potential issueMethod call to non-existent
isSubclassOfClass().This issue was flagged in previous reviews but remains unresolved.
ClassReflectionexposesisSubclassOf(string $className), notisSubclassOfClass(). This will cause a fatal error.Apply this diff to fix the method call:
-$classReflection->isSubclassOfClass($this->reflectionProvider->getClass(User::class)) === false +$classReflection->isSubclassOf(User::class) === false
148-151:⚠️ Potential issue
hasProperty()returns true for any name when identity class exists.This issue was flagged in previous reviews but remains unresolved. The current logic ignores
$propertyNameand returnstruefor any property name as long as anidentityClassis configured, causinggetProperty()to later throwMissingPropertyFromReflectionException.Apply this diff to properly check property names:
-return - $this->getIdentityClass() !== null || - $this->serviceMap->getComponentClassById($propertyName) !== null; +return in_array($propertyName, ['id', 'identity', 'isGuest'], true) + ? $this->getIdentityClass() !== null + : $this->serviceMap->getComponentClassById($propertyName) !== null;
85-119: 🛠️ Refactor suggestionPotential type inconsistency for
idproperty - missing null possibility.The
idproperty is typed asIntegerType | StringTypebut according to Yii'sIdentityInterface::getId(), it can also returnnull. This could cause type errors in static analysis when the user is not authenticated.Apply this diff to include null in the union type:
if ($propertyName === 'id') { return new ComponentPropertyReflection( new DummyPropertyReflection($propertyName), - TypeCombinator::union(new IntegerType(), new StringType()), + TypeCombinator::addNull(TypeCombinator::union(new IntegerType(), new StringType())), $classReflection, ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/reflection/UserPropertiesClassReflectionExtension.php(4 hunks)tests/stub/User.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/stub/User.php
⏰ Context from checks skipped due to timeout of 90000ms (18)
- GitHub Check: phpstan / PHP 8.4-ubuntu-latest
- GitHub Check: composer-require-checker / PHP 8.4-ubuntu-latest
- GitHub Check: phpunit-compatibility / PHP 8.2-ubuntu-latest
- GitHub Check: phpunit-compatibility / PHP 8.3-windows-latest
- GitHub Check: phpunit-compatibility / PHP 8.1-windows-latest
- GitHub Check: phpunit-compatibility / PHP 8.2-windows-latest
- GitHub Check: phpunit-compatibility / PHP 8.4-windows-latest
- GitHub Check: phpunit / PHP 8.4-windows-latest
- GitHub Check: phpunit-compatibility / PHP 8.3-ubuntu-latest
- GitHub Check: phpunit / PHP 8.1-windows-latest
- GitHub Check: phpunit / PHP 8.1-ubuntu-latest
- GitHub Check: phpunit / PHP 8.3-windows-latest
- GitHub Check: phpunit-compatibility / PHP 8.3-windows-latest
- GitHub Check: phpunit / PHP 8.3-windows-latest
- GitHub Check: phpunit-compatibility / PHP 8.2-windows-latest
- GitHub Check: phpunit-compatibility / PHP 8.1-windows-latest
- GitHub Check: phpunit / PHP 8.2-windows-latest
- GitHub Check: phpunit / PHP 8.1-windows-latest
🔇 Additional comments (3)
src/reflection/UserPropertiesClassReflectionExtension.php (3)
161-172: Approve the newgetIdentityClass()method implementation.The implementation correctly extracts the identity class from the user component configuration and provides appropriate null handling. The method encapsulates the logic well and improves code readability.
16-22: Approve the additional type imports.The new type imports (
BooleanType,IntegerType,ObjectType,StringType,TypeCombinator) are correctly added to support the enhanced property type inference functionality.
64-64: Approve theReflectionProviderconstructor parameter addition.The addition of
ReflectionProviderto the constructor is necessary for the enhanced subclass checking functionality and follows proper dependency injection patterns.
… to use `ConstantBooleanType` for 'isGuest' property.
…Extension` for improved readability and maintainability.
…ionExtension` for improved clarity and performance.
There was a problem hiding this comment.
Pull Request Overview
This PR fixes property reflection issues in the UserPropertiesClassReflectionExtension by enhancing the handling of user identity-related properties, such as "id", "identity", and "isGuest". It updates test stubs and component definitions to verify accurate type inference and reflection.
- Updated test stubs and configuration to support user identity resolution.
- Refined UserPropertiesClassReflectionExtension to handle special properties based on identityClass.
- Adjusted ServiceMap to properly extract and store component definitions.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/stub/User.php | Introduces a stub for a User model implementing IdentityInterface. |
| tests/stub/MyController.php | Adds an assignment for user id; consider checking its usage. |
| tests/fixture/config.php | Adds user component configuration with identityClass resolution. |
| tests/ServiceMapTest.php | Adds tests to validate the resolution of component definitions. |
| src/reflection/UserPropertiesClassReflectionExtension.php | Enhances property reflection for special user component properties. |
| src/ServiceMap.php | Updates component definitions handling for improved static analysis. |
| CHANGELOG.md | Updates changelog to reflect fixes and improvements in user property reflection. |
Comments suppressed due to low confidence (1)
src/reflection/UserPropertiesClassReflectionExtension.php:143
- The hasProperty method determines the existence of 'id' and 'isGuest' based solely on getIdentityClass() being non-null, yet getProperty always returns a reflection for these properties. Consider updating hasProperty to unconditionally return true for 'id' and 'isGuest' to ensure consistent behavior.
return in_array($propertyName, ['id', 'identity', 'isGuest'], true) ? $this->getIdentityClass() !== null : $this->serviceMap->getComponentClassById($propertyName) !== null;
Summary by CodeRabbit