Use Ray.Aop's new binding injector _setBindings #297
Conversation
The "baseline" script generates static analysis baselines for PHPStan and Psalm. This simplifies managing analysis results and helps suppress existing issues for future incremental improvements.
Introduced baseline files for PHPStan and Psalm to suppress known issues temporarily, improving focus on new errors. Added references to these baselines in the respective configuration files to aid static analysis setup.
Moved Aspect-Oriented Programming (AOP) binding logic to a new `enableAop` method for better modularity and clarity. Replaced direct assertions with a safer method call (`__setBindings`) to streamline the binding process and improve maintainability.
Switched the Ray/AOP dependency from a stable version to the ^2.x-dev branch. This allows for using the latest development changes while maintaining compatibility with the project.
Added stricter type assertion in `AnnotatedClassMethods` with `assert` to ensure constructor type safety. Removed unnecessary suppression comments and improved type annotations for better code reliability and maintainability.
Upgraded multiple packages to ensure compatibility and receive the latest bug fixes and improvements. Adjusted metadata such as branch aliases and autoload paths where necessary. This helps maintain system stability and prepares for future development.
Disabled the `--show-info` output in the `sa` script for psalm to reduce unnecessary verbosity. This ensures the analysis output remains focused on essential issues.
Added a normalization function to standardize line endings to '\n' in string assertions for consistent behavior across environments. Updated the test to apply this normalization to both expected and actual strings.
Reviewer's Guide by SourceryThis pull request migrates the AOP binding injection to use the Sequence diagram for AOP binding injection using _setBindingssequenceDiagram
participant NewInstance
participant AspectBind
participant WeavedInterface
participant Container
NewInstance->>NewInstance: enableAop(instance, container)
alt bind instanceof AspectBind
NewInstance->>AspectBind: inject(container)
AspectBind-->>NewInstance: bindings
NewInstance->>WeavedInterface: _setBindings(bindings) // Ray.Aop ^2.18
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @koriym - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding a more specific assertion message in
NewInstance::enableAopto clarify the expected interface. - The change to
ray/aopto^2.x-devshould be reverted.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 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.
WalkthroughThe changes update dependency versions and modify static analysis configurations. They adjust script commands in the package configuration, add new baseline configuration files for PHPStan and Psalm, and update annotations for improved type safety. The DI-related code is refactored by adding assertions and a new method for enhancing AOP behavior, while test cases are updated to handle line normalization and PHPStan ignore comments. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant NewInstance
participant AspectBind
participant WeavedInterface
Client->>NewInstance: Request new instance creation
NewInstance->>NewInstance: Call enableAop(instance, container)
alt Bind is instance of AspectBind
NewInstance->>AspectBind: Call inject() to retrieve bindings
AspectBind-->>NewInstance: Return bindings
NewInstance->>WeavedInterface: Set bindings via _setBindings()
else Bind is not an instance of AspectBind
NewInstance-->>Client: Return instance without AOP modifications
end
Poem
✨ Finishing Touches
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 (
|
![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 (1)
tests/di/AnnotatedClassTest.php (1)
24-24: Evaluate the Necessity of the PHPStan Ignore Comment
Static analysis indicates that no PHPStan error is reported on this line. Please confirm if the// @phpstan-ignore-linedirective is still needed. If it was added to bypass a past false positive that is no longer applicable, consider removing it to keep the code clean and avoid masking potential issues in the future.- $newInstance = $this->annotatedClass->getNewInstance(new ReflectionClass(FakeCar::class)); // @phpstan-ignore-line + $newInstance = $this->annotatedClass->getNewInstance(new ReflectionClass(FakeCar::class));🧰 Tools
🪛 PHPStan (2.0.3)
24-24: No error to ignore is reported on line 24.
(ignore.unmatchedLine)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
vendor-bin/tools/composer.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
composer.json(2 hunks)phpstan-baseline.neon(1 hunks)psalm-baseline.xml(1 hunks)psalm.xml(1 hunks)src/di/AnnotatedClassMethods.php(2 hunks)src/di/AspectBind.php(1 hunks)src/di/BindValidator.php(1 hunks)src/di/NewInstance.php(2 hunks)tests/di/AnnotatedClassTest.php(1 hunks)tests/di/ModuleTest.php(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- phpstan-baseline.neon
🧰 Additional context used
🪛 PHPStan (2.0.3)
src/di/BindValidator.php
45-45: Method Ray\Di\BindValidator::to() should return Ray\Aop\ReflectionClass but returns Ray\Aop\ReflectionClass.
(return.type)
tests/di/AnnotatedClassTest.php
24-24: No error to ignore is reported on line 24.
(ignore.unmatchedLine)
🔇 Additional comments (9)
src/di/AspectBind.php (1)
25-25: LGTM! Enhanced type safety for method names.The change from
stringtonon-empty-stringin the return type annotation is a good improvement as it better reflects that method names cannot be empty strings, helping catch potential issues during static analysis.tests/di/ModuleTest.php (1)
10-11: LGTM! Improved test reliability with line ending normalization.The addition of the
$normalizefunction makes the test more robust by handling different OS line endings (CRLF, CR, LF) consistently.Also applies to: 55-57
src/di/AnnotatedClassMethods.php (1)
12-12: LGTM! Enhanced type safety with runtime assertion.The addition of the assertion helps ensure type safety by verifying that
$constructoris aReflectionMethodbefore use, making the code more robust and helping static analysis tools verify type correctness.Also applies to: 43-43
src/di/NewInstance.php (3)
8-8: LGTM! Added import for type safety.The addition of WeavedInterface import enhances type safety for AOP functionality.
98-98: LGTM! Improved code organization.Extracting AOP logic into a dedicated method improves code organization and maintainability.
106-115: LGTM! Well-structured AOP enablement.The new enableAop method:
- Properly checks for AspectBind instance
- Ensures type safety with WeavedInterface assertion
- Uses Ray.Aop's new _setBindings method as intended
psalm.xml (1)
7-7: LGTM! Added error baseline configuration.The addition of errorBaseline improves static analysis management by tracking known issues.
psalm-baseline.xml (1)
28-31:❓ Verification inconclusive
Verify the InvalidArgument issue in NewInstance.
The baseline indicates a potential type mismatch in
$this->bind->inject($container). Please verify that the type signatures are correct with the new _setBindings implementation.
🏁 Script executed:
#!/bin/bash # Check the return type of inject() method and parameter type of _setBindings ast-grep --pattern 'class AspectBind { $$$ inject($$$) { $$$ } }' ast-grep --pattern 'interface WeavedInterface { $$$ _setBindings($$$) { $$$ } }'Length of output: 170
Attention: Verify Type Signatures for InvalidArgument Issue in NewInstance
The psalm baseline still flags a potential type mismatch in the call to
$this->bind->inject($container)insrc/di/NewInstance.php. Our initial automated search for the definitions of theinject()method in theAspectBindclass and the_setBindings()method in theWeavedInterfacedid not return any results. This lack of output suggests that additional manual verification is required.Please verify the following manually:
- Confirm that the
inject()method in theAspectBindclass is declared with the correct parameter and return types.- Ensure that the
_setBindings()method in theWeavedInterfaceis implemented with a signature that is consistent with whatinject()expects when invoked inNewInstance.php.If you find any discrepancies between these signatures—especially in light of the new
_setBindingsimplementation—please update them to resolve the InvalidArgument issue.composer.json (1)
50-50: LGTM! Improved static analysis configuration.The changes enhance the static analysis setup:
- Reduced noise by disabling info messages
- Added baseline generation commands
Also applies to: 54-54
| @@ -42,7 +42,7 @@ public function to(string $interface, string $class): ReflectionClass | |||
| throw new InvalidType("[{$class}] is no implemented [{$interface}] interface"); | |||
| } | |||
|
|
|||
| return new ReflectionClass($class); // @phpstan-ignore-line | |||
| return new ReflectionClass($class); | |||
There was a problem hiding this comment.
Fix type safety issue in return type.
The method promises to return ReflectionClass<T> but returns ReflectionClass<object>, which could lead to type safety issues.
Apply this diff to fix the type safety issue:
- return new ReflectionClass($class);
+ /** @var ReflectionClass<T> */
+ $reflectionClass = new ReflectionClass($class);
+ return $reflectionClass;📝 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.
| return new ReflectionClass($class); | |
| /** @var ReflectionClass<T> */ | |
| $reflectionClass = new ReflectionClass($class); | |
| return $reflectionClass; |
🧰 Tools
🪛 PHPStan (2.0.3)
45-45: Method Ray\Di\BindValidator::to() should return Ray\Aop\ReflectionClass but returns Ray\Aop\ReflectionClass.
(return.type)
| @@ -14,7 +14,7 @@ | |||
| "koriym/attributes": "^1.0.4", | |||
| "koriym/null-object": "^1.0", | |||
| "koriym/param-reader": "^1.0", | |||
| "ray/aop": "^2.16", | |||
| "ray/aop": "^2.x-dev", | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the stability implications of using dev version.
Using ^2.x-dev for ray/aop could introduce instability. Consider using a stable version that supports _setBindings.
🌐 Web query:
What is the latest stable version of ray/aop that supports _setBindings method?
💡 Result:
The latest stable version of Ray.Aop that supports the _setBindings method is version 2.16 "Tanabata", as indicated by its release notes[10]. However, this version introduces significant API simplifications and shifts toward using bind() instead of _setBindings for aspect configuration[1][10].
Key points:
- Version 2.16 explicitly mentions API changes aimed at simplifying usage[10].
- Earlier versions (like 2.10.x) still support
_setBindings, as seen in Laravel integration examples requiringray/aop ^2.10.1[4]. - The newer
bind()method (demonstrated in examples[1]) replaces_setBindingsin modern implementations:$aspect->bind((new Matcher())->any(), ..., [new Interceptor()]);
For projects requiring _setBindings, use versions ≤2.15, but consider migrating to bind() for future compatibility[10]. Always verify method availability in specific releases via official documentation[1][10].
Citations:
- 1: https://github.com/ray-di/Ray.Aop
- 2: https://hub.jmonkeyengine.org/t/scripting-languages-in-jme3/18289
- 3: https://github.com/ray-project/ray/releases
- 4: https://packagist.org/packages/ngmy/laravel.aop
- 5: https://chromium-bugs.chromium.narkive.com/L5dQ3PJM/100-messages-from-google-code
- 6: https://www.anyscale.com/blog/ray-version-1-11-released
- 7: https://docs.aws.amazon.com/xray/latest/devguide/xray-sdk-java-aop-spring.html
- 8: https://myray.app/blog/pausing-and-disabling-ray-execution
- 9: https://pypi.org/project/ray/
- 10: https://github.com/ray-di/Ray.Aop/releases
Stable Dependency Recommendation: Update ray/aop Version
- The use of
"ray/aop": "^2.x-dev"poses a potential risk due to instability from a dev release. - If your project depends on the legacy
_setBindingsmethod, note that while version 2.16 "Tanabata" is the latest stable release, it shifts to usingbind(), which may introduce breaking API changes. - To preserve compatibility with
_setBindings, consider pinning the dependency to a stable version prior to 2.16 (e.g.,^2.15.0or an earlier 2.x release). - Alternatively, if you prefer to upgrade, update your codebase to switch from
_setBindingsto the newbind()method as per the API changes in 2.16.
There was a problem hiding this comment.
@coderabbitai The next version still not released.
There was a problem hiding this comment.
@koriym Understood, thank you for the clarification about the unreleased version.
✏️ Learnings added
Learnt from: koriym
PR: ray-di/Ray.Di#297
File: composer.json:17-17
Timestamp: 2025-02-22T08:34:22.538Z
Learning: The use of "ray/aop": "^2.x-dev" in composer.json is intentional as it depends on features from the upcoming version that hasn't been released yet.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
|
Move to #298 |
Summary by Sourcery
This pull request updates the dependency injection mechanism to use Ray.Aop's new binding injector, enhancing the framework's capabilities and preparing it for future development. It also includes updates to the build and CI configurations, as well as improvements to the test suite.
Enhancements:
_setBindingsmethod inRay\Di\NewInstanceto inject dependencies, leveraging the new binding injector in Ray.Aop.Build:
ray/aopdependency to the^2.x-devversion to utilize the latest features and improvements.CI:
Tests:
Summary by CodeRabbit