Update CI to support PHP 8.4 and include PHP 8.3 in old_stable #293
Conversation
The continuous integration workflow now supports PHP 8.4 as the current stable version. PHP 8.3 has been moved to the old_stable versions list to ensure compatibility continues with earlier versions.
WalkthroughThe continuous integration workflow configuration in the Changes
Possibly related PRs
Suggested reviewers
Poem
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 3 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. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
.github/workflows/continuous-integration.yml(1 hunks)
🔇 Additional comments (1)
.github/workflows/continuous-integration.yml (1)
12-12: LGTM! Moving PHP 8.3 to old_stable is appropriate.
The addition of PHP 8.3 to the old_stable versions is correct as it's now a stable release.
Upgraded PHPStan and PHPStan-PHPUnit to version 2.0 to ensure compatibility with the latest features and improvements. Additionally, Symfony components have been updated to version 7.2.0 to benefit from bug fixes and performance enhancements. This update maintains alignment with the supported PHP versions and related dependencies.
This commit eliminates redundant assert statements to streamline the code and improve readability. Additionally, it uses `@phpstan-ignore-line` annotations to suppress specific warnings, enhancing compatibility with static analysis tools.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/di/InjectionPoint.php (1)
Line range hint
116-123: Consider strengthening type safetyWhile the
@psalm-param string $dataannotation is helpful, consider using PHP 8's native type hints for better type safety:- public function unserialize($data): void + public function unserialize(string $data): voidThis change would make the type requirement explicit at the language level rather than relying solely on static analysis tools.
📜 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 (9)
src/di/Argument.php(1 hunks)src/di/Di/Inject.php(1 hunks)src/di/InjectionPoint.php(1 hunks)src/di/MultiBinding/LazyProvider.php(0 hunks)src/di/Name.php(0 hunks)tests/di/ArgumentsTest.php(0 hunks)tests/di/Fake/FakePhp8Car.php(1 hunks)tests/di/GrapherTest.php(0 hunks)vendor-bin/tools/composer.json(1 hunks)
💤 Files with no reviewable changes (4)
- src/di/MultiBinding/LazyProvider.php
- src/di/Name.php
- tests/di/ArgumentsTest.php
- tests/di/GrapherTest.php
✅ Files skipped from review due to trivial changes (1)
- src/di/Di/Inject.php
🔇 Additional comments (6)
vendor-bin/tools/composer.json (2)
6-6: LGTM! Synchronized version updates
The synchronized update of both PHPStan core and its PHPUnit extension maintains compatibility between these interdependent packages.
Also applies to: 9-9
6-6: Verify compatibility with PHPStan 2.0
The major version upgrade of PHPStan (1.x → 2.x) may introduce breaking changes. Let's verify the codebase compatibility.
✅ Verification successful
Let me analyze the findings from the shell script results to verify PHPStan 2.0 compatibility:
- Found phpstan.neon configuration file
- Reviewed PHPStan annotations usage:
- Most annotations are standard docblock annotations (@var, @param, @return) that are compatible across versions
- Found @phpstan-ignore-line and @phpstan-ignore-next-line usages which are still supported in PHPStan 2.0
- Found @phpstan-param and @phpstan-return annotations which are still valid in 2.0
- No deprecated or removed PHPStan annotations found
Let me check the phpstan.neon configuration file content to ensure compatibility:
PHPStan 2.0 upgrade is compatible with the codebase
The codebase is compatible with PHPStan 2.0 based on:
- All PHPStan annotations used (@phpstan-ignore-line, @phpstan-param, @phpstan-return) are supported in 2.0
- The phpstan.neon configuration uses standard parameters (level, paths, excludePaths, ignoreErrors) that are unchanged in 2.0
- The ignore patterns for generic ReflectionAttribute types are still valid in 2.0
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for PHPStan-specific annotations and potential incompatibilities
# Search for PHPStan annotations that might need updates
rg -g '*.php' '@phpstan|/\*\* @var|@param|@return' -A 2
# Look for phpstan.neon configuration
fd 'phpstan.neon'
Length of output: 62378
Script:
#!/bin/bash
# Check phpstan.neon configuration
cat phpstan.neon
Length of output: 396
tests/di/Fake/FakePhp8Car.php (1)
66-66: LGTM! Type declaration change improves type safety.
The change to use ?FakeMirrorInterface makes the nullable parameter explicit and aligns with PHP 8.x type system best practices.
Let's verify if there are other similar patterns that could benefit from explicit nullable types:
✅ Verification successful
Nullable parameter type declaration is consistent with codebase patterns
The change to use ?FakeMirrorInterface in notQualifer method aligns with existing patterns in the codebase:
- Similar nullable parameter declarations are used consistently across test files (e.g., in
FakeAssistedParamsConsumer,FakeAssistedInjectDb) - The
FakeNotQualiferattribute is used only in this specific method, making it a unique test case - The change maintains compatibility with PHP 8.x while improving type safety
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other method parameters with null defaults that might benefit from explicit nullable types
ast-grep --pattern 'function $FNAME($PARAM = null)'
# Search for FakeNotQualifer attribute usage to ensure consistent parameter typing
ast-grep --pattern '#[FakeNotQualifer] $TYPE $PARAM'
Length of output: 108
Script:
#!/bin/bash
# Let's try a different approach to find nullable parameters and their type declarations
# First, let's look at the specific file content
cat tests/di/Fake/FakePhp8Car.php
# Then search for similar patterns in other files
rg "function.*=\s*null\)" --type php
# Also search for FakeNotQualifer attribute usage
rg "FakeNotQualifer" --type php
Length of output: 4974
src/di/InjectionPoint.php (2)
Line range hint 110-123: Verify serialization implementation consistency
The current implementation maintains both old and new serialization methods. While this provides backward compatibility, ensure that both methods produce consistent results.
Let's verify the serialization implementation across the codebase:
✅ Verification successful
Let me try a different approach to verify the serialization implementation:
Let me try one more approach focusing specifically on the InjectionPoint class:
Serialization implementation is consistent and well-designed
The implementation in InjectionPoint class shows a proper and consistent serialization pattern where:
- The legacy
serialize()method correctly delegates to__serialize() - The legacy
unserialize()method correctly delegates to__unserialize() - Both methods maintain the same data structure and serialization format
- The implementation follows PHP's serialization protocol migration guidelines
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find all classes implementing custom serialization
ast-grep --pattern 'function __serialize()' > modern_serial.txt
ast-grep --pattern 'function serialize()' > legacy_serial.txt
echo "Classes with potential serialization inconsistencies:"
comm -12 modern_serial.txt legacy_serial.txt
rm modern_serial.txt legacy_serial.txt
Length of output: 389
Script:
#!/bin/bash
# Check the implementation of both serialization methods
echo "=== Modern Serialization (__serialize) ==="
ast-grep --pattern 'function __serialize() {
$$$
}'
echo -e "\n=== Legacy Serialization (serialize) ==="
ast-grep --pattern 'function serialize() {
$$$
}'
echo -e "\n=== Unserialization Methods ==="
ast-grep --pattern 'function __unserialize($$$) {
$$$
}'
ast-grep --pattern 'function unserialize($$$) {
$$$
}'
Length of output: 500
Script:
#!/bin/bash
# Search for the complete implementation of serialization methods in InjectionPoint
echo "=== InjectionPoint Serialization Implementation ==="
rg -A 10 "__serialize|serialize|__unserialize|unserialize" src/di/InjectionPoint.php
Length of output: 1169
110-113: Consider removing the Serializable interface for PHP 8.4 compatibility
The Serializable interface is deprecated as of PHP 8.4. Since you're already implementing the modern __serialize() and __unserialize() methods, consider removing the legacy serialization methods.
Let's verify the PHP version compatibility requirements:
src/di/Argument.php (1)
94-94: 🛠️ Refactor suggestion
Consider modernizing serialization implementation
While adding @phpstan-ignore-line resolves the static analysis warning, consider a more forward-looking approach since:
- The
Serializableinterface is soft-deprecated - The class already implements the newer
__serialize()and__unserialize()methods
Consider removing the Serializable interface implementation entirely and rely on the magic methods:
-class Argument implements Serializable, AcceptInterface
+class Argument implements AcceptInterface
- public function serialize(): ?string // @phpstan-ignore-line
- {
- return serialize($this->__serialize());
- }
-
- public function unserialize($data): void
- {
- /** @var array{0: string, 1: bool, 2: string, 3: string, 4: string, 5: array{0: string, 1: string, 2:string}} $array */
- $array = unserialize($data, ['allowed_classes' => false]);
- $this->__unserialize($array);
- }Let's verify PHP 8.4 compatibility and usage of Serializable interface:
Removed unnecessary assertions such as `assert` and `property_exists` to make the tests cleaner and more efficient. Adjusted array handling by casting `bindings` to an array for better compatibility and added necessary code comments to suppress static analysis warnings where applicable. These changes improve the maintainability and readability of the codebase without affecting the functionality.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
tests/di/DependencyTest.php (1)
118-118: LGTM! Consider adding type documentation.The array type casting is a good defensive programming practice that makes the test more robust by handling potential variations in the
bindingsproperty type.Consider adding a comment to document the expected type of
$instance->bindings:assert(property_exists($instance, 'bindings')); + /** @var array|ArrayAccess $instance->bindings */ $this->assertArrayHasKey('returnSame', (array) $instance->bindings);tests/di/InjectorTest.php (1)
430-430: Consider improving the test's property access patternWhile the phpstan-ignore comment resolves the static analysis warning, consider these alternatives for more maintainable testing:
- Add a getter method in the FakeAop class to expose the bindings in a type-safe way
- Create a test-specific method to verify the interceptor binding
- Document why the phpstan-ignore is necessary in a comment
Example implementation:
- $this->assertInstanceOf(NullInterceptor::class, $instance->bindings['returnSame'][0]); // @phpstan-ignore-line + // Option 1: Add a getter + $this->assertInstanceOf(NullInterceptor::class, $instance->getMethodInterceptors('returnSame')[0]); + + // Option 2: Add a test method + $this->assertTrue($instance->hasNullInterceptorForMethod('returnSame'));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
src/di/Bind.php(1 hunks)src/di/MultiBinding/LazyProvider.php(0 hunks)src/di/Name.php(1 hunks)tests/di/ArgumentsTest.php(0 hunks)tests/di/DependencyTest.php(1 hunks)tests/di/GrapherTest.php(0 hunks)tests/di/InjectorTest.php(1 hunks)
💤 Files with no reviewable changes (3)
- src/di/MultiBinding/LazyProvider.php
- tests/di/ArgumentsTest.php
- tests/di/GrapherTest.php
🚧 Files skipped from review as they are similar to previous changes (1)
- src/di/Name.php
🔇 Additional comments (1)
src/di/Bind.php (1)
221-221: LGTM! Consider adding a comment explaining the PHPStan ignore.
The PHPStan ignore is valid here since the runtime check is necessary despite the type hint, as array keys in PHP can be either strings or integers. However, consider adding a brief comment explaining why the ignore is needed for better maintainability.
- if (! is_string($key)) { // @phpstan-ignore-line
+ // PHPStan doesn't recognize that array keys can be integers despite array<string,string> type hint
+ if (! is_string($key)) { // @phpstan-ignore-lineLet's verify if similar PHPStan ignores were added consistently across the codebase:
✅ Verification successful
PHPStan ignore is correctly applied and consistent with codebase patterns
The codebase shows a consistent pattern of handling string type validation:
- Similar
is_string()checks exist in multiple files (Name.php, Container.php, AssistedInterceptor.php) - PHPStan ignores are used judiciously for cases where runtime checks are necessary despite type hints
- The specific case in Bind.php follows the same pattern as Name.php which also has a PHPStan ignore for string key assertion
The added PHPStan ignore is appropriate and follows established patterns in the codebase. While a comment explaining the ignore would be nice-to-have, it's not strictly necessary as this is a common pattern in the codebase for handling array key type validation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for similar array key type checks and their PHPStan annotations
# Expected: Find similar patterns in other files that might need the same treatment
# Search for array key type checks
rg "is_string\(\$key\)" --type php
# Search for related PHPStan ignores
rg "@phpstan-ignore.*is_string" --type php
Length of output: 80
Script:
#!/bin/bash
# Let's try a broader search to find similar patterns and PHPStan ignores
# Search for is_string checks in PHP files with context
rg "is_string\(" --type php -B 1 -A 1
# Search for PHPStan ignore comments
rg "@phpstan-ignore-line" --type php -B 1 -A 1
# Search for array key type validations with different variable names
rg "if.*!.*is_string\(\$\w+\)" --type php
Length of output: 5638
The PHPUnit version constraint in composer.json has been updated to "^8.5.40 || ^9.5" from "^8.5.24 || ^9.5" to ensure compatibility with recent bug fixes and improvements. This change will help maintain the stability and reliability of the development and testing environment.
This change updates the "ray/aop" package to version 2.16 in the composer.json file. The update aims to incorporate enhancements, bug fixes, or security improvements that come with the new version, ensuring better performance or compliance with the latest standards.
|
Summary by CodeRabbit
ray/aopdependency version incomposer.json.ArgumentsTest,GrapherTest, andInjectorTest.DependencyTestto ensure correct type checking of bindings.FakePhp8Carto allow for optional values.