Enable PHP 8.4 comat #59
Conversation
WalkthroughThe pull request introduces changes to the Changes
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 (
|
Replaced BetterReflection with native PHP directory and token functions for class discovery in `ClassesInDirectories.php`. This simplifies the dependency tree by removing the `roave/better-reflection` package from `composer.json`.
![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 (2)
src/ClassesInDirectories.php (2)
22-22: Add trailing comma in multi-line function callAccording to coding standards, multi-line function calls should have a trailing comma after the last parameter. This improves the readability and simplifies future code modifications.
Apply this diff to fix:
$iterator = new RecursiveIteratorIterator( - new RecursiveDirectoryIterator($directory) + new RecursiveDirectoryIterator($directory), );🧰 Tools
🪛 GitHub Check: cs / Coding Standards
[failure] 22-22:
Multi-line function calls must have a trailing comma after the last parameter.
59-59: Add space after the NOT operatorCoding standards require a space after the NOT operator
!. Please add a space to conform to the style guidelines.Apply this diff to fix:
- if (!isset($tokens[$i][0])) { + if (! isset($tokens[$i][0])) {🧰 Tools
🪛 GitHub Check: cs / Coding Standards
[failure] 59-59:
Expected 1 space after NOT operator; 0 found
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
composer.json(0 hunks)src/ClassesInDirectories.php(1 hunks)
💤 Files with no reviewable changes (1)
- composer.json
🧰 Additional context used
🪛 GitHub Check: sa / Psalm
src/ClassesInDirectories.php
[failure] 25-25:
MixedAssignment: Unable to determine the type that $file is being assigned to
🪛 GitHub Check: sa / PHPStan
src/ClassesInDirectories.php
[failure] 18-18:
PHPDoc tag @param for parameter $directories with type array<int, string> is incompatible with native type string.
🪛 GitHub Check: cs / Coding Standards
src/ClassesInDirectories.php
[failure] 15-15:
Incorrect annotations group.
[failure] 22-22:
Multi-line function calls must have a trailing comma after the last parameter.
[failure] 39-39:
Use early exit to reduce code nesting.
[failure] 39-39:
Function class_exists() should not be referenced via a fallback global name, but via a use statement.
[failure] 39-39:
Function interface_exists() should not be referenced via a fallback global name, but via a use statement.
[failure] 46-46:
Usage of short nullable type hint in "?string" is disallowed.
[failure] 48-48:
Function file_get_contents() should not be referenced via a fallback global name, but via a use statement.
[failure] 55-55:
Function token_get_all() should not be referenced via a fallback global name, but via a use statement.
[failure] 56-56:
Function count() should not be referenced via a fallback global name, but via a use statement.
[failure] 59-59:
Expected 1 space after NOT operator; 0 found
🪛 phpstan
src/ClassesInDirectories.php
18-18: PHPDoc tag @param for parameter $directories with type array<int, string> is incompatible with native type string.
(parameter.phpDocType)
src/ClassesInDirectories.php
Outdated
| } | ||
| } | ||
|
|
||
| private static function getClassFromFile(string $filePath): ?string |
There was a problem hiding this comment.
Avoid short nullable type hints
Usage of short nullable type hints like ?string is disallowed by coding standards. Please use the explicit string|null syntax instead.
Apply this diff to fix:
- private static function getClassFromFile(string $filePath): ?string
+ private static function getClassFromFile(string $filePath): string|null📝 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.
| private static function getClassFromFile(string $filePath): ?string | |
| private static function getClassFromFile(string $filePath): string|null |
🧰 Tools
🪛 GitHub Check: cs / Coding Standards
[failure] 46-46:
Usage of short nullable type hint in "?string" is disallowed.
Introduced `@phpstan-ignore-line` and `@psalm-suppress` to suppress static analysis warnings. Also added an explicit `assert` to ensure type checking for class and interface existence. These changes enhance code safety and maintainability by preventing potential runtime errors.
Separated class and namespace extraction into distinct methods, improving readability and maintainability. Added assertions to ensure token types and values during extraction.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
src/ClassesInDirectories.php (1)
36-65: Remove redundant assertionThe assertion on line 60 is redundant as the same condition is already checked in the if statement on line 56.
Apply this diff to remove the redundancy:
if (! class_exists($className) && ! interface_exists($className)) { continue; } - assert(class_exists($className) || interface_exists($className)); - yield $className;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/ClassesInDirectories.php(1 hunks)
🔇 Additional comments (1)
src/ClassesInDirectories.php (1)
8-25: LGTM: Well-organized imports
The imports are properly organized and follow PSR-12 standards, with clear separation between class imports, function imports, and constants.
src/ClassesInDirectories.php
Outdated
| public static function list(string ...$directories): Generator // @phpstan-ignore-line | ||
| { |
There was a problem hiding this comment.
Remove @phpstan-ignore and fix PHPDoc
The return type change to Generator is good, but there are two issues to address:
- The
@phpstan-ignore-lineannotation suggests an unresolved type issue - The PHPDoc parameter type
list<string>is inconsistent with the variadic signature
Apply this diff to fix:
/**
- * @param list<string> $directories
+ * @param string ...$directories
* @return Generator<int, class-string>
*/
- public static function list(string ...$directories): Generator // @phpstan-ignore-line
+ public static function list(string ...$directories): Generator📝 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.
| public static function list(string ...$directories): Generator // @phpstan-ignore-line | |
| { | |
| /** | |
| * @param string ...$directories | |
| * @return Generator<int, class-string> | |
| */ | |
| public static function list(string ...$directories): Generator |
| private static function getClassFromFile(string $filePath): string|null | ||
| { | ||
| $content = file_get_contents($filePath); | ||
| if ($content === false) { | ||
| return null; | ||
| } | ||
|
|
||
| $tokens = token_get_all($content); | ||
| /** @var array<int, mixed> $tokens */ | ||
|
|
||
| $namespace = self::extractNamespace($tokens); | ||
| $class = self::extractClassName($tokens); | ||
|
|
||
| if ($class === null) { | ||
| return null; | ||
| } | ||
|
|
||
| if ($namespace === null) { | ||
| return $class; | ||
| } | ||
|
|
||
| return $namespace . '\\' . $class; | ||
| } |
There was a problem hiding this comment.
Add error handling for token_get_all
While the method handles file reading errors, it should also handle potential token_get_all failures.
Apply this diff to add error handling:
$content = file_get_contents($filePath);
if ($content === false) {
return null;
}
- $tokens = token_get_all($content);
+ $tokens = @token_get_all($content);
+ if (! is_array($tokens)) {
+ return null;
+ }📝 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.
| private static function getClassFromFile(string $filePath): string|null | |
| { | |
| $content = file_get_contents($filePath); | |
| if ($content === false) { | |
| return null; | |
| } | |
| $tokens = token_get_all($content); | |
| /** @var array<int, mixed> $tokens */ | |
| $namespace = self::extractNamespace($tokens); | |
| $class = self::extractClassName($tokens); | |
| if ($class === null) { | |
| return null; | |
| } | |
| if ($namespace === null) { | |
| return $class; | |
| } | |
| return $namespace . '\\' . $class; | |
| } | |
| private static function getClassFromFile(string $filePath): string|null | |
| { | |
| $content = file_get_contents($filePath); | |
| if ($content === false) { | |
| return null; | |
| } | |
| $tokens = @token_get_all($content); | |
| if (! is_array($tokens)) { | |
| return null; | |
| } | |
| /** @var array<int, mixed> $tokens */ | |
| $namespace = self::extractNamespace($tokens); | |
| $class = self::extractClassName($tokens); | |
| if ($class === null) { | |
| return null; | |
| } | |
| if ($namespace === null) { | |
| return $class; | |
| } | |
| return $namespace . '\\' . $class; | |
| } |
src/ClassesInDirectories.php
Outdated
| /** @param array<int, mixed> $tokens */ | ||
| private static function extractClassName(array $tokens): string|null | ||
| { | ||
| /** @var list<string> $directories */ | ||
| $sourceLocator = new AggregateSourceLocator([ | ||
| new DirectoriesSourceLocator( | ||
| $directories, | ||
| (new BetterReflection())->astLocator(), | ||
| ), | ||
| // ↓ required to autoload parent classes/interface from another directory than /src (e.g. /vendor) | ||
| new AutoloadSourceLocator((new BetterReflection())->astLocator()), | ||
| ]); | ||
|
|
||
| foreach ((new DefaultReflector($sourceLocator))->reflectAllClasses() as $class) { | ||
| $className = $class->getName(); | ||
| assert(class_exists($className) || interface_exists($className)); | ||
|
|
||
| yield $className; | ||
| foreach ($tokens as $index => $token) { | ||
| assert(is_array($token)); | ||
| if ($token[0] !== T_CLASS && $token[0] !== T_INTERFACE) { | ||
| continue; | ||
| } | ||
|
|
||
| for ($j = $index + 1, $count = count($tokens); $j < $count; $j++) { | ||
| if (is_array($tokens[$j]) && $tokens[$j][0] === T_STRING) { | ||
| $string = $tokens[$j][1]; | ||
| assert(is_string($string)); | ||
|
|
||
| return $string; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return null; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add trait support and optimize class name extraction
The method should also handle trait declarations and can be optimized similarly to the namespace extraction.
Apply this diff to improve:
+use const T_TRAIT;
/** @param array<int, mixed> $tokens */
private static function extractClassName(array $tokens): string|null
{
- foreach ($tokens as $index => $token) {
- assert(is_array($token));
- if ($token[0] !== T_CLASS && $token[0] !== T_INTERFACE) {
- continue;
- }
-
- for ($j = $index + 1, $count = count($tokens); $j < $count; $j++) {
- if (is_array($tokens[$j]) && $tokens[$j][0] === T_STRING) {
- $string = $tokens[$j][1];
- assert(is_string($string));
-
- return $string;
- }
- }
+ $typeTokens = [T_CLASS, T_INTERFACE, T_TRAIT];
+ foreach ($tokens as $index => $token) {
+ if (! is_array($token) || ! in_array($token[0], $typeTokens, true)) {
+ continue;
+ }
+
+ $remaining = array_slice($tokens, $index + 1);
+ foreach ($remaining as $token) {
+ if (is_array($token) && $token[0] === T_STRING) {
+ return $token[1];
+ }
+ }
}
return null;
}📝 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.
| /** @param array<int, mixed> $tokens */ | |
| private static function extractClassName(array $tokens): string|null | |
| { | |
| /** @var list<string> $directories */ | |
| $sourceLocator = new AggregateSourceLocator([ | |
| new DirectoriesSourceLocator( | |
| $directories, | |
| (new BetterReflection())->astLocator(), | |
| ), | |
| // ↓ required to autoload parent classes/interface from another directory than /src (e.g. /vendor) | |
| new AutoloadSourceLocator((new BetterReflection())->astLocator()), | |
| ]); | |
| foreach ((new DefaultReflector($sourceLocator))->reflectAllClasses() as $class) { | |
| $className = $class->getName(); | |
| assert(class_exists($className) || interface_exists($className)); | |
| yield $className; | |
| foreach ($tokens as $index => $token) { | |
| assert(is_array($token)); | |
| if ($token[0] !== T_CLASS && $token[0] !== T_INTERFACE) { | |
| continue; | |
| } | |
| for ($j = $index + 1, $count = count($tokens); $j < $count; $j++) { | |
| if (is_array($tokens[$j]) && $tokens[$j][0] === T_STRING) { | |
| $string = $tokens[$j][1]; | |
| assert(is_string($string)); | |
| return $string; | |
| } | |
| } | |
| } | |
| return null; | |
| } | |
| /** @param array<int, mixed> $tokens */ | |
| private static function extractClassName(array $tokens): string|null | |
| { | |
| $typeTokens = [T_CLASS, T_INTERFACE, T_TRAIT]; | |
| foreach ($tokens as $index => $token) { | |
| if (! is_array($token) || ! in_array($token[0], $typeTokens, true)) { | |
| continue; | |
| } | |
| $remaining = array_slice($tokens, $index + 1); | |
| foreach ($remaining as $token) { | |
| if (is_array($token) && $token[0] === T_STRING) { | |
| return $token[1]; | |
| } | |
| } | |
| } | |
| return null; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (1)
src/ClassesInDirectories.php (1)
PHP 8.4 compatibility needs version constraint update
The package currently requires PHP
^8.1which doesn't include PHP 8.4. The version constraint should be updated to include PHP 8.4 compatibility.
- Update
composer.jsonto use"php": "^8.1 || ^8.4"or"php": "^8.1 || ~8.4.0"to explicitly declare PHP 8.4 support🔗 Analysis chain
Line range hint
1-128: Verify PHP 8.4 compatibilityThe implementation uses standard PHP features and should be compatible with PHP 8.4. However, let's verify that all dependencies in composer.json are also PHP 8.4 compatible.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check composer.json for PHP 8.4 compatibility if [ -f "composer.json" ]; then echo "Checking composer.json for PHP version constraints..." cat composer.json | grep -A 5 '"require":' fiLength of output: 366
🧰 Tools
🪛 GitHub Check: sa / Psalm
[failure] 92-92:
MixedInferredReturnType: Could not verify return type 'null|string' for Ray\MediaQuery\ClassesInDirectories::extractNamespace
[failure] 100-100:
MixedAssignment: Unable to determine the type that $token is being assigned to
[failure] 102-102:
MixedReturnStatement: Could not infer a return type🪛 GitHub Check: cs / Coding Standards
[failure] 94-94:
Function array_search() should not be referenced via a fallback global name, but via a use statement.
[failure] 94-94:
Function array_column() should not be referenced via a fallback global name, but via a use statement.
[failure] 98-98:
Whitespace found at end of line
[failure] 99-99:
Function array_slice() should not be referenced via a fallback global name, but via a use statement.
[failure] 105-105:
Whitespace found at end of line
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/ClassesInDirectories.php(1 hunks)
🧰 Additional context used
🪛 GitHub Check: sa / Psalm
src/ClassesInDirectories.php
[failure] 92-92:
MixedInferredReturnType: Could not verify return type 'null|string' for Ray\MediaQuery\ClassesInDirectories::extractNamespace
[failure] 100-100:
MixedAssignment: Unable to determine the type that $token is being assigned to
[failure] 102-102:
MixedReturnStatement: Could not infer a return type
🪛 GitHub Check: cs / Coding Standards
src/ClassesInDirectories.php
[failure] 94-94:
Function array_search() should not be referenced via a fallback global name, but via a use statement.
[failure] 94-94:
Function array_column() should not be referenced via a fallback global name, but via a use statement.
[failure] 98-98:
Whitespace found at end of line
[failure] 99-99:
Function array_slice() should not be referenced via a fallback global name, but via a use statement.
[failure] 105-105:
Whitespace found at end of line
🔇 Additional comments (1)
src/ClassesInDirectories.php (1)
34-35:
Remove @phpstan-ignore-line annotation
The @phpstan-ignore-line annotation suggests unresolved type issues. Since the return type is correctly specified as Generator<int, class-string> in the PHPDoc, this annotation should be unnecessary.
Apply this diff to fix:
- public static function list(string ...$directories): Generator // @phpstan-ignore-line
+ public static function list(string ...$directories): GeneratorLikely invalid or redundant comment.
src/ClassesInDirectories.php
Outdated
| if (! class_exists($className) && ! interface_exists($className)) { | ||
| continue; | ||
| } | ||
|
|
||
| assert(class_exists($className) || interface_exists($className)); | ||
|
|
There was a problem hiding this comment.
Remove redundant assertion
The assertion on line 60 is redundant with the check on line 56. The condition has already been verified by the if statement.
Apply this diff to fix:
if (! class_exists($className) && ! interface_exists($className)) {
continue;
}
- assert(class_exists($className) || interface_exists($className));
-
yield $className;📝 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.
| if (! class_exists($className) && ! interface_exists($className)) { | |
| continue; | |
| } | |
| assert(class_exists($className) || interface_exists($className)); | |
| if (! class_exists($className) && ! interface_exists($className)) { | |
| continue; | |
| } | |
| yield $className; |
| $tokens = token_get_all($content); | ||
| /** @var array<int, mixed> $tokens */ | ||
|
|
There was a problem hiding this comment.
Add error handling for token_get_all
The token_get_all function can fail and return false. This case should be handled explicitly.
Apply this diff to fix:
$tokens = token_get_all($content);
+ if (! is_array($tokens)) {
+ return null;
+ }
/** @var array<int, mixed> $tokens */Committable suggestion skipped: line range outside the PR's diff.
Eliminated unnecessary assert statements that checked for array type, which are verified earlier in the code. This improves code clarity and reduces redundant operations.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/ClassesInDirectories.php(1 hunks)
🔇 Additional comments (4)
src/ClassesInDirectories.php (4)
8-30: LGTM! Well-organized imports
All necessary classes, functions, and constants are properly imported with clear organization.
79-81:
Add error handling for token_get_all
The token_get_all function can fail and return false on parsing errors. This case should be handled explicitly.
Apply this diff to fix:
$tokens = token_get_all($content);
+ if (! is_array($tokens)) {
+ return null;
+ }
/** @var array<int, mixed> $tokens */Likely invalid or redundant comment.
61-66:
Remove redundant assertion
The assertion on line 65 is redundant as the condition is already verified by the if statement on line 61.
Apply this diff to fix:
if (! class_exists($className) && ! interface_exists($className)) {
continue;
}
- assert(class_exists($className) || interface_exists($className));
-
yield $className;Likely invalid or redundant comment.
Line range hint 1-139: Code is PHP 8.4 compatible
The implementation uses modern PHP features and follows best practices. The changes from reflection-based to file parsing approach should work well with PHP 8.4.
Let's verify PHP 8.4 compatibility:
✅ Verification successful
Let's analyze the specific files that might contain PHP 8.4 compatibility issues by checking the actual usage of potentially problematic patterns.
Let's check the actual assertions in the code since our previous search patterns might have been too strict.
Based on the comprehensive analysis of the codebase, I can now generate the final response:
Code is PHP 8.4 compatible - No deprecated features found
The code in ClassesInDirectories.php and the entire codebase shows:
- Proper use of modern PHP features like union types (e.g.,
string|null) - Correct usage of attributes and strict types declarations
- No usage of deprecated features like
each(),create_function(), or string assertions - All
assert()calls are used correctly with type checks, not string evaluations - Modern PHP 8+ features like readonly properties and attributes are used appropriately
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any PHP 8.4 deprecated features or syntax
rg -l "(?i)(each|create_function|assert\s*\(.*string.*\)|split\(|\{\$|ereg|mysql_|mcrypt_)" || echo "No deprecated features found"
Length of output: 3880
Script:
#!/bin/bash
# Check for actual assert string usage in the codebase
rg "assert\s*\(['\"].*['\"]" -A 2
# Check for actual usage of each() function
rg "\beach\b\s*\(" -A 2
# Check for actual usage of create_function
rg "create_function\s*\(" -A 2
# Check for actual usage of split function
rg "\bsplit\s*\(" -A 2
# Check for actual usage of ereg
rg "\bereg\b" -A 2
# Check for actual usage of mysql_ functions
rg "mysql_\w+\s*\(" -A 2
# Check for actual usage of mcrypt
rg "mcrypt_\w+\s*\(" -A 2
Length of output: 199
Script:
#!/bin/bash
# Check all assert statements in the codebase
rg "assert\(" -A 2
# Check for any PHP 7.x or 8.x specific features that might be deprecated
rg "(?i)(declare\(strict_types|readonly|enum\s+class|match\s*\(|#\[.*\])" -A 2
# Look for type declarations that might be affected
rg "(function.*\?.*:|\|null|\?string|\?int|\?float|\?bool|\?array|\?object)" -A 2
Length of output: 47058
src/ClassesInDirectories.php
Outdated
| */ | ||
| public static function list(string ...$directories): iterable | ||
| public static function list(string ...$directories): Generator // @phpstan-ignore-line |
There was a problem hiding this comment.
Remove @phpstan-ignore-line annotation
The @phpstan-ignore-line annotation suggests unresolved static analysis issues. Consider fixing the underlying type issue instead of suppressing it.
The return type hint and PHPDoc are correct. Try updating PHPStan's configuration or baseline if needed.
src/ClassesInDirectories.php
Outdated
| if (is_array($token) && $token[0] === T_NAME_QUALIFIED) { | ||
| $namespace = $token[1]; | ||
| assert(is_string($namespace)); | ||
|
|
||
| return $namespace; |
There was a problem hiding this comment.
Improve type safety in namespace extraction
The token array access needs better type safety to ensure the namespace value exists and is a string.
Apply this diff to fix:
if (is_array($token) && $token[0] === T_NAME_QUALIFIED) {
+ if (! isset($token[1]) || ! is_string($token[1])) {
+ continue;
+ }
$namespace = $token[1];
- assert(is_string($namespace));
return $namespace;📝 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.
| if (is_array($token) && $token[0] === T_NAME_QUALIFIED) { | |
| $namespace = $token[1]; | |
| assert(is_string($namespace)); | |
| return $namespace; | |
| if (is_array($token) && $token[0] === T_NAME_QUALIFIED) { | |
| if (! isset($token[1]) || ! is_string($token[1])) { | |
| continue; | |
| } | |
| $namespace = $token[1]; | |
| return $namespace; |
This change ensures that the tokenizer extension is available for use in the project. The tokenizer extension is crucial for some of the functionalities and its absence could lead to runtime errors.
Ensure token elements are defined before accessing them to prevent potential PHP warnings. Added checks using `isset` to verify element existence and included `@phpstan-ignore-line` comments for static analysis tools.
Changed the list method to accept variadic string parameters for directories. Removed an assertion for class/interface existence that is deemed unnecessary.
Combined the checks for file type and extension into a single condition. Additionally, consolidated the class name validation to reduce redundancy and improve readability.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
composer.json(1 hunks)src/ClassesInDirectories.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- composer.json
🔇 Additional comments (2)
src/ClassesInDirectories.php (2)
8-25: LGTM! All necessary imports are present
The imports are well-organized and include all required functions, classes, and constants for the implementation.
65-67:
Add error handling for token_get_all
The token_get_all function can return false on failure, which should be handled explicitly.
$tokens = token_get_all($content);
+ if (! is_array($tokens)) {
+ return null;
+ }
/** @var array<int, mixed> $tokens */Likely invalid or redundant comment.
| if (isset($tokens[$j][0]) && $tokens[$j][0] === T_NAME_QUALIFIED) { // @phpstan-ignore-line | ||
| assert(isset($tokens[$j][1])); // @phpstan-ignore-line | ||
| $string = $tokens[$j][1]; |
There was a problem hiding this comment.
Remove @phpstan-ignore-line annotations
Instead of suppressing PHPStan warnings, we should fix the underlying type issues.
- if (isset($tokens[$j][0]) && $tokens[$j][0] === T_NAME_QUALIFIED) { // @phpstan-ignore-line
- assert(isset($tokens[$j][1])); // @phpstan-ignore-line
+ if (! isset($tokens[$j][0], $tokens[$j][1])) {
+ continue;
+ }
+ if ($tokens[$j][0] !== T_NAME_QUALIFIED) {
+ continue;
+ }📝 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.
| if (isset($tokens[$j][0]) && $tokens[$j][0] === T_NAME_QUALIFIED) { // @phpstan-ignore-line | |
| assert(isset($tokens[$j][1])); // @phpstan-ignore-line | |
| $string = $tokens[$j][1]; | |
| if (! isset($tokens[$j][0], $tokens[$j][1])) { | |
| continue; | |
| } | |
| if ($tokens[$j][0] !== T_NAME_QUALIFIED) { | |
| continue; | |
| } | |
| $string = $tokens[$j][1]; |
| { | ||
| /** @psalm-suppress MixedAssignment */ | ||
| foreach ($tokens as $index => $token) { | ||
| if (isset($token[0]) && $token[0] !== T_CLASS && $token[0] !== T_INTERFACE) { // @phpstan-ignore-line |
There was a problem hiding this comment.
Fix type checking in extractClassName
Similar to the namespace extraction, the type checking should be improved here as well.
- if (isset($token[0]) && $token[0] !== T_CLASS && $token[0] !== T_INTERFACE) { // @phpstan-ignore-line
+ if (! isset($token[0]) || ! in_array($token[0], [T_CLASS, T_INTERFACE], true)) {📝 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.
| if (isset($token[0]) && $token[0] !== T_CLASS && $token[0] !== T_INTERFACE) { // @phpstan-ignore-line | |
| if (! isset($token[0]) || ! in_array($token[0], [T_CLASS, T_INTERFACE], true)) { |
| public static function list(string ...$directories): Generator | ||
| { | ||
| foreach ($directories as $directory) { | ||
| $iterator = new RecursiveIteratorIterator( | ||
| new RecursiveDirectoryIterator($directory), | ||
| ); | ||
|
|
||
| /** @psalm-suppress MixedAssignment */ | ||
| foreach ($iterator as $file) { | ||
| if (! $file instanceof SplFileInfo || $file->getExtension() !== 'php') { | ||
| continue; | ||
| } | ||
|
|
||
| $className = self::getClassFromFile($file->getRealPath()); | ||
|
|
||
| if ($className === null || ! class_exists($className) && ! interface_exists($className)) { | ||
| continue; | ||
| } | ||
|
|
||
| yield $className; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Traits are not supported in the current implementation
The code inspection reveals that traits are not supported because:
- The
extractClassNamemethod only checks forT_CLASSandT_INTERFACEtokens - The existence check in the
listmethod only usesclass_exists()andinterface_exists() - The
T_TRAITtoken constant is not imported and trait detection is missing
This means traits like CamelCaseTrait in the codebase will be skipped by this implementation.
🔗 Analysis chain
LGTM! Verify trait support
The implementation is correct and efficient. However, since we're scanning for PHP classes, we should verify if trait declarations are also supported as they are common in PHP projects.
Let me check the implementation of getClassFromFile method to verify trait support.
Let me try a different approach to check the implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for trait declarations in the codebase to assess impact
rg -l "trait\s+\w+"
# Verify if any traits would be missed by current implementation
for file in $(rg -l "trait\s+\w+"); do
echo "=== $file ==="
rg "trait\s+(\w+)" --only-matching --replace '$1' "$file"
done
Length of output: 265
Script:
#!/bin/bash
# Check implementation of getClassFromFile method
ast-grep --pattern 'function getClassFromFile'
# Also check if trait_exists is used alongside class_exists and interface_exists
rg "trait_exists" -A 2 -B 2
Length of output: 76
Script:
#!/bin/bash
# Check the entire ClassesInDirectories.php file
cat src/ClassesInDirectories.php
# Also check if the trait in our codebase is detected by current implementation
php -r "var_dump(class_exists('CamelCaseTrait')); var_dump(interface_exists('CamelCaseTrait'));"
Length of output: 3723
Added PHP 8.4 as the current stable version while moving PHP 8.3 to the list of old stable versions. This change ensures the CI system tests against the latest PHP version.
Summary by CodeRabbit
New Features
listmethod to yield class names directly.Bug Fixes
Documentation