Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable PHP 8.4 comat #59

Merged
merged 10 commits into from
Nov 18, 2024
2 changes: 1 addition & 1 deletion composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
"php": "^8.1",
"ext-pdo": "*",
"ext-mbstring": "*",
"ext-tokenizer": "*",
"aura/sql": "^4.0 || ^5.0",
"doctrine/annotations": "^1.12 || ^2.0",
"guzzlehttp/guzzle": "^6.3 || ^7.2",
Expand All @@ -24,7 +25,6 @@
"ray/aop": "^2.10.4",
"ray/aura-sql-module": "^1.12.0",
"ray/di": "^2.14",
"roave/better-reflection": "^4.12 || ^5.6 || ^6.25",
"symfony/polyfill-php81": "^1.24"
},
"require-dev": {
Expand Down
137 changes: 104 additions & 33 deletions src/ClassesInDirectories.php
Original file line number Diff line number Diff line change
Expand Up @@ -5,51 +5,122 @@
namespace Ray\MediaQuery;

use Generator;
use Roave\BetterReflection\BetterReflection;
use Roave\BetterReflection\Reflector\DefaultReflector;
use Roave\BetterReflection\SourceLocator\Type\AggregateSourceLocator;
use Roave\BetterReflection\SourceLocator\Type\AutoloadSourceLocator;
use Roave\BetterReflection\SourceLocator\Type\DirectoriesSourceLocator;
use RecursiveDirectoryIterator;
use RecursiveIteratorIterator;
use SplFileInfo;

use function assert;
use function class_exists;
use function count;
use function file_get_contents;
use function interface_exists;
use function is_array;
use function is_string;
use function token_get_all;

use const T_CLASS;
use const T_INTERFACE;
use const T_NAME_QUALIFIED;
use const T_NAMESPACE;
use const T_STRING;
koriym marked this conversation as resolved.
Show resolved Hide resolved

final class ClassesInDirectories
{
/**
* get a list of all classes in the given directories.
*
* Based on: https://github.com/Roave/BetterReflection/blob/396a07c9d276cb9ffba581b24b2dadbb542d542e/demo/parsing-whole-directory/example2.php.
*
* @param list<string> $directories
+ * @param string ...$directories
*
* @return Generator<int, class-string>
*
* This function code is taken from https://github.com/WyriHaximus/php-list-classes-in-directory/blob/master/src/functions.php
* and modified for roave/better-reflection 5.x
*
* @see https://github.com/WyriHaximus/php-list-classes-in-directory
* @psalm-suppress MixedReturnTypeCoercion
* @phpstan-ignore-next-line/
*/
public static function list(string ...$directories): iterable
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;
}
}
}
Comment on lines +34 to +56
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Traits are not supported in the current implementation

The code inspection reveals that traits are not supported because:

  • The extractClassName method only checks for T_CLASS and T_INTERFACE tokens
  • The existence check in the list method only uses class_exists() and interface_exists()
  • The T_TRAIT token 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


private static function getClassFromFile(string $filePath): string|null
{
$content = file_get_contents($filePath);
koriym marked this conversation as resolved.
Show resolved Hide resolved
if ($content === false) {
return null;
}

$tokens = token_get_all($content);
/** @var array<int, mixed> $tokens */

Comment on lines +65 to +67
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

$namespace = self::extractNamespace($tokens);
$class = self::extractClassName($tokens);

if ($class === null) {
return null;
}

if ($namespace === null) {
return $class;
}

return $namespace . '\\' . $class;
}
Comment on lines +58 to +80
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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;
}


/** @param array<int, mixed> $tokens*/
private static function extractNamespace(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;
/** @psalm-suppress MixedAssignment */
foreach ($tokens as $index => $token) {
if (is_array($token) && $token[0] !== T_NAMESPACE) {
continue;
}

for ($j = $index + 1, $count = count($tokens); $j < $count; $j++) {
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];
Comment on lines +92 to +94
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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];

assert(is_string($string));

return $string;
}
}
}

return null;
}

/** @param array<int, mixed> $tokens */
private static function extractClassName(array $tokens): string|null
{
/** @psalm-suppress MixedAssignment */
foreach ($tokens as $index => $token) {
if (isset($token[0]) && $token[0] !== T_CLASS && $token[0] !== T_INTERFACE) { // @phpstan-ignore-line
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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)) {

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;
}
}
Loading