Use Ray.Aop's _setBindings() and Bump SA tool #298
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.
Upgraded `vimeo/psalm` to `^6.8` and adjusted `phpmetrics` to support version `^2.7 || v3.0.0rc8`. Removed `psalm/plugin-phpunit` as it is no longer required.
Marking classes as final prevents unwanted inheritance and ensures safer, more predictable behavior. This change improves code maintainability and aligns with best practices for limiting class extensibility where unnecessary.
The "tests/type" directory was excluded from the Psalm analysis configuration as it is no longer relevant. This simplifies the configuration and ensures that only essential directories are analyzed.
Replaced implicit null checks with strict comparisons to improve clarity and prevent potential bugs. Adjusted or added suppress annotations for static analysis tools where required. Removed unnecessary assertions and cleaned up redundant code.
Annotations restrict `testIsOptionalValue` and `testToConstructor` to Linux and Darwin. This ensures tests are only executed in compatible environments.
Marking `ProviderSetModule` and `NullModule` as `final` prevents further extension and enforces immutability. This change improves code clarity and ensures that these classes are used as intended.
Removed unnecessary typecasting to string for function names, as the `getDeclaringFunction()->name` already returns a string. This improves code clarity and avoids redundant operations.
Suppressed 'MissingOverrideAttribute' errors in psalm configuration to prevent unnecessary warnings. Downgraded the sebastian/diff package from version 7.0.0 to 6.0.2 to maintain compatibility with PHP 8.2 and adjust dependency requirements.
Upgraded the Ray/AOP package version from ^2.x-dev to ^2.18 in composer.json. This ensures more stability by relying on a specific version rather than a development branch.
Removed redundant inline comments suppressing static analysis warnings in test files. Added explicit type casting for array keys in the Name class to align with strict typing requirements. These changes improve code clarity and maintain consistency with coding standards.
Reviewer's Guide by SourceryThis pull request includes improvements to type annotations, updates to dependencies (ray/aop), adjustments to the static analysis configuration (SA tool), and minor code cleanup. The changes enhance code clarity, maintain consistency with coding standards, and improve the reliability of static analysis. Sequence diagram for AOP enablement in NewInstancesequenceDiagram
participant NewInstance
participant AspectBind
participant WeavedInterface
participant Container
NewInstance->NewInstance: postNewInstance(container, instance)
alt is AspectBind
NewInstance->NewInstance: enableAop(instance, container)
NewInstance->AspectBind: inject(container)
AspectBind->Container: Get interceptors
AspectBind-->NewInstance: Interceptors
NewInstance->WeavedInterface: _setBindings(interceptors)
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
WalkthroughThis pull request updates dependency versions and enhances static analysis configurations across the project. Changes include updating the Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant NI as NewInstance
participant AB as AspectBind
participant WI as WeavedInterface
C->>NI: Request new instance with args
NI->>NI: Instantiate object
NI->>NI: Call enableAop(object, container)
NI->>AB: Check if bind is AspectBind
AB-->>NI: Return AOP bindings
NI->>WI: Assert instance is WeavedInterface and set bindings
NI-->>C: Return enhanced instance
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 (
|
![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 using more specific types for array keys in AspectBind::inject to improve type safety.
- The addition of #[Override] is good, but make sure your IDE or tooling enforces this annotation.
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.
The #[Override] annotations were removed as they are unnecessary and do not impact the functionality of the methods. This change simplifies the code without altering its behavior or logic.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
src/di/Exception/InvalidContext.php (1)
9-9: LGTM! Completes the pattern of final exception classes.This change, along with the others, creates a consistent pattern of final exception classes throughout the codebase, which improves type safety and makes exception handling more predictable.
Consider documenting this pattern in your coding standards to ensure future exception classes are also marked as final.
tests/di/ModuleTest.php (1)
55-57: LGTM! Good handling of cross-platform line endings.The introduction of the
$normalizefunction improves test reliability by handling platform-specific line endings consistently.Consider extracting this as a reusable test utility function since line ending normalization might be needed in other tests:
+ /** + * Normalizes line endings to \n + */ + private static function normalizeLineEndings(string $str): string + { + return str_replace(["\r\n", "\r"], "\n", $str); + }psalm.xml (1)
15-17: Consider alternatives to suppressing MissingOverrideAttribute.Suppressing
MissingOverrideAttributecould hide potential inheritance issues. Consider adding@overrideannotations instead of suppressing the check.composer.json (1)
53-54: Baseline Script Addition: Validate Integration with Static Analysis Tools.
The newly addedbaselinescript, which runs PHPStan and Psalm baseline commands, should streamline future static analysis improvements. Ensure that your configuration files (such asphpstan.neonandpsalm-baseline.xml) are adjusted accordingly so that baseline generation works as intended.tests/di/AnnotatedClassTest.php (1)
24-24: Unnecessary Suppression Warning: Review@phpstan-ignore-line.
The addition of// @phpstan-ignore-lineon the instantiation line suppresses errors that static analysis (PHPStan 2.0.3) reports as non-issues. Please consider removing this suppression to avoid concealing potential future problems and to keep the code cleaner.🧰 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 (36)
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/Argument.php(3 hunks)src/di/AspectBind.php(1 hunks)src/di/AssistedInjectModule.php(1 hunks)src/di/AssistedModule.php(1 hunks)src/di/Bind.php(1 hunks)src/di/BindValidator.php(1 hunks)src/di/Container.php(1 hunks)src/di/Dependency.php(2 hunks)src/di/DependencyProvider.php(1 hunks)src/di/Exception.php(1 hunks)src/di/Exception/DirectoryNotWritable.php(1 hunks)src/di/Exception/InvalidContext.php(1 hunks)src/di/Exception/InvalidProvider.php(1 hunks)src/di/Exception/InvalidToConstructorNameParameter.php(1 hunks)src/di/Exception/InvalidType.php(1 hunks)src/di/Exception/MethodInvocationNotAvailable.php(1 hunks)src/di/Exception/NotFound.php(1 hunks)src/di/Exception/SetNotFound.php(1 hunks)src/di/Exception/Untargeted.php(1 hunks)src/di/InjectionPoint.php(1 hunks)src/di/Injector.php(1 hunks)src/di/MultiBinding/MultiBindingModule.php(1 hunks)src/di/Name.php(1 hunks)src/di/NewInstance.php(4 hunks)src/di/NullModule.php(1 hunks)src/di/ProviderSetModule.php(1 hunks)tests/di/AnnotatedClassTest.php(1 hunks)tests/di/InjectorTest.php(2 hunks)tests/di/ModuleTest.php(2 hunks)tests/type/InjectorInterfaceTest.php(1 hunks)vendor-bin/tools/composer.json(1 hunks)
✅ Files skipped from review due to trivial changes (12)
- src/di/NullModule.php
- src/di/ProviderSetModule.php
- phpstan-baseline.neon
- src/di/InjectionPoint.php
- src/di/MultiBinding/MultiBindingModule.php
- src/di/Exception/NotFound.php
- src/di/Exception/Untargeted.php
- src/di/Container.php
- src/di/Exception.php
- src/di/Exception/MethodInvocationNotAvailable.php
- tests/di/InjectorTest.php
- src/di/Exception/InvalidToConstructorNameParameter.php
🧰 Additional context used
🧠 Learnings (1)
composer.json (1)
Learnt from: koriym
PR: ray-di/Ray.Di#297
File: composer.json:17-17
Timestamp: 2025-02-22T08:34:22.655Z
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.
🪛 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)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Demo
🔇 Additional comments (23)
src/di/AssistedInjectModule.php (1)
12-12: LGTM! Good use of thefinalkeyword.Making the class final is a good practice as it:
- Prevents unintended inheritance
- Makes the code more maintainable by clearly defining the class's contract
- Aligns with the PR's goal of improving type safety
src/di/AssistedModule.php (1)
12-12: LGTM! Good use of thefinalkeyword.Making the class final is a good practice as it:
- Prevents unintended inheritance
- Makes the code more maintainable by clearly defining the class's contract
- Aligns with the PR's goal of improving type safety
- Matches the pattern of making similar classes final across the codebase
src/di/Exception/SetNotFound.php (1)
9-9: LGTM! Making the exception class final improves type safety.Making exception classes final is a good practice as it prevents inheritance which could lead to LSP violations and makes exception handling more predictable. This change aligns well with PHP's built-in exceptions which are mostly final.
src/di/Exception/DirectoryNotWritable.php (1)
9-9: LGTM! Consistent with making all exception classes final.This change follows the same pattern of making exception classes final, which improves type safety and exception handling predictability.
src/di/Exception/InvalidType.php (1)
9-9: LGTM! Consistent application of final keyword to exception classes.Making this exception class final aligns with the codebase-wide improvement to exception handling and type safety.
src/di/Exception/InvalidProvider.php (1)
9-9: LGTM! Good use of thefinalkeyword.Making the exception class
finalimproves type safety by preventing inheritance, which is a good practice for exception classes that don't need to be extended.src/di/AspectBind.php (1)
25-25: LGTM! Good improvement in type safety.The change from
stringtonon-empty-stringin the return type annotation provides stronger type guarantees, ensuring that method names in the returned array cannot be empty strings.src/di/AnnotatedClassMethods.php (1)
12-12: LGTM! Good type safety improvement.The addition of the assertion helps static analysis tools understand the type better and catches potential type-related issues at runtime.
Also applies to: 43-43
src/di/DependencyProvider.php (1)
63-63: LGTM! Improved null check.The explicit
!== nullcheck is more precise than relying on PHP's loose type coercion, especially since$this->instanceis typed as?mixed.src/di/Injector.php (1)
21-21: LGTM! Good use offinalkeyword.Making the class
finalis appropriate here as it implementsInjectorInterface, which defines its contract. This prevents potential issues with inheritance and helps maintain the class's invariants.src/di/NewInstance.php (3)
8-8: LGTM! Added import for WeavedInterface.The import aligns with the new type assertion in the enableAop method.
107-116: LGTM! Well-structured AOP enablement method.The new method improves code organization by:
- Extracting AOP logic into a dedicated method
- Adding type safety with instanceof checks
- Using assertions for runtime type verification
51-51:❓ Verification inconclusive
Consider removing ArgumentTypeCoercion suppression.
The suppression might be unnecessary with the updated static analysis tools. Try removing it to see if it still raises issues.
🏁 Script executed:
#!/bin/bash # Check if removing the suppression causes any issues psalm --set-baseline=psalm-baseline.xmlLength of output: 137
Manual Verification Required: Confirm the effects of removing the suppression
The automated check couldn’t verify the impact because the psalm command wasn’t found in the current environment. Please ensure that psalm is installed and available in your local or CI environment. Then, manually re-run psalm to verify that removing the
ArgumentTypeCoercionsuppression does not introduce any issues.
- File:
src/di/NewInstance.php(line 51)- Action: Remove
ArgumentTypeCoercionfrom the suppression and verify with psalm that no new warnings or errors occur.src/di/Name.php (1)
159-159: LGTM! Improved type safety with explicit string casting.The explicit string casting before trim ensures type safety and aligns with the PR's objective of improving type annotations.
src/di/Dependency.php (1)
70-70: LGTM! Enhanced type safety with explicit null checks.The changes improve type safety by:
- Using explicit null comparison instead of implicit truthiness checks
- Making the code more maintainable and easier to understand
- Aligning with static analysis requirements
Also applies to: 94-94, 102-102
src/di/Argument.php (2)
7-7: LGTM! Added Override import.The import supports the new Override attributes.
105-105: LGTM! Added Override attributes.The Override attributes improve type safety by:
- Explicitly marking overridden methods
- Enabling compile-time verification of method signatures
- Making the code more maintainable
Also applies to: 151-152
src/di/Bind.php (1)
120-120: LGTM! Improved type safety with explicit null check.The change from a truthy check to an explicit null check (
$postConstruct !== null) enhances type safety and prevents potential issues with falsy non-null values.vendor-bin/tools/composer.json (2)
8-8:❓ Verification inconclusive
Verify compatibility with Psalm v6.x.
Major version upgrade from v4.x to v6.x could introduce breaking changes.
🌐 Web query:
What are the breaking changes in Psalm v6.x compared to v4.x?💡 Result:
Psalm v6.x introduced several breaking changes compared to v4.x, primarily focused on modernization and compatibility updates:
Core Dependency Updates
- PHP 8.1+ requirement: Psalm now requires PHP 8.1 or higher[7][9], dropping support for older PHP versions.
- PHP-Parser v5: Migrated to
nikic/php-parserv5[7][9], which removes dedicated parsing support for PHP 5.x code.- Amp v3: Updated the
amphp/ampdependency to version 3[7][9], introducing async runtime changes.Impact on Analysis
- PHP 5.x code compatibility: Projects relying on Psalm to analyze legacy PHP 5.x code may encounter issues due to PHP-Parser v5’s focus on modern PHP versions[7].
- Plugin compatibility: Plugin authors must update their code due to BC-breaking changes in dependencies and internal APIs[9].
Ecosystem Alignment
These changes ensure compatibility with tools like PHPStan 2.0 and Rector 2.0, which also adopted PHP-Parser v5[7]. Developers using Psalm alongside these tools benefit from unified dependency versions.
For detailed migration guidance, consult Psalm’s official changelog[9].
Citations:
- 1: https://www.blueletterbible.org/Comm/mhc/Psa/Psa_006.cfm
- 2: https://enduringword.com/bible-commentary/psalm-85/
- 3: https://enduringword.com/bible-commentary/psalm-6/
- 4: https://www.biblestudytools.com/psalms/6.html
- 5: https://www.biblegateway.com/passage/?search=Psalm+6&version=KJV
- 6: https://nextcloud.com/changelog/
- 7: 6.0 planning vimeo/psalm#10701
- 8: https://en.wikipedia.org/wiki/Ten_Commandments
- 9: https://newreleases.io/project/github/vimeo/psalm/release/6.0.0
- 10: https://www.youtube.com/watch?v=mGuTsIwKH3c
Verify compatibility with Psalm v6.x and assess PHPUnit plugin removal impact.
Upgrading from Psalm v4.x to ^6.8 introduces several breaking changes:
- PHP Version Requirement: Psalm now requires PHP 8.1+, so ensure your project is running on PHP 8.1 or newer.
- PHP-Parser Update: Migration to
nikic/php-parserv5 may impact legacy PHP code analysis.- Async Runtime Changes: The update to
amphp/amp(v3) may affect existing async workflows.- Plugin Adjustments: Removal of
psalm/plugin-phpunitnecessitates verifying alternative test coverage methods.Please review these areas to confirm that no unintended issues occur as a result of these updates.
9-9:✅ Verification successful
Verify impact of removing psalm/plugin-phpunit.
The PHPUnit plugin for Psalm has been removed. Please ensure that test coverage analysis is not affected.
🏁 Script executed:
#!/bin/bash # Check if PHPUnit assertions are still being type-checked without the plugin # Look for PHPUnit assertion methods in test files echo "Searching for PHPUnit assertions in tests..." rg -t php "assert[A-Z]\w+" "tests/"Length of output: 19340
Static Analysis Update: Removal of psalm/plugin-phpunit Verified
The grep output shows that PHPUnit assertions remain fully present across our tests. With the switch to phpstan/phpstan-phpunit, our static analysis for PHPUnit is still covered, and there’s no evident impact on test coverage.
psalm.xml (1)
7-7:❓ Verification inconclusive
Verify impact of disabling unused code detection.
Setting
findUnusedCode="false"could hide dead code. Please ensure this is intentional and that code coverage tools are in place to detect truly unused code.
🏁 Script executed:
#!/bin/bash # Check for potentially unused code that might be missed # Look for functions/methods without references echo "Searching for potentially unused functions..." ast-grep --pattern $'function $name($_) { $$$ }' # Look for classes without references echo "Searching for potentially unused classes..." ast-grep --pattern $'class $name { $$$ }'Length of output: 294
Action Required: Verify Overall Impact of Changes in psalm.xml
- The verification script did not detect any potentially unused functions or classes, which suggests that disabling unused code detection (via
findUnusedCode="false") hasn't immediately hidden dead code. However, remain cautious—ensure that you have supplementary code coverage or dynamic analysis in place to catch any genuinely unused code.- Please confirm that excluding the
tests/typedirectory from analysis does not compromise test type-safety, and that its removal is fully intentional.- Reevaluate the temporary suppression of
MissingOverrideAttributeto make sure that no critical inheritance issues are overlooked.composer.json (2)
17-17: Dependency Version Update: Verify Ray/AOP Version Bump.
The version update from an earlier specification to"ray/aop": "^2.18"is clearly intentional. However, prior guidance indicated using a development version (e.g.,"^2.x-dev") to leverage forthcoming features. Please confirm that this explicit version bump aligns with your dependency strategy.
50-50: Static Analysis Flag Change: Confirm Intent of--show-infoUpdate.
Changing the Psalm flag from--show-info=trueto--show-info=falseappears designed to reduce extraneous output. Please verify that this behavior meets your intended static analysis reporting requirements.
The pull request includes improvements to type annotations and removal of unnecessary suppressions in test files, enhancing code clarity and maintaining consistency with coding standards.
Summary by Sourcery
This pull request focuses on improving code quality and static analysis tooling. It includes enhancements to type annotations, updates to dependencies, and adjustments to the static analysis configuration.
Enhancements:
Build:
Summary by CodeRabbit