Refactor test method names and add doc comments #241
Conversation
Upgraded multiple packages, including `amphp/amp` to v3.1.0 and related dependencies for compatibility with PHP 8.1. Added several new libraries such as `amphp/cache`, `amphp/parallel`, and `amphp/pipeline`. These updates improve overall functionality, compatibility, and performance of the project.
This commit uniformly applies the #[\Override] attribute to methods that override parent class methods or implement interfaces. This change improves code clarity by explicitly indicating method overrides, aiding in code maintenance and readability.
This change includes the phpstan/phpstan-phpunit package in the vendor-bin/tools/composer.json file. It enhances PHPUnit support for static analysis, improving code quality and test reliability.
Replaced inline `assert` calls with `PHPUnit` assertions for better test readability and robustness. Improved type hints and added missing annotations for clarity and consistency across files. Removed unused statements and comments to simplify the codebase.
Marking classes as final prevents unintended extension, ensuring stricter adherence to design principles and better maintainability. This change reduces risks of unforeseen behavior and increases code clarity.
Unified the usage of attribute syntax across the codebase by replacing #[\Override] with #[Override]. This improves code consistency and adheres to the preferred attribute naming conventions.
Updated the PHP version from 8.3 to 8.4 in both coding standards and static analysis GitHub workflows. This ensures compatibility and aligns with the latest PHP version support.
Moved the template annotation below the param tag for better structure and consistency with coding standards. No functional changes introduced.
Renamed test methods for improved clarity and consistency, aligning them with their respective functionalities. Added detailed doc comments to better describe each test's purpose, enhancing code readability and maintainability.
Reviewer's Guide by SourceryThis pull request refactors test method names for improved clarity, adds documentation, and updates the project to PHP 8.4. It also includes several code improvements such as enhanced type hinting, updated annotations, and the addition of static analysis tools. Several other improvements to code readability and maintainability were made throughout the project, including updates to dependencies. Updated class diagram for MatcherclassDiagram
class Matcher {
+any()
+annotatedWith($annotationName)
+subclassesOf($superClass)
+startsWith($prefix)
+logicalOr(AbstractMatcher $matcherA, AbstractMatcher $matcherB)
+logicalAnd(AbstractMatcher $matcherA, AbstractMatcher $matcherB)
+logicalNot(AbstractMatcher $matcher)
}
Matcher --|> MatcherInterface : implements
Matcher --|> AbstractMatcher : extends
note for Matcher "Class marked as final"
Updated class diagram for AnyMatcherclassDiagram
class AnyMatcher {
+matchesClass(ReflectionClass $class, array $arguments) : bool
+matchesMethod(ReflectionMethod $method, array $arguments) : bool
}
AnyMatcher --|> AbstractMatcher : extends
note for AnyMatcher "Class marked as final"
Updated class diagram for NullInterceptorclassDiagram
class NullInterceptor {
+invoke(MethodInvocation $invocation)
}
NullInterceptor --|> MethodInterceptor : implements
note for NullInterceptor "Class marked as final"
Updated class diagram for AnnotatedMatcherclassDiagram
class AnnotatedMatcher {
+matchesClass(ReflectionClass $class, array $arguments) : bool
+matchesMethod(ReflectionMethod $method, array $arguments) : bool
}
AnnotatedMatcher --|> BuiltinMatcher : extends
note for AnnotatedMatcher "Class marked as final"
Updated class diagram for LogicalAndMatcherclassDiagram
class LogicalAndMatcher {
+matchesClass(ReflectionClass $class, array $arguments) : bool
+matchesMethod(ReflectionMethod $method, array $arguments) : bool
}
LogicalAndMatcher --|> AbstractMatcher : extends
note for LogicalAndMatcher "Class marked as final"
Updated class diagram for LogicalNotMatcherclassDiagram
class LogicalNotMatcher {
+matchesClass(ReflectionClass $class, array $arguments) : bool
+matchesMethod(ReflectionMethod $method, array $arguments) : bool
}
LogicalNotMatcher --|> AbstractMatcher : extends
note for LogicalNotMatcher "Class marked as final"
Updated class diagram for LogicalOrMatcherclassDiagram
class LogicalOrMatcher {
+matchesClass(ReflectionClass $class, array $arguments) : bool
+matchesMethod(ReflectionMethod $method, array $arguments) : bool
}
LogicalOrMatcher --|> AbstractMatcher : extends
note for LogicalOrMatcher "Class marked as final"
Updated class diagram for StartsWithMatcherclassDiagram
class StartsWithMatcher {
+matchesClass(ReflectionClass $class, array $arguments) : bool
+matchesMethod(ReflectionMethod $method, array $arguments) : bool
}
StartsWithMatcher --|> AbstractMatcher : extends
note for StartsWithMatcher "Class marked as final"
Updated class diagram for SubclassesOfMatcherclassDiagram
class SubclassesOfMatcher {
+matchesClass(ReflectionClass $class, array $arguments) : bool
+matchesMethod(ReflectionMethod $method, array $arguments) : bool
}
SubclassesOfMatcher --|> AbstractMatcher : extends
note for SubclassesOfMatcher "Class marked as final"
Updated class diagram for CompilationFailedExceptionclassDiagram
class CompilationFailedException
CompilationFailedException --|> LogicException : extends
note for CompilationFailedException "Class marked as final"
Updated class diagram for InvalidAnnotationExceptionclassDiagram
class InvalidAnnotationException
InvalidAnnotationException --|> InvalidArgumentException : extends
InvalidAnnotationException --|> ExceptionInterface : implements
note for InvalidAnnotationException "Class marked as final"
Updated class diagram for InvalidMatcherExceptionclassDiagram
class InvalidMatcherException
InvalidMatcherException --|> InvalidArgumentException : extends
InvalidMatcherException --|> ExceptionInterface : implements
note for InvalidMatcherException "Class marked as final"
Updated class diagram for InvalidSourceClassExceptionclassDiagram
class InvalidSourceClassException
InvalidSourceClassException --|> LogicException : extends
InvalidSourceClassException --|> ExceptionInterface : implements
note for InvalidSourceClassException "Class marked as final"
Updated class diagram for MultipleClassInOneFileExceptionclassDiagram
class MultipleClassInOneFileException
MultipleClassInOneFileException --|> LogicException : extends
MultipleClassInOneFileException --|> ExceptionInterface : implements
note for MultipleClassInOneFileException "Class marked as final"
Updated class diagram for NotWritableExceptionclassDiagram
class NotWritableException
NotWritableException --|> RuntimeException : extends
NotWritableException --|> ExceptionInterface : implements
note for NotWritableException "Class marked as final"
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## 2.x #241 +/- ##
===========================================
Coverage 100.00% 100.00%
- Complexity 236 237 +1
===========================================
Files 29 29
Lines 608 608
===========================================
Hits 608 608 ☔ View full report in Codecov by Sentry. |
WalkthroughThis pull request updates the PHP version parameters in GitHub Actions workflows, enhances type annotations and PHPDoc comments in multiple source files, and enforces class finality by adding the Changes
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 (
|
Aligned the method docblock with detailed type annotations to improve clarity and enforce stricter type safety. Added a template definition and Psalm suppression for better static analysis support.
|
@coderabbitai review |
![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
🔭 Outside diff range comments (1)
src/Pointcut.php (1)
7-17: 🛠️ Refactor suggestionConsider making properties readonly to ensure immutability.
The class is marked as
@psalm-immutable, but it has public mutable properties. Consider making the properties readonly to ensure true immutability:/** @psalm-immutable */ class Pointcut { /** @var AbstractMatcher */ - public $classMatcher; + public readonly AbstractMatcher $classMatcher; /** @var AbstractMatcher */ - public $methodMatcher; + public readonly AbstractMatcher $methodMatcher; /** @var array<MethodInterceptor|class-string<MethodInterceptor>> */ - public $interceptors; + public readonly array $interceptors;
🧹 Nitpick comments (7)
tests/script/compile.php (1)
26-26: Consider adding return type declaration.While not directly related to the current changes, consider adding a return type declaration to improve type safety further.
-return $compiler->compile(FakeMock::class, $bind); +/** @return string */ +return $compiler->compile(FakeMock::class, $bind);src/Weaver.php (1)
57-69: Consider adding explicit error handling for reflection failures.The
newInstanceArgscall could throw aReflectionException. Consider wrapping this in a try-catch block to provide more meaningful error messages.public function newInstance(string $class, array $args): object { $aopClass = $this->weave($class); + try { /** @var T $instance */ $instance = (new ReflectionClass($aopClass))->newInstanceArgs($args); + } catch (\ReflectionException $e) { + throw new \RuntimeException( + sprintf('Failed to create instance of %s: %s', $aopClass, $e->getMessage()), + 0, + $e + ); + } if (! $instance instanceof WeavedInterface) { return $instance; }src/Exception/InvalidAnnotationException.php (1)
9-9: LGTM! Completes the consistent exception handling pattern.Making all exception classes final is a solid architectural decision that:
- Prevents inheritance misuse
- Makes the exception hierarchy more maintainable
- Follows PHP 8.x best practices
- Improves static analysis capabilities
Consider documenting this pattern in your contributing guidelines to ensure future exception classes follow the same approach.
src/BuiltinMatcher.php (1)
19-20: Consider marking the class as final.For consistency with other matcher classes in the codebase, consider marking this class as
finalto prevent inheritance.-class BuiltinMatcher extends AbstractMatcher +final class BuiltinMatcher extends AbstractMatchertests/ReflectionClassTest.php (1)
18-18: Remove unnecessary PHPStan ignore comment.The static analysis tool indicates that there is no error to ignore on this line.
- $this->class = new ReflectionClass(FakeClassTartget::class); // @phpstan-ignore-line + $this->class = new ReflectionClass(FakeClassTartget::class);🧰 Tools
🪛 PHPStan (2.0.3)
18-18: No error to ignore is reported on line 18.
(ignore.unmatchedLine)
sl-src/CacheReader.php (1)
164-169: Consider using return type annotation.The explicit variable type declaration improves type safety, but consider adding a return type annotation to the closure for even better type safety.
- function () use ($method, $reflector): array { + function () use ($method, $reflector): array<object> {tests/CompilerTest.php (1)
284-286: Consider using PHPUnit assertion.Replace the direct
assertwith a PHPUnit assertion for consistency.- assert(is_array(FakeMethodAnnotationReaderInterceptor::$methodAnnotations)); + $this->assertIsArray(FakeMethodAnnotationReaderInterceptor::$methodAnnotations);
📜 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 (49)
.github/workflows/coding-standards.yml(1 hunks).github/workflows/static-analysis.yml(1 hunks)sl-src/CacheReader.php(8 hunks)src/AnnotatedMatcher.php(3 hunks)src/AspectPecl.php(0 hunks)src/Bind.php(4 hunks)src/BuiltinMatcher.php(3 hunks)src/ClassList.php(2 hunks)src/Compiler.php(3 hunks)src/Exception/CompilationFailedException.php(1 hunks)src/Exception/InvalidAnnotationException.php(1 hunks)src/Exception/InvalidArgumentException.php(1 hunks)src/Exception/InvalidMatcherException.php(1 hunks)src/Exception/InvalidSourceClassException.php(1 hunks)src/Exception/LogicException.php(1 hunks)src/Exception/MultipleClassInOneFileException.php(1 hunks)src/Exception/NotWritableException.php(1 hunks)src/InterceptTrait.php(1 hunks)src/Matcher.php(7 hunks)src/Matcher/AnnotatedWithMatcher.php(3 hunks)src/Matcher/AnyMatcher.php(4 hunks)src/Matcher/LogicalAndMatcher.php(3 hunks)src/Matcher/LogicalNotMatcher.php(3 hunks)src/Matcher/LogicalOrMatcher.php(3 hunks)src/Matcher/StartsWithMatcher.php(3 hunks)src/Matcher/SubclassesOfMatcher.php(3 hunks)src/MethodSignatureString.php(0 hunks)src/NullInterceptor.php(1 hunks)src/Pointcut.php(1 hunks)src/ReflectionClass.php(6 hunks)src/ReflectionMethod.php(4 hunks)src/ReflectiveMethodInvocation.php(6 hunks)src/TypeString.php(1 hunks)src/WeavedInterface.php(1 hunks)src/Weaver.php(1 hunks)tests/AnnotatedMatcherTest.php(1 hunks)tests/AopCodeTest.php(5 hunks)tests/Aspect/Fake/FakeMyInterceptor.php(1 hunks)tests/BindTest.php(1 hunks)tests/CompilerTest.php(9 hunks)tests/Fake/FakeMethodAnnotationReaderInterceptor.php(1 hunks)tests/PointcutTest.php(1 hunks)tests/ReflectionClassTest.php(2 hunks)tests/ReflectiveMethodInvocationTest.php(2 hunks)tests/VariadicParamsTest.php(1 hunks)tests/WeaverTest.php(3 hunks)tests/script/compile.php(1 hunks)tests/script/weave.php(1 hunks)vendor-bin/tools/composer.json(1 hunks)
💤 Files with no reviewable changes (2)
- src/AspectPecl.php
- src/MethodSignatureString.php
✅ Files skipped from review due to trivial changes (10)
- .github/workflows/coding-standards.yml
- src/Exception/LogicException.php
- src/Exception/InvalidSourceClassException.php
- src/WeavedInterface.php
- src/Exception/InvalidArgumentException.php
- src/InterceptTrait.php
- src/Exception/MultipleClassInOneFileException.php
- src/Matcher/LogicalAndMatcher.php
- src/Matcher/SubclassesOfMatcher.php
- src/Compiler.php
🧰 Additional context used
🪛 PHPStan (2.0.3)
tests/ReflectionClassTest.php
18-18: No error to ignore is reported on line 18.
(ignore.unmatchedLine)
🔇 Additional comments (44)
tests/script/weave.php (2)
7-9: LGTM! Well-structured import statements.The addition of explicit imports for
ClassLoaderandassertimproves code clarity and maintainability.
13-13: LGTM! Good type safety enhancement.The assertion ensures type safety by verifying that
$loaderis an instance ofClassLoaderbefore it's used. This aligns well with the PR's objective of improving type safety.tests/script/compile.php (2)
7-9: LGTM! Well-structured import statements.The addition of explicit imports for
ClassLoaderandassertimproves code clarity and maintainability.
13-13: LGTM! Good type safety enhancement.The assertion ensures type safety by verifying that
$loaderis an instance ofClassLoaderbefore it's used. This aligns well with the PR's objective of improving type safety.src/Weaver.php (3)
13-46: LGTM! Well-structured class with proper type safety.The class is correctly marked as final with immutable properties (readonly), and the constructor parameter uses a precise type hint (non-empty-string).
48-56: LGTM! Excellent use of PHP generics for type safety.The method signature and documentation properly utilize PHP's template types to ensure type safety through generics.
71-105: LGTM! Well-documented methods with proper type hints.Both methods have clear implementations with appropriate type hints and error handling.
src/Exception/CompilationFailedException.php (1)
9-9: LGTM! Making the exception class final improves maintainability.Making exception classes final is a good practice as it prevents potentially incorrect inheritance and makes the exception hierarchy more maintainable.
src/Exception/NotWritableException.php (1)
9-9: LGTM! Consistent with other exception class changes.Making the exception class final aligns with the project's approach to exception handling and modern PHP practices.
src/Exception/InvalidMatcherException.php (1)
9-9: LGTM! Follows consistent exception handling pattern.Making the exception class final maintains consistency with other exception classes in the codebase.
src/NullInterceptor.php (1)
7-9: LGTM! Good improvements to type safety.The changes enhance type safety and code clarity by:
- Making the class immutable with the
finalkeyword- Explicitly marking method overrides with the
#[Override]attributeAlso applies to: 12-13
tests/Aspect/Fake/FakeMyInterceptor.php (1)
7-8: LGTM! Good addition of runtime type checking.The assertion enhances type safety by validating the return type at runtime, complementing the static return type declaration.
Also applies to: 16-16
tests/PointcutTest.php (1)
11-14: LGTM! Good improvements to test documentation.The changes improve code clarity by:
- Using a more descriptive test method name
- Adding clear documentation about the test's purpose
tests/AnnotatedMatcherTest.php (1)
15-19: LGTM! Good improvements to test documentation.The changes improve code clarity by:
- Using a more descriptive test method name
- Adding clear documentation about the test's purpose
tests/VariadicParamsTest.php (1)
11-14: LGTM! Improved test method name and documentation.The changes enhance clarity by:
- Using a more descriptive method name that better reflects the test's purpose
- Adding a clear docblock comment explaining what is being tested
src/Matcher/StartsWithMatcher.php (1)
7-7: LGTM! Added #[Override] attributes to improve clarity.The addition of
#[Override]attributes explicitly indicates that these methods override methods from the parent class, improving code clarity and maintainability.Also applies to: 19-19, 31-31
src/Matcher/LogicalNotMatcher.php (1)
7-7: LGTM! Added #[Override] attributes to improve clarity.The addition of
#[Override]attributes explicitly indicates that these methods override methods from the parent class, improving code clarity and maintainability.Also applies to: 19-19, 31-31
tests/Fake/FakeMethodAnnotationReaderInterceptor.php (1)
16-17: LGTM! Good type annotation.The added PHPDoc comment correctly specifies the type as
array<object>|null, which aligns with the property's usage in theinvokemethod.src/Matcher/AnnotatedWithMatcher.php (1)
7-7: LGTM! Good use of Override attribute.The
#[Override]attribute correctly marks methods that override parent class methods, improving code clarity and static analysis capabilities.Also applies to: 19-20, 34-35
src/Matcher/LogicalOrMatcher.php (1)
7-7: LGTM! Good use of Override attribute.The
#[Override]attribute correctly marks methods that override parent class methods, improving code clarity and static analysis capabilities.Also applies to: 19-20, 36-37
src/BuiltinMatcher.php (1)
7-7: LGTM! Good use of Override attribute.The
#[Override]attribute correctly marks methods that override parent class methods, improving code clarity and static analysis capabilities.Also applies to: 53-54, 62-63
src/AnnotatedMatcher.php (1)
7-7: LGTM! Good improvements to type safety and inheritance clarity.The changes enhance code clarity and type safety through:
- Making the class
finalto prevent unexpected inheritance- Adding
#[Override]attributes to clearly indicate method overridesAlso applies to: 14-14, 36-37, 50-51
src/Matcher/AnyMatcher.php (1)
8-8: LGTM! Good improvements to method override clarity.The changes enhance code clarity through:
- Adding
#[Override]attributes to clearly indicate method overrides- Maintaining proper type safety with
@var BuiltinMethodsNamesannotationAlso applies to: 23-24, 40-41, 51-52
src/ReflectionMethod.php (1)
7-7: LGTM! Good improvements to method override clarity.The changes enhance code clarity through:
- Adding
#[Override]attributes to clearly indicate method overrides- Maintaining existing type safety with psalm annotations
Also applies to: 33-34, 52-53, 72-73
src/Matcher.php (1)
7-7: LGTM! Good improvements to type safety and inheritance clarity.The changes enhance code clarity and type safety through:
- Making the class
finalto prevent unexpected inheritance- Adding
#[Override]attributes to clearly indicate interface method implementationsAlso applies to: 13-13, 20-21, 29-30, 42-43, 57-58, 88-89
tests/ReflectionClassTest.php (1)
21-78: LGTM! Test method names and documentation improvements.The changes improve test clarity by:
- Using more descriptive method names that better reflect their purpose
- Adding clear PHPDoc comments that explain what each test verifies
src/ReflectionClass.php (1)
17-98: LGTM! Enhanced type safety and inheritance clarity.The changes improve the codebase by:
- Making the class final to prevent inheritance
- Adding #[Override] attributes to clearly indicate method overrides
- Using #[ReturnTypeWillChange] for PHP 8.0+ compatibility
src/ClassList.php (1)
9-81: LGTM! Added Override attribute for clarity.The addition of the #[Override] attribute to getIterator method clearly indicates it implements the IteratorAggregate interface method.
src/TypeString.php (1)
72-77: LGTM! Improved readability with foreach loop.The change from array_map to foreach loop:
- Makes the code more readable and straightforward
- Properly handles type annotations
- Maintains the same functionality
src/Bind.php (1)
7-7: LGTM! The #[Override] attributes are correctly placed.The addition of the #[Override] attributes to the methods implementing BindInterface enhances code clarity and type safety.
Also applies to: 53-53, 79-80, 96-97
tests/WeaverTest.php (1)
16-19: LGTM! Test methods are now more descriptive and better documented.The renaming of test methods and addition of clear docblocks significantly improves test readability and maintainability.
Also applies to: 30-35, 37-45, 47-54, 56-63, 65-73, 75-82, 84-94, 96-105, 107-114, 116-124, 126-133, 135-142
src/ReflectiveMethodInvocation.php (1)
8-8: LGTM! The #[Override] attributes are correctly placed.The addition of the #[Override] attributes to the methods implementing MethodInvocation enhances code clarity and type safety.
Also applies to: 77-78, 99-100, 110-111, 129-130, 145-146
tests/ReflectiveMethodInvocationTest.php (1)
28-35: LGTM! Test methods are now more descriptive and better documented.The renaming of test methods and addition of clear docblocks significantly improves test readability and maintainability.
Also applies to: 37-45, 47-54, 56-63, 65-73, 75-82, 84-94, 96-105, 107-114, 116-124, 126-133, 135-142
tests/BindTest.php (1)
60-69: Add assertions to validate behavior intestToString.The test method executes code but doesn't verify any behavior. Consider either:
- Adding assertions to validate the string representation of the
Bindobject- Documenting why assertions were removed if this is intentional
Example assertions that could be added:
public function testToString(): void { $nullBind = (string) (new Bind()); + $this->assertNotEmpty($nullBind, 'Empty Bind should have a string representation'); $interceptors = [new FakeDoubleInterceptor()]; $pointcut = new Pointcut((new Matcher())->startsWith('Ray'), (new Matcher())->startsWith('get'), $interceptors); $this->bind->bind(FakeAnnotateClass::class, [$pointcut]); $bindString = (string) $this->bind; + $this->assertNotEmpty($bindString, 'Bind with interceptors should have a string representation'); + $this->assertNotEquals($nullBind, $bindString, 'Bind with interceptors should have different string representation than empty Bind'); }sl-src/CacheReader.php (2)
52-72: LGTM! Enhanced type safety and documentation.The addition of type annotations and the
#[Override]attribute improves code clarity and type safety.
74-95: LGTM! Improved type annotations for better type safety.The enhanced docblock with template type annotations provides better type safety and clarity.
tests/AopCodeTest.php (4)
32-42: LGTM! Improved test method name and documentation.The renamed method and added docblock clearly describe the test's purpose.
44-54: LGTM! Enhanced test method clarity.The renamed method and docblock improve understanding of what's being tested.
56-171: LGTM! Comprehensive test documentation.The detailed docblocks for PHP 8.1 and 8.2 method signatures provide clear context for the tests.
173-180: LGTM! Clear exception test documentation.The renamed method and docblock clearly indicate the expected exception behavior.
tests/CompilerTest.php (2)
193-196: LGTM! Improved type safety with class-string annotation.The added type annotation clarifies that
$mockis expected to be a valid class name.
208-211: LGTM! Enhanced type safety for annotation arrays.The added
object[]type annotations improve type safety for annotation handling.Also applies to: 216-219, 257-262, 270-274
.github/workflows/static-analysis.yml (1)
12-13: PHP Version Update VerifiedThe
php_versionparameter has been correctly updated to8.4in the static analysis workflow configuration. This change aligns with the project's aim to use a consistent PHP version across all GitHub Actions workflows. Please ensure that any other workflows (e.g., coding standards) are similarly updated and fully compatible with PHP 8.4.vendor-bin/tools/composer.json (1)
5-9: Dependency Versions Successfully UpdatedThe updates in the
"require-dev"section—updatingphpmetrics/phpmetricsto^v3.0.0rc8 || ^v3.0,phpstan/phpstanto^2.1,vimeo/psalmto^6.8, and addingphpstan/phpstan-phpunitwith version^2.0—are well-aligned with the shift to PHP 8.4 and the overall goal of bolstering static analysis support. These changes should help improve type safety and compatibility with advanced analysis tools such as PHPStan and Psalm.As these upgrades introduce new major versions (and even a release candidate), please ensure that:
- The updated dependencies are fully compatible with your existing codebase and CI/CD pipelines.
- You run the full test suite and static analysis tools to catch any unforeseen issues related to these dependency changes.
🧩 Analysis chain@koriym I'll review the changes in this PR. 🏁 Script executed: Length of output: 72346 @koriym The changes look great! The improvements include:
The changes improve code quality and maintainability while maintaining backward compatibility. ✅ Actions performedComments resolved. Review triggered.
|
Replaced incorrect attribute syntax `[\Override]` with `[Override]` for consistency. Added 'sl-src' directory to PHP CodeSniffer configuration to ensure style checks cover the new directory.
|
@NaokiTsuchiya Attribute Readerなどでインスタンス化されてしまったりするとクラスが無いことでエラーになってしまうことを防ぐために2d78490を加えました。 |
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 (4)
composer.json(1 hunks)phpcs.xml(1 hunks)src-compat/Override.php(1 hunks)src/Matcher.php(7 hunks)
✅ Files skipped from review due to trivial changes (1)
- src-compat/Override.php
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Matcher.php
🧰 Additional context used
🪛 GitHub Actions: Continuous Integration
composer.json
[warning] 1-1: Defining autoload.psr-4 with an empty namespace prefix is a bad idea for performance.
🔇 Additional comments (1)
phpcs.xml (1)
19-19: Directory Check Addition: Validate New "sl-src" Entry.
The new<file>sl-src</file>element adds thesl-srcdirectory to the PHP_CodeSniffer analysis scope. Ensure that this directory exists and follows the same coding standards assrcandtests.
The Override.php class was added and registered in the autoload section of composer.json. This ensures the class is loaded automatically if not already defined,(=PHP 8.0-8.2)
The `#[Override]` attribute and its usages have been removed across the codebase, as it is no longer required. The `src-compat/Override.php` file has also been deleted, and references in `composer.json` were updated to reflect the change.
Updated psalm configuration to suppress MissingOverrideAttribute errors. This ensures the codebase avoids unnecessary warnings for missing override attributes, streamlining the static analysis process.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
composer.json(1 hunks)sl-src/CacheReader.php(3 hunks)src/AnnotatedMatcher.php(1 hunks)src/Matcher.php(3 hunks)src/Matcher/AnyMatcher.php(1 hunks)src/NullInterceptor.php(1 hunks)src/ReflectionClass.php(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- composer.json
🚧 Files skipped from review as they are similar to previous changes (4)
- src/NullInterceptor.php
- src/Matcher/AnyMatcher.php
- src/AnnotatedMatcher.php
- src/Matcher.php
🧰 Additional context used
🪛 GitHub Check: sa / Psalm
sl-src/CacheReader.php
[failure] 59-59:
MissingOverrideAttribute: Method Ray\ServiceLocator\CacheReader::getclassannotations should have the "Override" attribute
[failure] 83-83:
MissingOverrideAttribute: Method Ray\ServiceLocator\CacheReader::getclassannotation should have the "Override" attribute
🔇 Additional comments (4)
sl-src/CacheReader.php (2)
48-48: LGTM!The property assignment formatting change improves code consistency.
158-161: LGTM!The explicit type declaration for
$annotationsand split return statement improve code readability and type safety.src/ReflectionClass.php (2)
12-15: Great job on type safety annotations!The code demonstrates excellent type safety practices:
- Uses template annotations for generic type safety
- Has detailed parameter and return type annotations
- Includes appropriate psalm suppressions with clear reasons
- Documents external mutation-free methods
Also applies to: 19-22, 32-40, 53-59, 72-75
16-16:❓ Verification inconclusive
Breaking Change: Class made final.
Making the class final prevents inheritance, which is a breaking change. However, this aligns with the PR's objective of improving inheritance control and type safety. Since this class already extends
\ReflectionClassand implementsReader, preventing further inheritance helps maintain a cleaner inheritance hierarchy.Let's verify if this class is being extended anywhere in the codebase:
🏁 Script executed:
#!/bin/bash # Search for any class that extends ReflectionClass ast-grep --pattern 'class $_ extends ReflectionClass'Length of output: 55
Attention: Final Class Declaration Verification Needed
The change to mark
ReflectionClassas final is an intentional breaking change aimed at improving inheritance control and ensuring robust type safety. An automated search for classes extendingReflectionClassproduced no output, suggesting that there are no downstream extensions relying on it. However, the low-confidence output means that you should manually verify that no classes in the codebase extendReflectionClass.
- The final keyword introduces a breaking change by disallowing inheritance.
- Automated search (using
ast-grep) did not reveal any subclass extensions, but manual verification is advised.- The type safety measures (strict types, template annotations, psalm suppressions) are correctly implemented and need no changes.
Please confirm that no other parts of the codebase rely on subclassing
ReflectionClass.
| /** | ||
| * {@inheritDoc} | ||
| * @param ReflectionClass<object> $class | ||
| * @param class-string<TAnnotation> $annotationName | ||
| * | ||
| * @return TAnnotation|null | ||
| * | ||
| * @template TAnnotation of object | ||
| * | ||
| * @psalm-suppress MoreSpecificImplementedParamType | ||
| * @psalm-external-mutation-free | ||
| */ | ||
| public function getClassAnnotation(ReflectionClass $class, $annotationName) // @phpstan-ignore-line | ||
| public function getClassAnnotation(ReflectionClass $class, $annotationName) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add #[Override] attribute and LGTM on type improvements!
The enhanced type annotations with template type TAnnotation and detailed parameter/return types improve type safety. However, the method should have an Override attribute as it implements an interface method.
Apply this diff to add the Override attribute:
/**
* @param ReflectionClass<object> $class
* @param class-string<TAnnotation> $annotationName
*
* @return TAnnotation|null
*
* @template TAnnotation of object
*
* @psalm-suppress MoreSpecificImplementedParamType
* @psalm-external-mutation-free
*/
+ #[Override]
public function getClassAnnotation(ReflectionClass $class, $annotationName)📝 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.
| /** | |
| * {@inheritDoc} | |
| * @param ReflectionClass<object> $class | |
| * @param class-string<TAnnotation> $annotationName | |
| * | |
| * @return TAnnotation|null | |
| * | |
| * @template TAnnotation of object | |
| * | |
| * @psalm-suppress MoreSpecificImplementedParamType | |
| * @psalm-external-mutation-free | |
| */ | |
| public function getClassAnnotation(ReflectionClass $class, $annotationName) // @phpstan-ignore-line | |
| public function getClassAnnotation(ReflectionClass $class, $annotationName) | |
| /** | |
| * @param ReflectionClass<object> $class | |
| * @param class-string<TAnnotation> $annotationName | |
| * | |
| * @return TAnnotation|null | |
| * | |
| * @template TAnnotation of object | |
| * | |
| * @psalm-suppress MoreSpecificImplementedParamType | |
| * @psalm-external-mutation-free | |
| */ | |
| + #[Override] | |
| public function getClassAnnotation(ReflectionClass $class, $annotationName) |
🧰 Tools
🪛 GitHub Check: sa / Psalm
[failure] 83-83:
MissingOverrideAttribute: Method Ray\ServiceLocator\CacheReader::getclassannotation should have the "Override" attribute
| /** | ||
| * {@inheritDoc} | ||
| * | ||
| * @param ReflectionClass<T> $class | ||
| * | ||
| * @template T of object | ||
| * @psalm-suppress MoreSpecificImplementedParamType | ||
| */ | ||
| public function getClassAnnotations(ReflectionClass $class) // @phpstan-ignore-line | ||
| public function getClassAnnotations(ReflectionClass $class) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add #[Override] attribute and LGTM on type improvements!
The enhanced type annotations with template type T improve type safety. However, the method should have an Override attribute as it implements an interface method.
Apply this diff to add the Override attribute:
/**
* {@inheritDoc}
*
* @param ReflectionClass<T> $class
*
* @template T of object
* @psalm-suppress MoreSpecificImplementedParamType
*/
+ #[Override]
public function getClassAnnotations(ReflectionClass $class)📝 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.
| /** | |
| * {@inheritDoc} | |
| * | |
| * @param ReflectionClass<T> $class | |
| * | |
| * @template T of object | |
| * @psalm-suppress MoreSpecificImplementedParamType | |
| */ | |
| public function getClassAnnotations(ReflectionClass $class) // @phpstan-ignore-line | |
| public function getClassAnnotations(ReflectionClass $class) | |
| /** | |
| * {@inheritDoc} | |
| * | |
| * @param ReflectionClass<T> $class | |
| * | |
| * @template T of object | |
| * @psalm-suppress MoreSpecificImplementedParamType | |
| */ | |
| #[Override] | |
| public function getClassAnnotations(ReflectionClass $class) |
🧰 Tools
🪛 GitHub Check: sa / Psalm
[failure] 59-59:
MissingOverrideAttribute: Method Ray\ServiceLocator\CacheReader::getclassannotations should have the "Override" attribute
Upgraded the PHP version in the Psalm configuration file from 8.3 to 8.4. This aligns the static analysis tool with the project's updated PHP version requirements.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
psalm.xml (1)
18-20: Addition of Issue Handlers for Psalm
The newly introduced<issueHandlers>section, particularly the suppression ofMissingOverrideAttributewarnings, is a pragmatic approach to reduce noise from known issues. However, consider adding an inline comment or documentation explaining the rationale for this suppression to assist future maintainers in understanding its intent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
psalm.xml(2 hunks)
🔇 Additional comments (1)
psalm.xml (1)
8-8: PHP Version Update
The update ofphpVersionfrom "8.3" to "8.4" is consistent with the changes applied in the corresponding GitHub Actions workflows and static analysis configurations. This change helps ensure that Psalm and other tools run under the intended PHP runtime version.
+phpstan 2 and vimeo 6 support
Align method docblock with detailed type annotations for improved clarity and stricter type safety. Add a definition and Psalm suppression for better static analysis support.
#[Override]https://wiki.php.net/rfc/marking_overriden_methodsThis pull request enhances the type safety annotations in CacheReader by aligning the method docblock with detailed type annotations. This improvement enhances code clarity and enforces stricter type safety measures. Additionally, a definition and Psalm suppression have been added to enhance static analysis support.
Summary by CodeRabbit
Chores
Refactor
Tests