Add support for readonly class in PHP 8.2 #236
Conversation
Reviewer's Guide by SourceryThis pull request refactors the mechanism for interceptor state management by replacing direct use of the 'bindings' property with an encapsulating 'state' object. It adds support for PHP 8.2 readonly classes by introducing a new trait with readonly properties, updates related interface and tests, and adjusts code paths based on the PHP version. No diagrams generated as the changes look simple and do not need a visual representation. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Rate limit exceeded@koriym has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 17 minutes and 32 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (9)
WalkthroughThis update introduces new interception mechanisms in the codebase to support PHP 8.2's readonly properties. The changes add new traits, update existing ones, and refactor the control flow for method interception. A new state class encapsulates the bindings and aspect status, and the instantiation logic for weaved classes now uses a dedicated method to initialize state based on interface support. Various tests and mock classes have been updated or added to verify these changes across different scenarios and PHP versions. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant Trait as Php82InterceptTrait
participant Parent as ParentClassMethod
participant Invocation as Invocation
Caller->>Trait: Call intercepted method
Trait-->>Trait: Check $_state->isAspect
alt Not in aspect context
Trait->>Parent: Invoke parent's method via call_user_func_array
else Already in aspect
Trait->>Trait: Set $_state->isAspect to false
Trait->>Invocation: Create new Invocation(context)
Invocation->>Trait: Proceed with method execution
Trait->>Trait: Reset $_state->isAspect to true
end
Trait-->>Caller: Return method result
sequenceDiagram
participant Weaver as Weaver/Compiler
participant Instance as New Object
participant Weaved as WeavedInterface
Weaver->>Instance: Create new instance
alt Instance implements WeavedInterface
Weaver->>Instance: _initState(bindings)
else
Weaver-->>Instance: Return instance directly
end
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 @@
## 2.x #236 +/- ##
=======================================
Coverage ? 100.00%
Complexity ? 233
=======================================
Files ? 29
Lines ? 603
Branches ? 0
=======================================
Hits ? 603
Misses ? 0
Partials ? 0 ☔ View full report in Codecov by Sentry. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @ngmy - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider using constructor property promotion in
InterceptTraitState. - The conditional logic for PHP 8.2 in
AopCode::addIntercepterTrait()could be simplified by extracting it to a separate method.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![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 (7)
src/InterceptTrait.php (2)
14-19: Prefer restricting property visibility if possible.
Thepublic $state;property is declared public primarily for testing. Although the doc comment indicates it's “public for CompilerTest,” this visibility might increase the risk of unintended external access. Consider reducing visibility to private or protected if feasible, or providing a dedicated getter method to maintain encapsulation.
39-48: Consider validating$funcin bindings to avoid potential undefined index.
When_interceptexecutes,$funcis used as a key for$this->state->bindings. If$funcis missing or misspelled, an undefined index error could occur. Consider gracefully handling this case (e.g., by checkingisset($this->state->bindings[$func])).src-php8/Php82InterceptTrait.php (1)
32-53: Duplicate logic withInterceptTraitcould be extracted for DRY compliance.
This_interceptmethod closely mirrors the logic inInterceptTrait. While the duplication is presumably necessary for PHP 8.2's readonly constraints, consider extracting common logic in the future to reduce duplication and maintenance costs.tests/tmp_unerase/Ray_Aop_FakeWeaverMock_523567342.php (2)
14-17: Fix indentation.The indentation of the
returnSamemethod is inconsistent with other methods.- public function returnSame($a) - { + public function returnSame($a) + {
27-30: Add docblocks for consistency.The
returnValueandgetPrivateValmethods are missing docblocks, unlike other methods.+ /** + * doc comment of returnValue + */ public function returnValue(null|\Ray\Aop\FakeNum $num = NULL) { return $this->_intercept(__FUNCTION__, func_get_args()); } + /** + * doc comment of getPrivateVal + */ public function getPrivateVal() { return $this->_intercept(__FUNCTION__, func_get_args()); }Also applies to: 32-35
src/AopCode.php (1)
206-210: Simplify the conditional logic and document the version check.The code can be simplified by removing the else clause. Also, consider documenting why PHP 8.2 requires a different trait.
Apply this diff to simplify the code:
- if (PHP_VERSION_ID >= 80200) { - $this->add(sprintf("{\n use \%s;\n}\n", Php82InterceptTrait::class)); - } else { - $this->add(sprintf("{\n use \%s;\n}\n", InterceptTrait::class)); - } + $trait = PHP_VERSION_ID >= 80200 ? Php82InterceptTrait::class : InterceptTrait::class; + $this->add(sprintf("{\n use \%s;\n}\n", $trait));Add a comment explaining the version check:
+ // PHP 8.2 introduced readonly classes which require special handling through Php82InterceptTrait $trait = PHP_VERSION_ID >= 80200 ? Php82InterceptTrait::class : InterceptTrait::class;🧰 Tools
🪛 GitHub Actions: Static Analysis
[warning] 208-208: The method addIntercepterTrait uses an else expression. Else clauses are basically not necessary and you can simplify the code by not using them.
tests/CompilerTest.php (1)
354-360: Consider adding more test cases for PHP 8.2 readonly class.While the basic test case is good, consider adding more test cases to verify:
- State initialization with different binding configurations
- Interaction with interceptors
- Error cases with readonly violations
Would you like me to help generate additional test cases?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src-php8/Php82InterceptTrait.php(1 hunks)src/AopCode.php(2 hunks)src/Compiler.php(1 hunks)src/InterceptTrait.php(2 hunks)src/InterceptTraitState.php(1 hunks)src/WeavedInterface.php(1 hunks)src/Weaver.php(1 hunks)tests/CompilerTest.php(5 hunks)tests/Fake/FakePhp82ReadOnlyClass.php(1 hunks)tests/Fake/FakeWeavedClass.php(1 hunks)tests/tmp_unerase/Ray_Aop_FakeWeaverMock_523567342.php(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/Fake/FakePhp82ReadOnlyClass.php
🧰 Additional context used
🪛 PHPStan (2.0.3)
src/Weaver.php
62-62: PHPDoc tag @var with type T of object is not subtype of native type mixed.
(varTag.nativeType)
tests/CompilerTest.php
82-82: Cannot access property $bindings on mixed.
(property.nonObject)
121-121: Cannot access property $bindings on mixed.
(property.nonObject)
132-132: Cannot access property $bindings on mixed.
(property.nonObject)
142-142: Cannot access property $bindings on mixed.
(property.nonObject)
🪛 GitHub Actions: Static Analysis
src/AopCode.php
[warning] 208-208: The method addIntercepterTrait uses an else expression. Else clauses are basically not necessary and you can simplify the code by not using them.
🔇 Additional comments (7)
src/InterceptTrait.php (1)
21-28: Consistent initialization of trait state.
The newinitStatemethod aligns well withWeavedInterface::initState(). It properly encapsulates the creation ofInterceptTraitState. No immediate issues.src-php8/Php82InterceptTrait.php (2)
1-13: File introduction for PHP 8.2 support.
These lines introduce a dedicated trait that supports readonly properties, aligning with PHP 8.2 features. The overall structure and namespace usage are correct.
16-30: Readonly state property and state initialization.
Declaring$stateaspublic readonlyis suitable for usage in a readonly class. Assigning a newInterceptTraitStateobject here is permitted once, which satisfies the typical usage pattern.src/WeavedInterface.php (1)
9-10: New method contract for initializing weaved state.
DeclaringinitStateat the interface level ensures consistent state initialization across weaved classes. The doc block clarifies the expected structure of$bindings, promoting type safety and clarity.tests/Fake/FakeWeavedClass.php (1)
9-12: LGTM!The empty implementation is appropriate for a test class, and the method signature correctly implements the
WeavedInterfacecontract.src/Weaver.php (1)
65-66: LGTM!The state initialization through
initStatemethod is correctly implemented, maintaining consistency with the PHP 8.2 readonly class support.src/Compiler.php (1)
67-69: LGTM! Clean implementation of state initialization.The changes properly handle state initialization through the
WeavedInterface, which is a good approach for supporting readonly classes in PHP 8.2.
ngmy
force-pushed
the
support-readonly-class
branch
from
fe16772 to
2700cd4
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/InterceptTrait.php (1)
20-28: Add validation and prevent multiple initialization.The
initStatemethod could benefit from additional safeguards:
- Prevent multiple initialization
- Validate array structure
Consider this improvement:
public function initState(array $bindings): void { + if ($this->state !== null) { + throw new \RuntimeException('State already initialized'); + } + foreach ($bindings as $method => $interceptors) { + if (!is_string($method) || !is_array($interceptors)) { + throw new \InvalidArgumentException('Invalid bindings structure'); + } + } $this->state = new InterceptTraitState($bindings, true); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src-php8/Php82InterceptTrait.php(1 hunks)src/AopCode.php(2 hunks)src/Compiler.php(1 hunks)src/InterceptTrait.php(2 hunks)src/InterceptTraitState.php(1 hunks)src/WeavedInterface.php(1 hunks)src/Weaver.php(1 hunks)tests/CompilerTest.php(5 hunks)tests/Fake/FakePhp82ReadOnlyClass.php(1 hunks)tests/Fake/FakeWeavedClass.php(1 hunks)tests/tmp_unerase/Ray_Aop_FakeWeaverMock_523567342.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- tests/Fake/FakeWeavedClass.php
- src/WeavedInterface.php
- tests/Fake/FakePhp82ReadOnlyClass.php
- src/InterceptTraitState.php
- src/AopCode.php
- src/Compiler.php
- tests/tmp_unerase/Ray_Aop_FakeWeaverMock_523567342.php
- src-php8/Php82InterceptTrait.php
🧰 Additional context used
🪛 PHPStan (2.0.3)
src/Weaver.php
62-62: PHPDoc tag @var with type T of object is not subtype of native type mixed.
(varTag.nativeType)
tests/CompilerTest.php
82-82: Cannot access property $bindings on mixed.
(property.nonObject)
121-121: Cannot access property $bindings on mixed.
(property.nonObject)
132-132: Cannot access property $bindings on mixed.
(property.nonObject)
142-142: Cannot access property $bindings on mixed.
(property.nonObject)
🔇 Additional comments (4)
tests/CompilerTest.php (2)
81-82: Add type assertions for state property access.Static analysis indicates potential type safety issues when accessing the state property. Add proper type assertions to ensure type safety.
Apply this pattern to all state property access:
assert(property_exists($weaved, 'state')); + assert(property_exists($weaved->state, 'bindings')); + /** @var object{state: object{bindings: mixed}} $weaved */ $weaved->state->bindings = $this->bind->getBindings();Also applies to: 120-121, 131-132, 141-142
🧰 Tools
🪛 PHPStan (2.0.3)
82-82: Cannot access property $bindings on mixed.
(property.nonObject)
354-360: LGTM! Well-structured test for PHP 8.2 readonly class support.The test method is properly annotated with PHP version requirement and includes appropriate assertions to verify readonly class instantiation and interface implementation.
src/Weaver.php (2)
60-63: Fix type variance in PHPDoc.The PHPDoc tag @var with type T is not a subtype of native type mixed.
Apply this diff to fix the type variance:
if (! $instance instanceof WeavedInterface) { - /** @var T $instance */ + /** @var T&object $instance */ return $instance; }🧰 Tools
🪛 PHPStan (2.0.3)
62-62: PHPDoc tag @var with type T of object is not subtype of native type mixed.
(varTag.nativeType)
65-65: LGTM! Good approach for readonly class support.The change from direct property assignment to method-based state initialization is a good approach for supporting PHP 8.2 readonly classes. The code maintains good practices with dependency injection and type safety through interfaces.
ngmy
changed the title
Replaced verbose array annotations with the `MethodBindings` type imported via Psalm across interfaces and traits. This improves code readability and ensures type consistency throughout the project.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/InterceptTrait.php (1)
38-51:⚠️ Potential issueAdd error handling for state and bindings.
The
_interceptmethod should validate state and bindings before proceeding.private function _intercept(string $func, array $args) // phpcs:ignore { + if ($this->state === null) { + throw new \RuntimeException('State not initialized'); + } + if (!isset($this->state->bindings[$func])) { + throw new \InvalidArgumentException(sprintf('No bindings found for method "%s"', $func)); + } if (! $this->state->isAspect) { $this->state->isAspect = true;
🧹 Nitpick comments (1)
src/WeavedInterface.php (1)
10-11: Consider renaming method to follow PHP naming conventions.The method name
_initStateuses an underscore prefix, which is discouraged in PHP. Consider renaming it toinitStateto follow PHP naming conventions.- public function _initState(array $bindings): void; + public function initState(array $bindings): void;🧰 Tools
🪛 GitHub Check: cs / Coding Standards
[failure] 11-11:
Method name "_initState" must not be prefixed with an underscore to indicate visibility🪛 GitHub Actions: Static Analysis
[warning] 11-11: The method _initState is not named in camelCase.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src-php8/Php82InterceptTrait.php(1 hunks)src/BindInterface.php(2 hunks)src/Compiler.php(1 hunks)src/InterceptTrait.php(2 hunks)src/WeavedInterface.php(1 hunks)src/Weaver.php(1 hunks)tests/Fake/FakeWeavedClass.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/Fake/FakeWeavedClass.php
- src/Compiler.php
🧰 Additional context used
🪛 PHPStan (2.0.3)
src/InterceptTrait.php
12-12: Trait Ray\Aop\InterceptTrait is used zero times and is not analysed.
See: https://phpstan.org/blog/how-phpstan-analyses-traits
(trait.unused)
src/Weaver.php
62-62: PHPDoc tag @var with type T of object is not subtype of native type mixed.
(varTag.nativeType)
🪛 GitHub Check: cs / Coding Standards
src/InterceptTrait.php
[failure] 26-26:
Method name "_initState" must not be prefixed with an underscore to indicate visibility
src/WeavedInterface.php
[failure] 11-11:
Method name "_initState" must not be prefixed with an underscore to indicate visibility
🪛 GitHub Actions: Static Analysis
src/InterceptTrait.php
[warning] 26-26: The method _initState is not named in camelCase.
src/WeavedInterface.php
[warning] 11-11: The method _initState is not named in camelCase.
🔇 Additional comments (6)
src/WeavedInterface.php (1)
7-12: LGTM! Well-structured interface with proper type imports.The interface is well-defined with proper type imports and clear method signature.
🧰 Tools
🪛 GitHub Check: cs / Coding Standards
[failure] 11-11:
Method name "_initState" must not be prefixed with an underscore to indicate visibility🪛 GitHub Actions: Static Analysis
[warning] 11-11: The method _initState is not named in camelCase.
src/BindInterface.php (1)
7-36: LGTM! Enhanced type safety with proper imports.The changes improve type safety by:
- Adding proper type imports
- Using specific return type for
getBindings()src/InterceptTrait.php (1)
14-19: Consider improving property encapsulation.The
$stateproperty should be better encapsulated.- /** - * @var InterceptTraitState - * @readonly - * @internal Public for CompilerTest - */ - public $state; + /** + * @var InterceptTraitState + * @internal Public for CompilerTest + */ + private $state; + + public function getState(): InterceptTraitState + { + return $this->state; + }src-php8/Php82InterceptTrait.php (1)
11-21: LGTM! Well-implemented readonly support.The trait properly implements readonly support for PHP 8.2:
- Clear warning about readonly requirements
- Properly typed readonly property
- Consistent with the base trait's functionality
src/Weaver.php (2)
60-63: Fix type variance in PHPDoc.The PHPStan warning about type variance is valid. The
@varannotation should be updated to match the native type.if (! $instance instanceof WeavedInterface) { - /** @var T $instance */ + /** @var T&object $instance */ return $instance; }🧰 Tools
🪛 PHPStan (2.0.3)
62-62: PHPDoc tag @var with type T of object is not subtype of native type mixed.
(varTag.nativeType)
65-65: LGTM! Good approach for readonly class support.The change from property assignment to method invocation is a good approach for supporting readonly classes in PHP 8.2. The code is properly guarded by the WeavedInterface check.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/InterceptTrait.php (1)
41-49:⚠️ Potential issueAdd error handling and null checks.
The method still needs additional error handling.
Apply this diff:
private function _intercept(string $func, array $args) // phpcs:ignore { + if ($this->state === null) { + throw new \RuntimeException('State not initialized'); + } + if (!isset($this->state->bindings[$func])) { + throw new \InvalidArgumentException(sprintf('No bindings found for method "%s"', $func)); + } if (! $this->state->isAspect) { $this->state->isAspect = true; return call_user_func_array([parent::class, $func], $args); } $this->state->isAspect = false; $result = (new Invocation($this, $func, $args, $this->state->bindings[$func]))->proceed(); $this->state->isAspect = true; return $result; }
🧹 Nitpick comments (1)
src/InterceptTrait.php (1)
27-30: Add input validation.The method should validate the input array to ensure it contains valid bindings.
Apply this diff:
public function _initState(array $bindings): void { + foreach ($bindings as $method => $interceptors) { + if (!is_string($method) || !is_array($interceptors)) { + throw new \InvalidArgumentException('Invalid bindings format'); + } + } $this->state = new InterceptTraitState($bindings, true); }🧰 Tools
🪛 GitHub Check: cs / Coding Standards
[failure] 27-27:
Method name "_initState" must not be prefixed with an underscore to indicate visibility
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/InterceptTrait.php(2 hunks)src/WeavedInterface.php(1 hunks)
🧰 Additional context used
🪛 PHPStan (2.0.3)
src/WeavedInterface.php
13-13: PHPDoc tag @SuppressWarnings has invalid value ((PHPMD.CamelCaseMethodName)): Unexpected token ".CamelCaseMethodName)", expected ')' at offset 80 on line 4
(phpDoc.parseError)
src/InterceptTrait.php
12-12: Trait Ray\Aop\InterceptTrait is used zero times and is not analysed.
See: https://phpstan.org/blog/how-phpstan-analyses-traits
(trait.unused)
🪛 GitHub Check: cs / Coding Standards
src/WeavedInterface.php
[failure] 15-15:
Method name "_initState" must not be prefixed with an underscore to indicate visibility
src/InterceptTrait.php
[failure] 27-27:
Method name "_initState" must not be prefixed with an underscore to indicate visibility
🔇 Additional comments (3)
src/WeavedInterface.php (1)
7-7: LGTM!The type import is correctly defined and follows PHP best practices.
src/InterceptTrait.php (2)
11-12: LGTM!The Psalm import type annotation is correctly formatted and helps with static type checking.
🧰 Tools
🪛 PHPStan (2.0.3)
12-12: Trait Ray\Aop\InterceptTrait is used zero times and is not analysed.
See: https://phpstan.org/blog/how-phpstan-analyses-traits(trait.unused)
14-19: Consider improving property encapsulation using PHP 8.2 features.Since this PR adds PHP 8.2 support, consider these improvements:
- Use the
readonlykeyword instead of PHPDoc annotation- Make the property private and provide a getter
Apply this diff:
- /** - * @var InterceptTraitState - * @readonly - * @internal Public for CompilerTest - */ - public $state; + /** + * @var InterceptTraitState + * @internal Public for CompilerTest + */ + private readonly InterceptTraitState $state; + + public function getState(): InterceptTraitState + { + return $this->state; + }
|
@sourcery-ai guide in Japanese |
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/WeavedInterface.php (1)
10-13:⚠️ Potential issueFix the @SuppressWarnings syntax.
The @SuppressWarnings syntax is incorrect. According to PHPMD documentation, the correct syntax should use quotes.
Apply this diff to fix the syntax:
- * @SuppressWarnings(PHPMD.CamelCaseMethodName) + * @SuppressWarnings("PHPMD.CamelCaseMethodName")🧰 Tools
🪛 PHPStan (2.0.3)
13-13: PHPDoc tag @SuppressWarnings has invalid value ((PHPMD.CamelCaseMethodName)): Unexpected token ".CamelCaseMethodName)", expected ')' at offset 80 on line 4
(phpDoc.parseError)
🧹 Nitpick comments (2)
tests/Fake/FakeMockChild.php (1)
9-9: Add type information for the state property.The property should be typed to ensure type safety and improve code maintainability.
Apply this diff:
- public $state; + public InterceptTraitState $state;src/InterceptTrait.php (1)
14-19: Consider using property promotion for better encapsulation.Since Ray.Aop maintains compatibility with PHP 7.2+:
- The
@readonlyannotation in docblock cannot be enforced.- The property is marked as
@internalbut is public.Consider using property promotion to improve encapsulation while maintaining backward compatibility.
Apply this diff:
- /** - * @var InterceptTraitState - * @readonly - * @internal Public for CompilerTest - */ - public $state; + /** @var InterceptTraitState */ + private $state; + + /** @internal */ + public function getState(): InterceptTraitState + { + return $this->state; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/InterceptTrait.php(2 hunks)src/WeavedInterface.php(1 hunks)tests/Fake/FakeMockChild.php(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: koriym
PR: ray-di/Ray.Aop#236
File: src/InterceptTraitState.php:9-13
Timestamp: 2025-02-13T15:39:57.211Z
Learning: The Ray.Aop package maintains compatibility with PHP 7.2 and higher versions. Therefore, PHP 8.2+ specific features like the native readonly modifier should not be used, and backward-compatible alternatives (like docblock annotations) should be preferred.
src/InterceptTrait.php (1)
Learnt from: koriym
PR: ray-di/Ray.Aop#236
File: src/InterceptTrait.php:39-52
Timestamp: 2025-02-13T15:35:13.091Z
Learning: The `_intercept` method in Ray.Aop is a special AOP method where robustness checks are intentionally omitted to prioritize simplicity and performance. The method is designed to be minimal and fast, following AOP principles.
🪛 PHPStan (2.0.3)
src/InterceptTrait.php
12-12: Trait Ray\Aop\InterceptTrait is used zero times and is not analysed.
See: https://phpstan.org/blog/how-phpstan-analyses-traits
(trait.unused)
src/WeavedInterface.php
13-13: PHPDoc tag @SuppressWarnings has invalid value ((PHPMD.CamelCaseMethodName)): Unexpected token ".CamelCaseMethodName)", expected ')' at offset 80 on line 4
(phpDoc.parseError)
🔇 Additional comments (4)
src/InterceptTrait.php (2)
21-30: LGTM!The method is well-documented and follows the project's conventions:
- Underscore prefix prevents naming conflicts.
- Type information is correctly annotated.
39-52: LGTM!The method maintains its simplicity while correctly managing the aspect state through the state object.
src/WeavedInterface.php (2)
7-7: LGTM!Good use of psalm type import for enhanced type safety.
15-15: LGTM! Good backward compatibility approach.The method signature is well-designed:
- Uses array type instead of PHP 8.2 specific features to maintain compatibility with PHP 7.2+
- The underscore prefix follows AOP framework conventions for internal methods
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/CompilerTest.php (1)
82-82:⚠️ Potential issueAdd type assertions for state property access.
Static analysis indicates potential type safety issues when accessing the
bindingsproperty. This issue was previously identified and should be addressed.Apply this pattern to all state property access:
assert(property_exists($weaved, '_state')); + assert(property_exists($weaved->_state, 'bindings')); + /** @var object{_state: object{bindings: mixed}} $weaved */ $weaved->_state->bindings = $this->bind->getBindings();Also applies to: 131-132, 141-142
🧰 Tools
🪛 PHPStan (2.0.3)
82-82: Cannot access property $bindings on mixed.
(property.nonObject)
🧹 Nitpick comments (2)
tests/CompilerTest.php (1)
353-359: Ensure backward compatibility with PHP 7.2+.While the test correctly uses
@requires PHP 8.2annotation, the retrieved learnings indicate that the package should maintain compatibility with PHP 7.2+. Consider adding a complementary test that verifies the fallback behavior for older PHP versions.Would you like me to help generate a test case that verifies backward compatibility with PHP 7.2?
src/InterceptTrait.php (1)
14-19: Document PHP version compatibility requirement.Since the package maintains compatibility with PHP 7.2+, consider adding a note in the PHPDoc to clarify that the
@readonlyannotation is for static analysis only and not the PHP 8.2+ native readonly modifier./** * @var InterceptTraitState * @readonly + * @note Using PHPDoc readonly for PHP 7.2+ compatibility * @internal Public for CompilerTest */ public $_state;🧰 Tools
🪛 GitHub Check: cs / Coding Standards
[failure] 19-19:
Public member variable "_state" must not contain a leading underscore
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src-php8/Php82InterceptTrait.php(1 hunks)src/InterceptTrait.php(2 hunks)tests/CompilerTest.php(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src-php8/Php82InterceptTrait.php
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: koriym
PR: ray-di/Ray.Aop#236
File: src/InterceptTraitState.php:9-13
Timestamp: 2025-02-13T15:39:57.211Z
Learning: The Ray.Aop package maintains compatibility with PHP 7.2 and higher versions. Therefore, PHP 8.2+ specific features like the native readonly modifier should not be used, and backward-compatible alternatives (like docblock annotations) should be preferred.
src/InterceptTrait.php (1)
Learnt from: koriym
PR: ray-di/Ray.Aop#236
File: src/InterceptTrait.php:39-52
Timestamp: 2025-02-13T15:35:13.091Z
Learning: The `_intercept` method in Ray.Aop is a special AOP method where robustness checks are intentionally omitted to prioritize simplicity and performance. The method is designed to be minimal and fast, following AOP principles.
🪛 PHPStan (2.0.3)
src/InterceptTrait.php
12-12: Trait Ray\Aop\InterceptTrait is used zero times and is not analysed.
See: https://phpstan.org/blog/how-phpstan-analyses-traits
(trait.unused)
tests/CompilerTest.php
82-82: Cannot access property $bindings on mixed.
(property.nonObject)
131-131: Cannot access property $bindings on mixed.
(property.nonObject)
141-141: Cannot access property $bindings on mixed.
(property.nonObject)
🪛 GitHub Check: cs / Coding Standards
src/InterceptTrait.php
[failure] 19-19:
Public member variable "_state" must not contain a leading underscore
🔇 Additional comments (5)
tests/CompilerTest.php (1)
81-82: LGTM! Property access changes are consistent.The changes from
stateto_stateare consistently applied across all test methods, maintaining the expected behavior while adapting to the new property naming convention.Also applies to: 120-121, 130-132, 140-142
🧰 Tools
🪛 PHPStan (2.0.3)
82-82: Cannot access property $bindings on mixed.
(property.nonObject)
src/InterceptTrait.php (4)
11-12: LGTM! Clean import and trait declaration.The Psalm import type annotation is correctly added and follows conventions.
🧰 Tools
🪛 PHPStan (2.0.3)
12-12: Trait Ray\Aop\InterceptTrait is used zero times and is not analysed.
See: https://phpstan.org/blog/how-phpstan-analyses-traits(trait.unused)
21-30: LGTM! Clean state initialization implementation.The method is well-documented and correctly implements the interface requirement.
32-52: LGTM! Maintains minimal and fast design.The changes correctly adapt the method to use the new state object while preserving its intentionally minimal design.
1-54:✅ Verification successful
Verify readonly class support.
Let's verify that these changes properly support readonly classes in PHP 8.2.
🏁 Script executed:
#!/bin/bash # Description: Check for readonly class usage and test coverage # Search for readonly class tests echo "Searching for readonly class tests..." rg -l 'readonly\s+class' tests/ # Search for readonly class implementations echo "Searching for readonly class implementations..." rg -l 'readonly\s+class' src/Length of output: 293
Readonly Class Support Verified
- The readonly class support is covered by the existing test:
tests/Fake/FakePhp82ReadOnlyClass.phpconfirms that PHP 8.2 readonly functionality is exercised.- No readonly class declarations were expected in the production code within
src/; the phpdoc-based readonly indication inInterceptTrait.phpaligns with the intended design.🧰 Tools
🪛 PHPStan (2.0.3)
12-12: Trait Ray\Aop\InterceptTrait is used zero times and is not analysed.
See: https://phpstan.org/blog/how-phpstan-analyses-traits(trait.unused)
🪛 GitHub Check: cs / Coding Standards
[failure] 19-19:
Public member variable "_state" must not contain a leading underscore
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src-php8/Php82InterceptTrait.php(1 hunks)src/InterceptTrait.php(2 hunks)tests/CompilerTest.php(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/CompilerTest.php
- src-php8/Php82InterceptTrait.php
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: koriym
PR: ray-di/Ray.Aop#236
File: src/InterceptTraitState.php:9-13
Timestamp: 2025-02-13T15:39:57.211Z
Learning: The Ray.Aop package maintains compatibility with PHP 7.2 and higher versions. Therefore, PHP 8.2+ specific features like the native readonly modifier should not be used, and backward-compatible alternatives (like docblock annotations) should be preferred.
src/InterceptTrait.php (1)
Learnt from: koriym
PR: ray-di/Ray.Aop#236
File: src/InterceptTrait.php:39-52
Timestamp: 2025-02-13T15:35:13.091Z
Learning: The `_intercept` method in Ray.Aop is a special AOP method where robustness checks are intentionally omitted to prioritize simplicity and performance. The method is designed to be minimal and fast, following AOP principles.
🪛 PHPStan (2.0.3)
src/InterceptTrait.php
12-12: Trait Ray\Aop\InterceptTrait is used zero times and is not analysed.
See: https://phpstan.org/blog/how-phpstan-analyses-traits
(trait.unused)
🔇 Additional comments (3)
src/InterceptTrait.php (3)
11-11: LGTM! Good use of Psalm type imports.The addition of the Psalm import type annotation enhances type safety and static analysis capabilities.
20-29: LGTM! Well-structured state initialization.The
_initStatemethod follows the established naming convention and properly initializes the state object.
31-51: LGTM! Clean and efficient implementation.The
_interceptmethod maintains its minimal and fast design while properly managing the aspect state through the new state object.
…he AOP class that inherits it
Add $state property to FakeMockChild class
This change updates the property name from `state` to `_state` across relevant classes and tests to reinforce encapsulation and align with internal naming conventions. Adjustments were made to all associated references to ensure consistency and functionality.
Changed `_state` visibility from public to private in `InterceptTrait` and `Php82InterceptTrait`. Updated tests to remove direct modifications to `_state`, ensuring encapsulation and adherence to best practices.
Removed the unnecessary `isAspect` parameter from the InterceptTraitState constructor and set its default value directly in the class property. This simplifies the initialization process and reduces redundant arguments.
Replaced explicit type definitions with imported Psalm type aliases for better readability and maintainability. Updated method parameter annotations to use `MethodBindings` and `Arguments` from the `Types` class.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/InterceptTraitState.php (1)
19-23: Consider initializing all properties in constructor.The constructor only initializes
$bindingsbut not$isAspect. While$isAspecthas a default value, it would be more explicit to initialize it in the constructor./** @param MethodBindings $bindings */ public function __construct(array $bindings) { $this->bindings = $bindings; + $this->isAspect = true; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src-php8/Php82InterceptTrait.php(1 hunks)src/InterceptTrait.php(2 hunks)src/InterceptTraitState.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src-php8/Php82InterceptTrait.php
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: koriym
PR: ray-di/Ray.Aop#236
File: src/InterceptTraitState.php:9-13
Timestamp: 2025-02-13T15:39:57.211Z
Learning: The Ray.Aop package maintains compatibility with PHP 7.2 and higher versions. Therefore, PHP 8.2+ specific features like the native readonly modifier should not be used, and backward-compatible alternatives (like docblock annotations) should be preferred.
src/InterceptTrait.php (3)
Learnt from: koriym
PR: ray-di/Ray.Aop#236
File: src/InterceptTraitState.php:9-13
Timestamp: 2025-02-13T15:39:57.211Z
Learning: The Ray.Aop package maintains compatibility with PHP 7.2 and higher versions. Therefore, PHP 8.2+ specific features like the native readonly modifier should not be used, and backward-compatible alternatives (like docblock annotations) should be preferred.
Learnt from: koriym
PR: ray-di/Ray.Aop#236
File: src/InterceptTrait.php:0-0
Timestamp: 2025-02-13T17:03:58.489Z
Learning: In Ray.Aop, internal state properties in traits should be private and prefixed with underscore (_) to indicate internal usage.
Learnt from: koriym
PR: ray-di/Ray.Aop#236
File: src/InterceptTrait.php:39-52
Timestamp: 2025-02-13T15:35:13.091Z
Learning: The `_intercept` method in Ray.Aop is a special AOP method where robustness checks are intentionally omitted to prioritize simplicity and performance. The method is designed to be minimal and fast, following AOP principles.
src/InterceptTraitState.php (2)
Learnt from: koriym
PR: ray-di/Ray.Aop#236
File: src/InterceptTraitState.php:9-13
Timestamp: 2025-02-13T15:39:57.211Z
Learning: The Ray.Aop package maintains compatibility with PHP 7.2 and higher versions. Therefore, PHP 8.2+ specific features like the native readonly modifier should not be used, and backward-compatible alternatives (like docblock annotations) should be preferred.
Learnt from: koriym
PR: ray-di/Ray.Aop#236
File: src/InterceptTrait.php:0-0
Timestamp: 2025-02-13T17:03:58.489Z
Learning: In Ray.Aop, internal state properties in traits should be private and prefixed with underscore (_) to indicate internal usage.
🪛 PHPStan (2.0.3)
src/InterceptTrait.php
12-12: Trait Ray\Aop\InterceptTrait is used zero times and is not analysed.
See: https://phpstan.org/blog/how-phpstan-analyses-traits
(trait.unused)
🔇 Additional comments (5)
src/InterceptTraitState.php (2)
8-9: LGTM!The class is correctly marked as
finaland uses strict types.
10-14: LGTM!The property declaration correctly uses docblock annotations for type hints and readonly modifier to maintain PHP 7.2+ compatibility.
src/InterceptTrait.php (3)
14-19: LGTM!The property declaration follows the package's conventions:
- Private visibility with underscore prefix
- Uses docblock annotations for type hints and readonly modifier
21-30: LGTM!The
_initStatemethod:
- Correctly uses underscore prefix to prevent naming conflicts
- Properly initializes the state with bindings
39-52: LGTM!The
_interceptmethod changes:
- Successfully migrated to use new state management
- Maintains simplicity and performance focus
koriym
force-pushed
the
support-readonly-class
branch
from
d4b508c to
105a980
Compare
|
@ngmy これでどうでしょうか。変更分のreviewをお願いします！ |
|
@koriym LGTMです！ |
#235 のPRです。
Summary by Sourcery
Add support for readonly classes in PHP 8.2.
New Features:
Tests:
Summary by CodeRabbit
Summary by CodeRabbit
New Features
WeavedInterfacefor improved binding initialization.Tests