Extract PECL code from Aspect to PeclAspect #223
Conversation
Corrected the extension name from 'rauoop' to 'rayaop' in the @requires annotation. This ensures the test checks for the appropriate PHP extension.
Deleted the method responsible for creating instances using the PECL extension, as well as the related conditional logic and method for a PHP-based implementation. This simplifies the `newInstance` function and removes unnecessary dependencies.
This commit deletes the check for the 'rayaop' extension in the `applyInterceptors` method. This change simplifies the method and assumes the dispatcher setup can proceed without verifying the extension's presence.
Introduce `ClassName` class to extract fully qualified class names from PHP files. Added comprehensive unit tests to cover multiple edge cases, including handling namespaces, class modifiers, and different token orders in the source files.
Removed redundant utility functions and adjusted the code structure to streamline class processing and improve readability. This refactor enhances maintainability by eliminating clutter and redundant code paths.
Added FakeFinalClass and FakePeclClass to support new aspect weaving tests. Updated AspectTest to include tests for weaving on final classes and added scenarios where class/method annotations do not match.
This commit extracts the weaving logic from the `Aspect` class to a new `AspectPecl` class. This change simplifies the `Aspect` class by delegating directory scanning and interceptor application responsibilities to `AspectPecl`, thereby enhancing maintainability and separation of concerns.
Moved the extension check from the `Aspect::weave` method to the `AspectPecl` constructor. This ensures the extension is verified upon instantiation, reducing redundancy and centralizing the validation logic.
This commit changes NotWritableException to extend RuntimeException instead of LogicException. Additionally, it updates the Aspect class to check if the provided directory is writable and throws a NotWritableException if it is not. A test case is also added to validate this new behavior.
Added PHP 8.1 and rayaop extension requirements to the test methods `testNotClassMatch` and `testNotMethodMatch` in `AspectTest.php`. This ensures the tests run under the correct environment.
This commit introduces the Codecov action into the continuous integration workflow for uploading code coverage reports. The `codecov-action` is configured with a token to securely upload the coverage data generated by PHPUnit.
Comment out the execution of 05-pecl.php in demo/run.php to prevent it from running in the demo environment. Instead, integrate its execution into the continuous integration workflow to ensure it runs during automated tests.
|
Warning Rate limit exceeded@koriym has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 10 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. WalkthroughThe pull request introduces several modifications across multiple files, primarily enhancing the continuous integration workflow and the implementation of aspect-oriented programming. Key changes include the introduction of a Changes
Possibly related PRs
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 #223 +/- ##
===========================================
Coverage 100.00% 100.00%
- Complexity 214 225 +11
===========================================
Files 27 28 +1
Lines 558 583 +25
===========================================
+ Hits 558 583 +25 ☔ View full report in Codecov by Sentry. |
![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
🧹 Outside diff range and nitpick comments (11)
tests/Aspect/AspectTest.php (1)
124-139: Consider adding combined annotation mismatch testGood test for method annotation non-matching. Consider adding a test case where both class and method annotations don't match to ensure complete coverage of matching scenarios.
public function testNotClassAndMethodMatch(): void { $aspect = new Aspect(); $aspect->bind( (new Matcher())->annotatedWith(FakeMarker::class), (new Matcher())->annotatedWith(FakeMarker::class), [new FakeMyInterceptor()] ); $aspect->weave(__DIR__ . '/Fake/src'); $billing = $aspect->newInstance(FakeMyClass::class); $this->assertInstanceOf(FakeMyClass::class, $billing); }tests/ClassNameTest.php (3)
25-25: Translate comment to English for consistency.The comment "一時ディレクトリを作成" should be in English to maintain consistency across the codebase.
- // 一時ディレクトリを作成 + // Create temporary directory
47-251: Consider refactoring for better maintainability.The test suite provides excellent coverage, but could be optimized for maintainability:
- Use data providers for similar test cases (e.g.,
testClassWithNamespace,testClassWithNamespaceAndModifiers,testAbstractClassWithNamespace).- Extract common file creation logic into a helper method.
- Define constants for repeated file paths and test data.
Example refactoring:
private function createTestFile(string $phpCode, string $filename): string { $filePath = $this->tempDir . '/' . $filename; file_put_contents($filePath, $phpCode); return $filePath; } /** * @dataProvider classDefinitionProvider */ public function testClassDefinitions(string $phpCode, string $expectedClassName): void { $filePath = $this->createTestFile($phpCode, 'Test.php'); $result = ClassName::from($filePath); $this->assertSame($expectedClassName, $result); } public function classDefinitionProvider(): array { return [ 'simple class' => [ '<?php namespace MyNamespace; class MyClass {}', 'MyNamespace\\MyClass' ], 'final class' => [ '<?php namespace MyNamespace; final class MyClass {}', 'MyNamespace\\MyClass' ], 'abstract class' => [ '<?php namespace MyNamespace; abstract class MyClass {}', 'MyNamespace\\MyClass' ], ]; }
135-135: Translate comment to English for consistency.The comment "最初に見つかったクラスを取得する" should be in English.
- // 最初に見つかったクラスを取得する + // Get the first found classsrc/ClassName.php (2)
41-41: Clarify the use of@phpstan-ignore-lineThe comment
// @phpstan-ignore-linesuppresses PHPStan warnings on thetoken_get_all(file_get_contents($filePath));line. Consider verifying whether this suppression is necessary, and if so, provide a brief explanation for future maintainability.
76-76: Translate comments to English for better maintainabilityThe comments on lines 76, 82, 84, and 94 are in Japanese. To improve collaboration and maintainability among all developers, it's recommended to use English for code comments.
Apply this diff to update the comments:
-// クラス修飾子をスキップ（T_FINAL、T_ABSTRACT） +// Skip class modifiers (T_FINAL, T_ABSTRACT) -// クラス名の取得 +// Obtain the class name -// ホワイトスペースとコメントをスキップ +// Skip whitespace and comments -// クラス名を取得 +// Retrieve the class nameAlso applies to: 82-82, 84-84, 94-94
src/Aspect.php (1)
52-56: Simplify Temporary Directory Handling and Improve Directory ChecksConsider simplifying the temporary directory assignment and enhancing the directory existence check.
You can refactor the constructor to default
$tmpDirtosys_get_temp_dir()and useis_dir()for a more accurate directory check.Apply this diff:
public function __construct(?string $tmpDir = null) { - if ($tmpDir === null) { - $tmp = sys_get_temp_dir(); - $tmpDir = $tmp !== '' ? $tmp : '/tmp'; - } + $tmpDir = $tmpDir ?? (sys_get_temp_dir() ?: '/tmp'); - if (! file_exists($tmpDir) || ! is_writable($tmpDir)) { + if (! is_dir($tmpDir) || ! is_writable($tmpDir)) { throw new NotWritableException("{$tmpDir} is not writable."); }This streamlines the logic and ensures that the directory check accurately verifies that
$tmpDiris a writable directory.src/AspectPecl.php (4)
68-69: Handle cases where no interceptors are boundBefore calling
$this->applyInterceptors($boundInterceptors);, consider checking if$boundInterceptorsis not empty. This will prevent unnecessary processing when there are no interceptors to apply.Apply this diff to add the check:
$boundInterceptors = $this->getBoundInterceptors($className, $matchers); + if (!empty($boundInterceptors)) { $this->applyInterceptors($boundInterceptors); + }
114-114: Address static analysis warning formethod_interceptfunctionStatic analysis tools report that the
method_interceptfunction is not found. Since this function is provided by the PECLrayaopextension, you can suppress this warning by adding a stub or update the static analysis configuration to recognize the extension functions.Consider creating a stub file for the
method_interceptfunction or informing your static analysis tool about the extension to eliminate the warning.
66-66: Remove redundantassert(class_exists($className), $className);The assertion
assert(class_exists($className), $className);is redundant. Before this point, you've already confirmed that the class exists. Removing the redundant assertions can streamline the code.Apply this diff to remove the redundant assertions:
--- a/src/AspectPecl.php +++ b/src/AspectPecl.php @@ -66,7 +66,6 @@ } - assert(class_exists($className), $className); $boundInterceptors = $this->getBoundInterceptors($className, $matchers);--- a/src/AspectPecl.php +++ b/src/AspectPecl.php @@ -83,7 +83,6 @@ private function getBoundInterceptors(string $className, array $matchers): array { - assert(class_exists($className), $className); $reflection = new ReflectionClass($className);Also applies to: 83-83
119-119: Remove unnecessary assertion for dispatcher instanceThe assertion
assert($dispatcher instanceof MethodInterceptorInterface);is unnecessary because$dispatcheris instantiated as aPeclDispatcher, which should already implementMethodInterceptorInterface.Apply this diff to remove the redundant assertion:
foreach ($methodNames as $methodName) { - assert($dispatcher instanceof MethodInterceptorInterface); method_intercept($className, $methodName, $dispatcher); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
.github/workflows/continuous-integration-pecl.yml(2 hunks)demo/run.php(1 hunks)src/Aspect.php(4 hunks)src/AspectPecl.php(1 hunks)src/ClassName.php(1 hunks)src/Exception/NotWritableException.php(1 hunks)tests/Aspect/AspectTest.php(5 hunks)tests/Aspect/Fake/src/FakeFinalClass.php(1 hunks)tests/Aspect/Fake/src/FakePeclClass.php(1 hunks)tests/Aspect/Fake/src/empty.php(1 hunks)tests/ClassNameTest.php(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- demo/run.php
- tests/Aspect/Fake/src/FakeFinalClass.php
- tests/Aspect/Fake/src/FakePeclClass.php
- tests/Aspect/Fake/src/empty.php
🧰 Additional context used
🪛 GitHub Check: sa / PHPStan
src/AspectPecl.php
[failure] 19-19:
Used function method_intercept not found.
🪛 phpstan
src/AspectPecl.php
19-19: Used function method_intercept not found.
Learn more at https://phpstan.org/user-guide/discovering-symbols
(function.notFound)
🔇 Additional comments (15)
src/Exception/NotWritableException.php (2)
7-7: LGTM!
The use statement correctly imports RuntimeException.
9-9: Semantically correct change, but note it's breaking.
The change from LogicException to RuntimeException is semantically correct since directory permissions are external factors that can vary at runtime rather than programming bugs. However, this is a breaking change that might affect catch blocks specifically targeting LogicException.
Let's verify the impact:
.github/workflows/continuous-integration-pecl.yml (1)
73-77: LGTM! Verify CODECOV_TOKEN secret availability.
The coverage report upload step is well-positioned after the PHPUnit tests and uses the latest version of the Codecov action.
tests/Aspect/AspectTest.php (6)
9-9: LGTM: New imports align with added test methods
The added imports for FakeMarker and NotWritableException are correctly placed and properly namespaced.
Also applies to: 11-11
33-37: LGTM: Well-structured exception test
Good test case that verifies the exception handling for invalid directory paths. The test follows PHPUnit best practices with proper exception expectation.
60-64: LGTM: Clear documentation of requirements and limitations
Good documentation of PECL extension requirements and explanation of why separate process testing isn't used.
74-78: LGTM: Comprehensive test coverage
The test properly verifies both the class type preservation and method interception, which are crucial for aspect-oriented programming functionality.
107-122: LGTM: Good negative test case
Well-structured test that verifies the aspect doesn't apply when class annotations don't match. This is crucial for ensuring aspects only apply where intended.
81-92: Verify final class behavior across PHP versions
Good test case for final class interception. However, final class behavior might vary across PHP versions.
✅ Verification successful
Final class interception test is well-implemented
The test case is appropriate and sufficient because:
- It verifies interception on a simple final class that matches the real-world use case
- The codebase contains many final classes (both standalone and inherited), and this test covers the critical path
- The test depends on
testWeavewhich sets up the necessary interception, making the test sequence logical - The PHP version and extension requirements are properly specified
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any other final classes in the codebase that should be tested
ast-grep --pattern 'final class $_ { $$$ }'
Length of output: 45
Script:
#!/bin/bash
# Let's try a different approach to find final classes
# First, let's use ripgrep to find final class declarations
rg "final\s+class" -l
# Then let's also check the test file content to understand the context better
cat tests/Aspect/AspectTest.php
# And let's check for the FakeFinalClass implementation
fd FakeFinalClass.php
Length of output: 5408
Script:
#!/bin/bash
# Let's check the implementation of FakeFinalClass
cat tests/Aspect/Fake/src/FakeFinalClass.php
# And let's check if there are any other test cases using final classes
rg "final\s+class.*extends" -A 5
Length of output: 4396
tests/ClassNameTest.php (2)
1-45: Well-structured test setup and teardown!
The test class follows PHPUnit best practices with proper:
- Strict typing and namespace declarations
- Temporary directory management
- Clean resource handling in teardown
47-251: Comprehensive test coverage!
The test suite thoroughly covers:
- File existence checks
- Class definitions with/without namespaces
- Class modifiers (final, abstract)
- Multiple namespaces
- Anonymous classes
- Commented code
- Interfaces and traits
- Whitespace handling
src/Aspect.php (4)
27-27: Enhanced Type Safety with MatcherConfigList
Great job defining MatcherConfigList using @psalm-type. This enhances type safety and aids static analysis tools like Psalm.
43-43: Updated Property Type Improves Clarity
Updating the $matchers property to use MatcherConfigList improves code clarity and enforces type correctness.
83-83: Specify non-empty-string for Parameter Type
Good use of non-empty-string in the @param annotation for $classDir. This provides better type checking and avoids potential issues with empty strings.
89-89: Verify the Availability of AspectPecl Class
Ensure that the AspectPecl class is properly included and autoloaded, as it is now being used to perform aspect weaving.
Run the following script to confirm that the AspectPecl class exists in the codebase:
✅ Verification successful
Let me verify if the class is properly included by checking for any import statements in the file where it's being used.
Let me check the namespace of both files to ensure they can properly reference each other.
AspectPecl Class is Properly Available
The AspectPecl class exists in src/AspectPecl.php and both classes are in the same namespace Ray\Aop, making it accessible without explicit import. The class reference is valid and properly structured.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that AspectPecl class is defined and accessible.
# Test: Search for the definition of the AspectPecl class.
# Expect: The class definition should be found in the codebase.
rg 'class AspectPecl' --type php
Length of output: 76
Script:
#!/bin/bash
# Check for import/use statements in Aspect.php
rg "use.*AspectPecl" src/Aspect.php
Length of output: 37
Script:
#!/bin/bash
# Check namespace declarations in both files
echo "=== Aspect.php namespace ==="
rg "^namespace" src/Aspect.php
echo -e "\n=== AspectPecl.php namespace ==="
rg "^namespace" src/AspectPecl.php
Length of output: 255
src/ClassName.php
Outdated
| break; | ||
| } | ||
|
|
||
| $namespace = implode('', $namespaceParts); |
There was a problem hiding this comment.
Use backslash as separator when building the namespace
When concatenating namespace parts, the separator in implode should be '\\' to correctly form the namespace. Without the backslash, the namespace segments will be joined without any separators, leading to an incorrect fully qualified class name.
Apply this diff to fix the issue:
-$namespace = implode('', $namespaceParts);
+$namespace = implode('\\', $namespaceParts);📝 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.
| $namespace = implode('', $namespaceParts); | |
| $namespace = implode('\\', $namespaceParts); |
Refactor namespace extraction logic in ClassName Replaced inline namespace part accumulation with more structured checking for T_NAME_QUALIFIED and T_STRING tokens. Added type annotations for improved static analysis and comprehension. This refactor simplifies and clarifies the logic for extracting fully qualified class names from files.
The check for the 'rayaop' extension has been removed from the weave method. This allows the method to proceed without verifying the extension, streamlining the code and possibly deferring the responsibility of extension checks to another part of the application.
The @var type annotations for $namespace and $className have been streamlined to improve code readability and reduce clutter. This maintains type safety while simplifying the code.
Update the function_exists check to include a leading backslash to ensure it correctly identifies the PECL Ray.Aop extension's method_intercept function. This change ensures accurate detection and proper functionality of the interception logic.
Simplify the class name extraction by introducing helper methods for token parsing. This improves readability and maintainability of the code. Added Psalm type annotations for better static analysis.
The assert function import in ClassName.php was not being used and has been removed to clean up the code. This helps maintain clarity and prevent potential confusion.
Consolidated the Composer dependency installation into a single step by removing the separate PHPUnit installation. This reduces redundancy and makes the workflow configuration cleaner.
Moved Fake classes from 'tests/Aspect/Fake/src' to 'tests/Aspect/Fake' to simplify directory structure and maintain consistency. Updated namespace declarations and adjusted file paths in related autoload and test files accordingly.
Upgraded phpdocumentor/type-resolver from 1.8.2 to 1.9.0 in composer.lock. This update includes adjustments to dependencies and the reference URL to the repository. This ensures compatibility with the latest features and fixes provided in the new version.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
tests/Aspect/Fake/FakePeclClass.php (1)
7-13: Consider adding docblock comments.While this is a test fixture, adding PHPDoc comments would improve code documentation and help other developers understand the purpose of this fake class in the testing context.
+/** + * Fake class for testing PECL aspect functionality + */ class FakePeclClass { + /** + * Test method that returns a fixed string + */ public function myMethod(): string { return 'original'; } }tests/Aspect/AspectTest.php (3)
32-36: Consider making the invalid directory test more platform-independent.While the test correctly verifies the NotWritableException, using a hardcoded path
'/__INVALID_DIR__'might not work consistently across different operating systems.Consider using
sys_get_temp_dir()with an invalid subdirectory or creating a temporary directory with restricted permissions:- new Aspect('/__INVALID_DIR__'); + $invalidDir = sys_get_temp_dir() . '/ray_aop_' . uniqid(); + mkdir($invalidDir); + chmod($invalidDir, 0444); // Read-only + try { + new Aspect($invalidDir); + } finally { + chmod($invalidDir, 0777); + rmdir($invalidDir); + }
80-91: Add explicit verification of final class.While the test verifies the interception behavior, it should also explicitly verify that FakeFinalClass is indeed final.
Add an assertion to verify the class is final:
public function testWeaveFinalClass(): void { + $reflection = new \ReflectionClass(FakeFinalClass::class); + $this->assertTrue($reflection->isFinal(), 'Class should be final'); $myClass = new FakeFinalClass(); $result = $myClass->myMethod();
Version constraint mismatch detected between test requirements and composer.json
The test requires PHP 8.1 and the rayaop extension, but composer.json allows PHP ^7.2 || ^8.0 and doesn't list the rayaop extension as a requirement. This could lead to installation and test execution issues.
- Update composer.json to require PHP ^8.1
- Add ext-rayaop as a required extension in composer.json
- Consider adding a note in README about the PECL extension requirement
🔗 Analysis chain
Line range hint
59-78: LGTM! Consider adding version constraint verification.The test is well-structured and properly verifies both the class type and interception behavior. The comment about separate processes is helpful.
Let's verify the PHP and extension version constraints:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify PHP version and PECL extension requirements # Check composer.json for PHP version constraint echo "Checking PHP version constraint..." cat composer.json | jq -r '.require.php' # Check if rayaop extension is listed in requirements echo "Checking rayaop extension requirement..." cat composer.json | jq -r '.require | keys[] | select(contains("ext-rayaop"))'Length of output: 304
Script:
#!/bin/bash # Let's check if there are any other version constraints in the test file or its parent directories # Check for phpunit.xml configuration files that might have version requirements echo "Checking PHPUnit configuration files..." fd phpunit.xml --exec cat {} # Check the test file and its directory for any additional version constraints echo -e "\nChecking test file and directory..." rg -A 2 -B 2 "@requires" "tests/Aspect/"Length of output: 2049
src/AspectPecl.php (1)
110-110: Consider removing redundant function existence checkSince the constructor already ensures the 'rayaop' extension is loaded, which provides the
method_interceptfunction, theassert(function_exists('\method_intercept'))may be redundant.Apply this diff to remove the redundant check:
private function applyInterceptors(array $boundInterceptors): void { $dispatcher = new PeclDispatcher($boundInterceptors); - assert(function_exists('\method_intercept')); // PECL Ray.Aop extension foreach ($boundInterceptors as $className => $methods) { $methodNames = array_keys($methods);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
vendor-bin/tools/composer.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
.github/workflows/continuous-integration-pecl.yml(3 hunks)composer.json(1 hunks)phpcs.xml(1 hunks)phpstan-baseline.neon(1 hunks)phpstan.neon(1 hunks)src/AspectPecl.php(1 hunks)src/ClassName.php(1 hunks)tests/Aspect/AspectTest.php(5 hunks)tests/Aspect/Fake/FakeFinalClass.php(1 hunks)tests/Aspect/Fake/FakeMyClass.php(1 hunks)tests/Aspect/Fake/FakePeclClass.php(1 hunks)tests/bootstrap_pecl_test.php(1 hunks)
✅ Files skipped from review due to trivial changes (5)
- phpstan-baseline.neon
- phpstan.neon
- tests/Aspect/Fake/FakeFinalClass.php
- tests/Aspect/Fake/FakeMyClass.php
- tests/bootstrap_pecl_test.php
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/continuous-integration-pecl.yml
- src/ClassName.php
🔇 Additional comments (2)
tests/Aspect/Fake/FakePeclClass.php (1)
1-6: LGTM! Well-structured file setup.
The file follows PSR-4 standards with proper namespace declaration and strict typing enabled.
phpcs.xml (1)
41-41: Verify the necessity of disabling explicit assertions.
The exclusion is properly placed within the Doctrine ruleset section. However, could you clarify why explicit assertions need to be disabled? This might impact type safety, especially in the context of PECL code extraction.
Let's check if there are any implicit assertions in the codebase that necessitate this exclusion:
✅ Verification successful
Disable explicit assertions rule is justified
The codebase extensively uses assert() statements for runtime type checking, particularly in test files and core functionality. These assertions serve as internal validation mechanisms rather than type declarations. Key examples:
- Runtime instance checks:
assert($mock instanceof FakeMock) - Class existence validation:
assert(class_exists($class)) - Callable verification:
assert(is_callable($callable)) - Property existence checks:
assert(property_exists($mock, 'bindings'))
These assertions are used appropriately for runtime validation and debugging, making the exclusion of the RequireExplicitAssertion rule reasonable.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for potential implicit assertions in the codebase
# that might have triggered this rule.
# Look for assert statements without explicit type checks
rg -g '*.php' 'assert\([^)]*\)' -A 2 -B 2
# Look for PHPUnit assertions that might be affected
rg -g '*/tests/*.php' '\$this->assert[A-Z][a-zA-Z]*\(' -A 2 -B 2
Length of output: 16850
A type hint comment was added to the $tokens variable in ClassName.php. This helps to improve code clarity and assists static analysis tools like PHPStan in understanding the type of this variable.
Simplify and optimize the parsing of namespaces and class names. Replaced conditional checks with switch cases, removed debug code, and centralized whitespace skipping in a helper function.
Inserted a `var_dump($tokens)` to output token data for debugging purposes. This will aid in understanding the token parsing behavior and help identify potential issues in the tokenization process.
Inserted var_dump() calls to display the namespace and namespaceResult variables. This will assist in debugging issues related to namespace parsing in the ClassName.php file.
Remove ClassName and its test file, replacing it with a new ClassList class that simplifies class extraction from directory paths. Updated AspectPecl to use ClassList for iterating PHP files more efficiently.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (6)
tests/ClassListTest.php (2)
7-16: Organize use statements and add missing imports.The use statements should be organized by type and the
substrfunction should be imported.Apply this diff:
use PHPUnit\Framework\TestCase; + use function file_exists; use function file_put_contents; use function glob; use function iterator_to_array; use function mkdir; use function rmdir; +use function substr; use function sys_get_temp_dir; use function uniqid; use function unlink;🧰 Tools
🪛 GitHub Check: cs / Coding Standards
[failure] 7-7:
Header blocks must be separated by a single blank line
[failure] 8-8:
Expected 1 line between different types of use statement, found 0.
23-29: Simplify conditional logic in setUp method.The conditional nesting can be simplified using early return pattern.
Apply this diff:
protected function setUp(): void { $this->tempDir = sys_get_temp_dir() . '/class_list_test_' . uniqid(); - if (! file_exists($this->tempDir)) { - mkdir($this->tempDir); - } + if (file_exists($this->tempDir)) { + return; + } + mkdir($this->tempDir); }🧰 Tools
🪛 GitHub Check: cs / Coding Standards
[failure] 26-26:
Use early exit to reduce code nesting.src/AspectPecl.php (2)
7-8: Remove unused imports to clean up codeThe classes
RecursiveDirectoryIterator,RecursiveIteratorIterator, andSplFileInfoare imported but not used in this file. Consider removing these unused imports to improve code clarity.Also applies to: 12-12
🧰 Tools
🪛 GitHub Check: cs / Coding Standards
[failure] 7-7:
Type RecursiveDirectoryIterator is not used in this file.
[failure] 8-8:
Type RecursiveIteratorIterator is not used in this file.
19-19: Ensuremethod_interceptfunction is recognized by static analysis toolsThe function
method_interceptis reported as not found by static analysis tools because it is provided by therayaopextension. Consider adding a stub or updating the static analysis configuration to recognize extension functions to prevent false positives during analysis.🧰 Tools
🪛 GitHub Check: sa / PHPStan
[failure] 19-19:
Used function method_intercept not found.🪛 phpstan
19-19: Used function method_intercept not found.
Learn more at https://phpstan.org/user-guide/discovering-symbols(function.notFound)
src/ClassList.php (2)
67-69: Resolve return type mismatch ingetIteratormethodThe static analysis tools report a mismatch between the declared return type
Generator<class-string>and the inferred return typeGenerator<int, string>. This occurs because the generator yields strings, but it's declared to yieldclass-string. Ensure that the yielded values are correctly recognized asclass-string.You can add a docblock comment to assert that
$classNameis aclass-stringbefore yielding it:yield $className; } + /** @var Generator<class-string> */ }Or add an annotation directly above the yield statement:
if ($className === null) { continue; } + /** @var class-string $className */ yield $className;🧰 Tools
🪛 GitHub Check: sa / Psalm
[failure] 67-67:
MoreSpecificReturnType: The declared return type 'Generator<mixed, class-string, mixed, mixed>' for Ray\Aop\ClassList::getIterator is more specific than the inferred return type 'Generator<int, string, mixed, void>'
85-85: Ensure$classNameis recognized asclass-stringTo help static analysis tools understand that
$classNameis indeed aclass-string, you can add an explicit assertion or type annotation before theyieldstatement.Apply this diff:
if ($className === null) { continue; } +/** @var class-string $className */ yield $className;🧰 Tools
🪛 GitHub Check: sa / PHPStan
[failure] 85-85:
Generator expects value type class-string, string given.🪛 phpstan
85-85: Generator expects value type class-string, string given.
(generator.valueType)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
src/AspectPecl.php(1 hunks)src/ClassList.php(1 hunks)tests/ClassListTest.php(1 hunks)
🧰 Additional context used
🪛 GitHub Check: sa / PHPStan
src/AspectPecl.php
[failure] 19-19:
Used function method_intercept not found.
src/ClassList.php
[failure] 29-29:
Parameter #1 $code of function token_get_all expects string, string|false given.
[failure] 85-85:
Generator expects value type class-string, string given.
tests/ClassListTest.php
[failure] 33-33:
Argument of an invalid type array<int, string>|false supplied for foreach, only iterables are supported.
🪛 GitHub Check: cs / Coding Standards
src/AspectPecl.php
[failure] 7-7:
Type RecursiveDirectoryIterator is not used in this file.
[failure] 8-8:
Type RecursiveIteratorIterator is not used in this file.
[failure] 12-12:
Type SplFileInfo is not used in this file.
tests/ClassListTest.php
[failure] 7-7:
Header blocks must be separated by a single blank line
[failure] 8-8:
Expected 1 line between different types of use statement, found 0.
[failure] 26-26:
Use early exit to reduce code nesting.
[failure] 33-33:
Expected 1 line after "foreach", found 0.
[failure] 60-60:
Function substr() should not be referenced via a fallback global name, but via a use statement.
[failure] 98-98:
Function substr() should not be referenced via a fallback global name, but via a use statement.
🪛 phpstan
src/AspectPecl.php
19-19: Used function method_intercept not found.
Learn more at https://phpstan.org/user-guide/discovering-symbols
(function.notFound)
src/ClassList.php
29-29: Parameter #1 $code of function token_get_all expects string, string|false given.
(argument.type)
85-85: Generator expects value type class-string, string given.
(generator.valueType)
tests/ClassListTest.php
33-33: Argument of an invalid type array<int, string>|false supplied for foreach, only iterables are supported.
(foreach.nonIterable)
🪛 GitHub Check: sa / Psalm
src/ClassList.php
[failure] 67-67:
MoreSpecificReturnType: The declared return type 'Generator<mixed, class-string, mixed, mixed>' for Ray\Aop\ClassList::getIterator is more specific than the inferred return type 'Generator<int, string, mixed, void>'
🔇 Additional comments (3)
tests/ClassListTest.php (2)
39-103: Test coverage looks comprehensive.
The test cases cover important scenarios:
- No class in file
- Available vs unavailable classes
- Empty directory
- Classes with declare statements
Good job on the test coverage!
🧰 Tools
🪛 GitHub Check: cs / Coding Standards
[failure] 60-60:
Function substr() should not be referenced via a fallback global name, but via a use statement.
[failure] 98-98:
Function substr() should not be referenced via a fallback global name, but via a use statement.
60-60: Consider alternatives to eval() for class loading.
Using eval() in tests can be dangerous and make debugging difficult. Consider using class autoloading or a separate test bootstrap file to define these test classes.
Let's check if there are other instances of eval() usage in tests:
Also applies to: 98-98
✅ Verification successful
Let me gather more context about how these eval() calls are being used in the tests.
Let me check the full test class to understand the testing context better.
Let me check what ClassList.php does to understand if eval is really necessary for testing.
The use of eval() is justified in this specific testing context
The eval() usage in tests is actually necessary and safe here. The ClassList class is designed to find and return fully qualified class names (FQCNs) from PHP files, but only if those classes are actually loaded/available. The eval() calls in tests are required to simulate this behavior by making the test classes available during runtime, which is essential for testing the class_exists() check in ClassList::from().
Alternative approaches like autoloading wouldn't work here because:
- The test files are created dynamically in a temporary directory
- The class availability check is a core part of the functionality being tested
- The eval'd code is strictly controlled test data
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for eval usage in test files
rg 'eval\(' 'tests/'
Length of output: 133
Script:
#!/bin/bash
# Get more context around the eval calls
rg -B 5 -A 5 'eval\(' tests/ClassListTest.php
Length of output: 656
Script:
#!/bin/bash
# Get the full test class content
cat tests/ClassListTest.php
Length of output: 2487
Script:
#!/bin/bash
# Get the ClassList implementation
cat src/ClassList.php
Length of output: 2162
🧰 Tools
🪛 GitHub Check: cs / Coding Standards
[failure] 60-60:
Function substr() should not be referenced via a fallback global name, but via a use statement.
src/AspectPecl.php (1)
40-44: Typo in parameter name $mathcers remains
The parameter name $mathcers is misspelled; it should be $matchers to match the context and maintain consistency.
Introduce a new demo script `05-pecl.php` under the `demo` directory. This script sets up an Aspect-Oriented Programming (AOP) configuration and demonstrates error handling within a billing service.
Updated `setUp` method to return early if temp directory exists and refactored `ClassList` to handle class existence checks. Removed unused imports from `AspectPecl` for code cleanliness.
Created a phpstan-baseline.neon file to suppress specific errors related to the method_intercept function. Updated phpstan.neon to include this baseline configuration for improved analysis consistency.
Inserted var_dump statements to log tokens for easier debugging when parsing namespaces. This will help identify issues with token processing and aid in troubleshooting namespace extraction.
Removed debugging `var_dump` statements and improved namespace collection. Now using `implode` and `array_filter` to build the namespace, ensuring cleaner and more reliable code execution.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (3)
phpstan-baseline.neon (1)
1-6: Consider adding a comment explaining why this error is ignored.Since this is ignoring a "function not found" error for a PECL extension function, it would be helpful to add a comment explaining this context for future maintainers.
parameters: ignoreErrors: - + # Ignore PHPStan error for method_intercept as it's provided by the PECL extension message: "#^Used function method_intercept not found\\.$#" count: 1 path: src/AspectPecl.phpdemo/05-pecl.php (1)
15-15: Consider making the temporary directory configurable.The temporary directory path is hardcoded. Consider making it configurable through environment variables or configuration files for better flexibility across different environments.
-$aspect = new Aspect(__DIR__ . '/tmp'); +$tmpDir = getenv('AOP_TMP_DIR') ?: __DIR__ . '/tmp'; +$aspect = new Aspect($tmpDir);src/ClassList.php (1)
72-95: Add directory validation.The iterator implementation should validate the directory path before attempting to traverse it.
Consider adding these checks:
public function getIterator(): Generator { + if (!is_dir($this->directory) || !is_readable($this->directory)) { + throw new \RuntimeException(sprintf('Directory "%s" is not readable or does not exist', $this->directory)); + } + $files = new RecursiveIteratorIterator( new RecursiveDirectoryIterator($this->directory) );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
demo/05-pecl.php(1 hunks)phpstan-baseline.neon(1 hunks)phpstan.neon(1 hunks)src/AspectPecl.php(1 hunks)src/ClassList.php(1 hunks)tests/ClassListTest.php(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- phpstan.neon
🚧 Files skipped from review as they are similar to previous changes (2)
- src/AspectPecl.php
- tests/ClassListTest.php
🧰 Additional context used
🪛 GitHub Check: sa / Psalm
src/ClassList.php
[failure] 44-44:
ForbiddenCode: Unsafe var_dump
[failure] 45-45:
ForbiddenCode: Unsafe var_dump
[failure] 46-46:
ForbiddenCode: Unsafe var_dump
🪛 GitHub Check: cs / Coding Standards
src/ClassList.php
[failure] 17-17:
Header blocks must not contain blank lines
[failure] 19-19:
Expected 0 lines between same types of use statement, found 1.
[failure] 19-19:
Header blocks must be separated by a single blank line
[failure] 20-20:
Expected 1 line between different types of use statement, found 0.
🔇 Additional comments (4)
phpstan-baseline.neon (1)
1-6: LGTM! The PHPStan baseline configuration is correctly structured.
The configuration properly ignores the expected "function not found" error for the PECL method_intercept function.
demo/05-pecl.php (2)
1-13: LGTM! File setup follows best practices.
The file setup is well-structured with appropriate declarations and imports.
15-21: Verify directory permissions and security implications.
Since this code handles file system operations, we should verify:
- Directory permissions are properly set
- No sensitive data is written to temporary files
- Source directory access is restricted
✅ Verification successful
Let me try a different approach to gather the necessary information.
Directory permissions and security concerns have been verified
The verification shows:
- Both
demo/srcanddemo/tmpdirectories have standard permissions (755) for directories and files (644) - No sensitive file operations found in the demo code
WeekendBlockerimplementation only checks the current date without file operations- The temporary directory is empty except for a
.gitkeepfile - Source files contain only business logic classes without sensitive data
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potentially sensitive files in source directory
echo "Checking for sensitive files..."
rg -l "password|secret|key|token" "demo/src"
# Check for proper file permissions in the demo directory
echo "Checking file permissions..."
find demo -type f -exec stat -c "%n %a" {} \;
Length of output: 755
Script:
#!/bin/bash
# Check if demo/src directory exists and list its contents
echo "Checking demo/src directory structure:"
ls -la demo/src 2>/dev/null || echo "demo/src directory not found"
# Check if demo/tmp directory exists and list its contents
echo -e "\nChecking demo/tmp directory structure:"
ls -la demo/tmp 2>/dev/null || echo "demo/tmp directory not found"
# Look for file operations in the codebase
echo -e "\nChecking file operation patterns:"
rg -A 2 "fopen|file_put_contents|file_get_contents" demo/
# Check WeekendBlocker implementation
echo -e "\nChecking WeekendBlocker implementation:"
rg -A 5 "class WeekendBlocker" demo/
Length of output: 1543
src/ClassList.php (1)
63-69: LGTM!
The constructor and property implementation is clean and properly typed.
Added specific token constants for better readability and maintainability. Refactored the condition to check for namespace tokens using these new constants. Updated function imports to better match coding style.
Replaces the short arrow function with a traditional anonymous function in the namespace collection logic. This change improves readability and maintains consistency throughout the codebase.
Replaced the usage of token_get_all with preg_match and string operations for extracting namespaces and class names. This eliminates reliance on specific token constants and simplifies the function logic to improve readability and maintainability.
Replaced the usage of the deprecated `str_contains` function with `strpos` to ensure compatibility and proper function operation. This change ensures the correct handling of files that contain PHP tags.
The "Run additional script" step was removed from the CI workflow as it was not essential for the build process. This clean-up helps streamline the workflow and reduce potential sources of failure.
Swapped out the str_contains function for strpos to improve compatibility and performance. This change ensures broad consistency across different PHP versions, aligning with project requirements.
Group several CI steps into a single command to streamline the process. Execute PHPUnit tests under Valgrind, ensuring memory leaks are checked, and upload comprehensive debug logs including Valgrind, coverage reports, and core dumps. Enhance workflow to always run critical debugging steps regardless of previous outcomes.
Added multiple necessary PHP extensions to the CI setup to facilitate testing and building. Improved the uploading of debug logs with PHP version-specific names and removed redundant core dump and segmentation fault handling to streamline workflow.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
.github/workflows/continuous-integration-pecl.yml (4)
70-70: Clean up trailing whitespace.Remove trailing spaces from lines 70, 85, and 116 to maintain consistent formatting.
- EOF + EOF - --coverage-clover=coverage.xml || true + --coverage-clover=coverage.xml || true - +Also applies to: 85-85, 116-116
🧰 Tools
🪛 yamllint
[error] 70-70: trailing spaces
(trailing-spaces)
80-84: Enhance error handling for coverage report generation.While the
|| trueprevents the step from failing, we should explicitly check if the coverage report was generated successfully.- --coverage-clover=coverage.xml || true + --coverage-clover=coverage.xml || true + + if [ ! -f coverage.xml ]; then + echo "Warning: Coverage report was not generated" + fi
91-99: Add retention period for artifacts.Consider adding a retention period to manage storage space efficiently.
with: name: debug-logs-php${{ matrix.php-version }} path: | valgrind.log coverage.xml core* if-no-files-found: warn + retention-days: 14
117-122: Enhance status output formatting.Consider structuring the output for better readability in GitHub Actions logs.
- # Check for critical issues in Valgrind log if [ -f valgrind.log ]; then - echo "=== Valgrind Error Summary ===" - grep "ERROR SUMMARY:" valgrind.log || true - grep "definitely lost:" valgrind.log || true - grep "indirectly lost:" valgrind.log || true + echo "::group::Valgrind Error Summary" + { + echo "• Error Summary: $(grep 'ERROR SUMMARY:' valgrind.log || echo 'Not found')" + echo "• Memory Definitely Lost: $(grep 'definitely lost:' valgrind.log || echo 'Not found')" + echo "• Memory Indirectly Lost: $(grep 'indirectly lost:' valgrind.log || echo 'Not found')" + } | tee valgrind-summary.txt + echo "::endgroup::" fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
.github/workflows/continuous-integration-pecl.yml(4 hunks)
🧰 Additional context used
🪛 yamllint
.github/workflows/continuous-integration-pecl.yml
[error] 70-70: trailing spaces
(trailing-spaces)
[error] 85-85: trailing spaces
(trailing-spaces)
[error] 116-116: trailing spaces
(trailing-spaces)
🔇 Additional comments (3)
.github/workflows/continuous-integration-pecl.yml (3)
15-15: LGTM! Good addition of fail-fast strategy.
Setting fail-fast: false is a good practice as it ensures tests run on all PHP versions even if one fails, providing a complete picture of compatibility across versions.
51-68: LGTM! Comprehensive Valgrind suppression configuration.
The suppression rules for PHP startup and Zend memory manager leaks are well-defined and necessary for accurate memory leak detection.
25-25: Verify required PHP extensions.
The added extensions look good for XML and tokenization support. However, let's verify if these are all the required extensions for the PECL module.
Also applies to: 34-34
✅ Verification successful
Let me try to find the PHP extension dependencies by searching through the codebase more thoroughly.
Required PHP extensions are correctly configured
The workflow's PHP extensions configuration is appropriate. From the codebase analysis:
- Required extensions from composer.json:
hash,tokenizer - Additional extensions in workflow:
mbstring,intl,dom,json,libxml,xml,xmlwriter
The workflow includes all required extensions plus additional ones needed for XML handling and internationalization support, which are commonly needed dependencies for PHP projects dealing with annotations and AST operations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check extension dependencies in the PECL module
grep -r "depends\|requires" ext-rayaop/config* || echo "No explicit dependencies found"
Length of output: 173
Script:
#!/bin/bash
# Search for config.m4 or config.w32 files which typically declare dependencies
fd "config\.(m4|w32)$"
# Search for extension dependencies in PHP files
rg -i "extension.*require"
# Look for composer.json which might list PHP extension requirements
fd "composer.json" --exec cat {}
Length of output: 3149
Simplified ClassListTest by removing temporary directory fixtures, and refactored ClassList to use RegexIterator for filtering PHP files. Additionally, extracted patterns to constants for better readability and maintainability.
Included a step to execute a demo script with Valgrind, enabling thorough memory leak checks and origin tracking. This addition helps in improving the robustness of the code by identifying and addressing potential memory issues early in the development cycle.
Annotated the AspectPecl and Aspect classes with `@codeCoverageIgnore` to exclude specific sections from code coverage reports. This change helps streamline testing by excluding non-critical paths.
Refactor the `getIterator` method in `ClassList` to streamline the class existence check by using a guard clause. This improves readability and maintains functionality. Also, added missing import statements in the `ClassListTest` to rectify the usage of `class_exists` and `iterator_to_array`.
Renamed the method from `from` to `getClassName` to improve clarity on its purpose. Also, added a code coverage ignore comment for the case where file content could not be retrieved.
Summary by CodeRabbit
Release Notes
New Features
AspectPecl,ClassList, and additional test classes for enhanced functionality.NotWritableExceptionfor improved error management.FakeFinalClass,FakePeclClass, andFakeNonFinalClassfor testing purposes.Bug Fixes
Documentation
Chores