Enable PHP 8.4 compat #28
Conversation
Replaced mutable properties with readonly ones for better immutability and clarity. Simplified callback functions to arrow functions for improved readability. Updated composer dependencies to require PHP 8.1 and align with newer library versions. # Conflicts: # composer.json
Upgraded the PHP version from 8.3 to 8.4 in `static-analysis.yml` and `coding-standards.yml` workflows. This ensures compatibility with the latest PHP features and improvements.
Removed redundant casting to string in server string processing. This simplifies the code and ensures proper type handling without affecting functionality.
Introduced `phpmetrics/phpmetrics` under `vendor-bin/metrics` to enhance PHP codebase analysis. Also updated `vendor-bin/tools` dependencies, including adjustments to versions for compatibility and improvements.
Added a meaningful description to clarify the purpose of the package. This improves documentation and makes the project more understandable for users.
Dropped support for PHP 7.3, 7.4, and 8.0 in the CI configuration. This ensures the workflow focuses on currently supported and actively maintained PHP versions, improving test relevance and coverage.
Set default value for 'value' property in multiple annotations for consistency and improve flexibility. Additionally, update PHP dependencies and requirements to align with compatibility goals.
WalkthroughThis pull request introduces several updates across the project, primarily focusing on PHP version upgrades, code modernization using PHP 8 features, and dependency management. The changes span multiple files in GitHub Actions workflows, 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 (
|
![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
🧹 Nitpick comments (10)
src/Annotation/RedisInstance.php (2)
12-13: Consider removing redundant annotations.The docblock annotations
@Qualifierand@NamedArgumentConstructorare redundant since the class already uses the corresponding PHP 8 attributes#[Attribute]and#[Qualifier]. Consider removing these annotations to reduce duplication and maintain a cleaner codebase./** * @Annotation - * @Qualifier - * @NamedArgumentConstructor */
19-21: Simplify empty constructor.The constructor with an empty body can be simplified using a shorter syntax.
- public function __construct(public string $value = '') - { - } + public function __construct(public string $value = '') {}src/Annotation/MemcacheConfig.php (1)
20-22: Maintain consistency with RedisConfigThe changes mirror RedisConfig.php. If implementing the readonly suggestion for RedisConfig, apply the same here for consistency:
- public function __construct(public string $value = '') + public function __construct(public readonly string $value = '')src/Annotation/Shared.php (1)
20-22: Consider architectural impact of annotation changesThe changes across all annotation classes show two patterns that need attention:
- Properties changing from private to public might affect encapsulation
- Default value changes could impact existing cache key generation
Recommendations:
- Use readonly properties to maintain some level of encapsulation
- Document the rationale for default value changes
- Consider adding migration notes if keeping the changes
src/Annotation/CacheDir.php (1)
Line range hint
11-18: Consider removing legacy docblock annotations.Since PHP 8 attributes are being used, the legacy docblock annotations (
@Annotation,@Qualifier,@NamedArgumentConstructor) could be removed to reduce duplication and improve maintainability.src/PhpFileAdapter.php (1)
Line range hint
13-14: Consider updating Serializable implementation.The
Serializableinterface is deprecated since PHP 8.1. Consider implementing__serialize()and__unserialize()methods instead.-class PhpFileAdapter extends OriginAdapter implements Serializable +class PhpFileAdapter extends OriginAdapter { - use SerializableTrait; + private array $args; + + public function __serialize(): array + { + return $this->args; + } + + public function __unserialize(array $data): void + { + $this->args = $data; + parent::__construct(...$this->args); + }src/RedisAdapter.php (1)
Line range hint
1-1: Consider a consistent update across all adapter classes.The changes nicely modernize the codebase with PHP 8 features. To complete the modernization:
- Consider updating all adapter classes (
PhpFileAdapter,FilesystemAdapter,RedisAdapter) to use__serialize()and__unserialize()instead of the deprecatedSerializableinterface.- This could be done in a separate PR to keep the current changes focused on PHP 8.4 compatibility.
src/RedisProvider.php (1)
23-23: Consider using more specific type hintsWhile the property promotion syntax is good, consider using a more specific type hint for
$server. The docblock indicates it's alist<string>, but the type hint only showsarray.- public function __construct(private array $server) + public function __construct(private array|list<string> $server)src/LocalCacheProvider.php (1)
28-28: Consider consistent property declaration styleWhile the readonly property promotion for
$namespaceis good, consider using the same style for$cacheDirto maintain consistency:- public function __construct(string $cacheDir = '', private readonly string $namespace = '') + public function __construct( + private readonly string $cacheDir = '', + private readonly string $namespace = '' + )src/Psr6MemcachedModule.php (1)
26-26: Consider extracting server parsing logicWhile the arrow function improves readability, consider extracting the server parsing logic into a dedicated method for better maintainability:
- $this->servers = array_map(static fn ($serverString) => explode(':', $serverString), explode(',', $servers)); + $this->servers = $this->parseServers($servers); + private function parseServers(string $servers): array + { + return array_map( + static fn ($serverString) => explode(':', $serverString), + explode(',', $servers) + ); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
vendor-bin/metrics/composer.lockis excluded by!**/*.lockvendor-bin/tools/composer.lockis excluded by!**/*.lock
📒 Files selected for processing (24)
.github/workflows/coding-standards.yml(1 hunks).github/workflows/continuous-integration.yml(0 hunks).github/workflows/static-analysis.yml(1 hunks)composer.json(2 hunks)src/Annotation/CacheDir.php(1 hunks)src/Annotation/CacheNamespace.php(1 hunks)src/Annotation/Local.php(1 hunks)src/Annotation/MemcacheConfig.php(1 hunks)src/Annotation/RedisConfig.php(1 hunks)src/Annotation/RedisInstance.php(1 hunks)src/Annotation/Shared.php(1 hunks)src/CacheDirModule.php(1 hunks)src/CacheNamespaceModule.php(1 hunks)src/FilesystemAdapter.php(1 hunks)src/LocalCacheProvider.php(1 hunks)src/MemcachedProvider.php(1 hunks)src/PhpFileAdapter.php(1 hunks)src/Psr6MemcachedModule.php(1 hunks)src/RedisAdapter.php(1 hunks)src/RedisProvider.php(1 hunks)tests/MemcachedAdapterTest.php(2 hunks)tests/RedisAdapterTest.php(2 hunks)vendor-bin/metrics/composer.json(1 hunks)vendor-bin/tools/composer.json(1 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/continuous-integration.yml
✅ Files skipped from review due to trivial changes (3)
- .github/workflows/coding-standards.yml
- tests/MemcachedAdapterTest.php
- tests/RedisAdapterTest.php
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: PHPUnit (ubuntu-latest, 8.4, lowest)
- GitHub Check: sa / PHPMD
🔇 Additional comments (16)
src/Annotation/Local.php (1)
20-22:⚠️ Potential issueVerify impact of changing default value from 'cache' to empty string
The default value change from 'cache' to '' is a potentially breaking change that could affect existing cache key generation or configuration logic.
Let's verify the usage and potential impact:
✅ Verification successful
Default value change appears safe
All discovered usages of the Local annotation in the codebase explicitly specify values, with no instances relying on the default value. The change from 'cache' to empty string doesn't impact existing functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for Local annotation usage without explicit values rg -l "Local" | grep -v "Local.php" | xargs rg -B 2 -A 2 "#\[Local\]|@Local" # Search for cache key generation logic rg -l "getCacheKey|cacheKey" | xargs rg -A 3 "getCacheKey|cacheKey"Length of output: 796
src/Annotation/CacheDir.php (1)
20-22: LGTM! Constructor property promotion improves code clarity.The update to use constructor property promotion is a good modernization of the code.
src/Annotation/CacheNamespace.php (2)
20-22: LGTM! Constructor property promotion improves code clarity.The update to use constructor property promotion is a good modernization of the code.
Line range hint
11-18: Consider removing legacy docblock annotations.Since PHP 8 attributes are being used, the legacy docblock annotations could be removed to reduce duplication and improve maintainability.
src/CacheDirModule.php (1)
12-15: LGTM! Excellent use of readonly property promotion.The implementation correctly uses PHP 8.1's readonly property promotion while maintaining the parent constructor call. This provides better immutability guarantees and cleaner code.
src/CacheNamespaceModule.php (1)
12-15: LGTM! Excellent use of readonly property promotion.The implementation correctly uses PHP 8.1's readonly property promotion while maintaining the parent constructor call.
src/PhpFileAdapter.php (1)
18-24: LGTM! Clean upgrade to PHP 8 attributes.The transition from PHPDoc annotation to PHP 8 attribute for
CacheNamespaceis a good modernization.src/MemcachedProvider.php (1)
21-26: LGTM! Excellent use of PHP 8 features.Good use of:
- Constructor property promotion
- Readonly properties
- Preserved documentation while simplifying the code
src/FilesystemAdapter.php (2)
20-27: LGTM! Clean upgrade to PHP 8 attributes.Good transition from PHPDoc annotations to PHP 8 attributes for both
CacheNamespaceandCacheDir.
Line range hint
15-16: Consider updating Serializable implementation.Similar to PhpFileAdapter, consider updating from the deprecated
Serializableinterface to__serialize()and__unserialize()methods.src/RedisAdapter.php (2)
22-30: LGTM! Clean upgrade to PHP 8 attributes.Good transition to PHP 8 attributes for both
NamedandCacheNamespacewhile maintaining proper type documentation.
Line range hint
16-17: Consider updating Serializable implementation.Similar to other adapter classes, consider updating from the deprecated
Serializableinterface to__serialize()and__unserialize()methods..github/workflows/static-analysis.yml (1)
12-12: Verify PHP 8.4 compatibility strategySince PHP 8.4 is currently in development/alpha stage, consider the following recommendations:
- Add PHP 8.4 as an additional test target rather than replacing 8.3
- Mark PHP 8.4 jobs as experimental in the matrix to allow failures
This ensures we catch compatibility issues early while maintaining stable CI pipelines.
vendor-bin/tools/composer.json (1)
9-9: Consider stability implications of using Psalm dev-masterWhile using
dev-mastermight be necessary for PHP 8.4 support, it could lead to unstable static analysis results. Consider:
- Pinning to a specific commit hash instead of dev-master
- Adding a comment explaining why dev-master is needed
Let's check if the current stable version of Psalm supports PHP 8.4:
composer.json (2)
3-3: LGTM! Clear and descriptive package descriptionThe updated description accurately reflects the package's purpose and integration with Ray.Di.
15-15: Verify Symfony Cache compatibility matrixThe removal of support for Symfony 5.x and update to ^7.2 needs verification:
- Ensure compatibility with PHP 8.4
- Check if this is a breaking change for existing users
Let's check Symfony's compatibility with PHP 8.4:
✅ Verification successful
Symfony Cache compatibility verified ✓
The update to
symfony/cache: "^6.0 || ^7.2"is safe because:
- PHP 8.4 is fully compatible (Symfony 7.2 requires PHP >= 8.2)
- Not a breaking change as it maintains compatibility with Symfony 6.0 for PHP 8.1 users
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check Symfony Cache's composer.json for PHP version support curl -s https://raw.githubusercontent.com/symfony/cache/7.2/composer.json | jq '.require.php'Length of output: 101
Script:
#!/bin/bash # Check current PHP version requirement and other Symfony dependencies cat composer.json | jq '.require | {php: .php, symfony: [to_entries[] | select(.key | startswith("symfony/"))]}' # Check if there are any references to Symfony 5.x in the codebase rg "symfony/[^\"]+\":\s*\"[^\"]*5\." -g "composer.json"Length of output: 278
| @@ -0,0 +1,5 @@ | |||
| { | |||
| "require-dev": { | |||
| "phpmetrics/phpmetrics": "^2.8" | |||
There was a problem hiding this comment.
💡 Codebase verification
The current version (v2.8.2) only requires PHP >= 5.5, which is very outdated. Consider:
- Upgrading to v3.0.0rc6 which might have better modern PHP support
- Or waiting for the stable v3.0.0 release
🔗 Analysis chain
Verify phpmetrics compatibility with PHP 8.4
Since the project is targeting PHP 8.4 compatibility, ensure that phpmetrics v2.8 is compatible with PHP 8.4.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check phpmetrics latest version and PHP compatibility
curl -s https://packagist.org/packages/phpmetrics/phpmetrics.json | jq -r '.package.versions["2.8.2"].require.php'
Length of output: 121
Script:
#!/bin/bash
# Get full package info to examine structure
curl -s https://packagist.org/packages/phpmetrics/phpmetrics.json | jq '.package.versions["2.8.2"]'
Length of output: 106
Script:
#!/bin/bash
# Get all package versions
curl -s https://packagist.org/packages/phpmetrics/phpmetrics.json | jq '.package.versions | keys'
Length of output: 1082
Script:
#!/bin/bash
# Get composer.json content from phpmetrics repository
curl -s https://raw.githubusercontent.com/phpmetrics/PhpMetrics/v2.8.2/composer.json | jq '.'
Length of output: 1298
| @@ -9,10 +9,10 @@ | |||
| } | |||
| ], | |||
| "require": { | |||
| "php": "^7.3 || ^8.0", | |||
| "php": "^8.1", | |||
There was a problem hiding this comment.
💡 Codebase verification
Breaking Change: Documentation Updates Required for PHP 8.1 Requirement
The PHP version requirement change to ^8.1 needs proper documentation:
- Create a CHANGELOG.md file to document this breaking change
- Ensure the package version is bumped to reflect this major change
- Consider adding an upgrade guide to help users migrate
🔗 Analysis chain
Breaking Change: Verify impact of increased PHP version requirement
Increasing minimum PHP version to ^8.1 is a breaking change that should be:
- Documented in CHANGELOG.md
- Reflected in the next major version number
- Mentioned in upgrade guide if available
Let's check for potential documentation updates:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if breaking change is documented
fd -e md . -E vendor -E .github | xargs grep -l "8.1"
Length of output: 53
Script:
#!/bin/bash
# Check for changelog and version documentation
echo "=== Checking for CHANGELOG.md ==="
fd "CHANGELOG" --type f
echo -e "\n=== Current version in composer.json ==="
cat composer.json | grep '"version":'
echo -e "\n=== Searching for upgrade guides ==="
fd "upgrade|migration|breaking|UPGRADE" --type f
echo -e "\n=== Searching for PHP version mentions with different formats ==="
rg -i "php.*8\.1|8\.1\.[0-9]|minimum.*php" -g "!{vendor,node_modules}/*"
Length of output: 1303
Description:
composer.jsonfor clarity.phpmetrics/phpmetricsundervendor-bin/metricsfor improved code analysis.Summary by CodeRabbit
PHP Version Upgrade
composer.jsonto ^8.1Dependency Updates
Code Modernization
Continuous Integration