Drop php 8.0 support #12
Conversation
Reorder assertion parameters in several test files for consistency and replace count assertions with assertCount for clarity. Update PDO usage to \PDO namespace consistently across the project. Ensure constructors and lambda functions have consistent formatting. Minor corrections in documentation and exception handling to improve accuracy and readability.
Remove PHP 8.0 from the GitHub Actions test matrix and update the minimum required PHP version in composer.json to 8.1. Adjust README to reflect the updated PHP version requirements. Additionally, streamline dev dependencies by updating PHPUnit requirements.
Refactor test assertions in PdoClassTest to use assertTrue and assertFalse for clarity and maintain consistency across tests. Remove unused variables in ResultTest and streamline imports in PdoStatementTest by removing unnecessary backslashes.
Updated constructors and methods to use nullable type hints for parameters to ensure consistency and improve code readability. Also, added the `failOnDeprecation` attribute in PHPUnit configuration to enhance error detection.
WalkthroughThe pull request includes updates to several files, primarily focused on correcting documentation, modifying PHP version requirements, and enhancing test assertions. Key changes involve the removal of PHP version Changes
Possibly related PRs
Warning Rate limit exceeded@ActuallyConnor has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 14 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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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
🧹 Outside diff range and nitpick comments (7)
tests/Unit/QueryLogTest.php (1)
PHP 8.1 compatibility improvements needed in QueryLogTest
The codebase has already dropped PHP 8.0 support (composer.json requires
>=8.1), but the test file isn't leveraging PHP 8.1 features. Consider these improvements:
- Add
readonlyproperty for the test class's$queryLogproperty- Use intersection types for the iterator assertions (e.g.,
Iterator&Traversable)- Add return type declarations for
testAddQuery()- Use typed properties with the
mixedtype for the test dataThe verification shows that PHP 8.1 features like
readonlyare already being used in other parts of the codebase (e.g.,PdoQueries.php), so these suggestions align with the existing codebase style.🔗 Analysis chain
Line range hint
1-50: Verify PHP 8.0 compatibility impactWhile the assertion improvement is good, this file doesn't contain any changes related to dropping PHP 8.0 support. Consider:
- Adding type hints that were introduced in PHP 8.1 to enforce the version requirement
- Using PHP 8.1+ features where applicable to prevent accidental usage with PHP 8.0
Let's verify the PHP version requirements:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check PHP version requirements in composer.json and other config files # Expected results: Confirm PHP 8.0 is properly excluded from supported versions # Check composer.json PHP version constraint fd -t f composer.json -x cat {} \; | jq -r '.require.php' # Check CI configuration for PHP versions fd -t f -e yml -e yaml . -x cat {} \; | grep -A 5 'php-version'Length of output: 133
Script:
#!/bin/bash # Let's verify PHP 8.1 specific features usage in the test file and related classes # Check for PHP 8.1 specific syntax in QueryLogTest.php ast-grep --pattern 'class QueryLogTest { $$$ }' # Check the QueryLog class implementation fd -t f -e php . -x cat {} \; | grep -A 20 "class QueryLog" # Check for intersection types, readonly properties, or other PHP 8.1 features rg "readonly|&\s*[A-Z]|\?\s*[A-Z]" -t phpLength of output: 1510
tests/Unit/ResultTest.php (2)
37-37: LGTM! Consider similar improvements throughout the test file.The change from
assertEquals(false, ...)toassertFalse(...)improves test clarity and follows PHPUnit best practices.Consider applying similar assertion improvements throughout the test file. For example:
- Line 71:
$this->assertEquals(null, $r->nextRow())could be$this->assertNull($r->nextRow())Would you like me to suggest more specific assertion improvements?
Line range hint
1-156: Consider leveraging PHP 8.1+ features to enhance test robustness.Since PHP 8.0 support is being dropped, consider enhancing the test file with PHP 8.1+ features:
- Use intersection types for mock objects
- Add readonly properties for test fixtures
- Use the new
neverreturn type for methods that always throw exceptionsExample improvements:
class ResultTest extends TestCase { + private readonly Result $result; + + protected function setUp(): void + { + $this->result = new Result(); + } - public function testSetErrorCode() + public function testSetErrorCode(): never // Method always throws { // ... } }tests/Unit/SampleModels/PdoQueries.php (2)
Line range hint
116-122: Security vulnerability: SQL injection risk in IN clause.The current implementation is vulnerable to SQL injection as it directly interpolates values into the query string. The
implode()approach for IN clauses bypasses prepared statement protection.Consider this safer alternative:
- $select = $this->pdo->prepare( - 'SELECT * FROM users WHERE id IN (:userIds) ORDER BY created_at DESC', - ); - - $userIds = implode(',', $ids); - - $select->execute([':userIds' => $userIds]); + $placeholders = str_repeat('?,', count($ids) - 1) . '?'; + $select = $this->pdo->prepare( + "SELECT * FROM users WHERE id IN ($placeholders) ORDER BY created_at DESC" + ); + $select->execute($ids);
Line range hint
17-23: Consider using more specific return types.Methods like
selectQueryWithNoParametersuse union typesbool|array. Consider using more specific return types like?arrayfor better type safety, which is well-supported in PHP 8.1+.- public function selectQueryWithNoParameters(): bool|array + public function selectQueryWithNoParameters(): ?array { // select all users $stmt = $this->pdo->query("SELECT * FROM users"); return $stmt->fetch(PDO::FETCH_ASSOC); }tests/Unit/PdoClassTest.php (1)
33-46: LGTM! Consider extending this pattern to other test methods.The changes from
assertEqualstoassertTrue/assertFalseimprove test readability and follow PHPUnit best practices by using more semantically appropriate assertions for boolean conditions.Consider updating similar assertions in other test methods for consistency. For example, the assertions in
testErrorInfoandtestErrorCodemethods could benefit from the same improvement.src/PdoStatement.php (1)
Line range hint
1-324: Consider additional PHP 8.1+ type system improvements.While the current changes improve type safety, there are additional PHP 8.1+ features that could be leveraged:
- Consider using readonly properties where applicable
- Intersection types could be used for more precise type constraints
- Pure intersection types are available for parameter and return types
Would you like me to suggest specific locations where these PHP 8.1+ features could be applied?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (16)
.github/dependabot.yaml(1 hunks).github/workflows/test.yml(2 hunks)README.md(2 hunks)composer.json(1 hunks)docs/index.md(1 hunks)phpunit.xml.dist(1 hunks)src/Exceptions/PseudoException.php(1 hunks)src/Exceptions/PseudoPdoException.php(1 hunks)src/Pdo.php(1 hunks)src/PdoStatement.php(3 hunks)tests/Unit/PdoClassTest.php(3 hunks)tests/Unit/PdoStatementTest.php(11 hunks)tests/Unit/QueryLogTest.php(1 hunks)tests/Unit/ResultCollectionTest.php(1 hunks)tests/Unit/ResultTest.php(1 hunks)tests/Unit/SampleModels/PdoQueries.php(8 hunks)
✅ Files skipped from review due to trivial changes (4)
- .github/dependabot.yaml
- docs/index.md
- src/Exceptions/PseudoException.php
- tests/Unit/ResultCollectionTest.php
🔇 Additional comments (22)
src/Exceptions/PseudoPdoException.php (1)
4-7: LGTM! Clean improvement to code readability.
The change from using a fully qualified namespace to an import statement:
- Follows PSR-12 coding standards
- Maintains consistency with other exception classes in the codebase
- Preserves identical functionality
phpunit.xml.dist (2)
2-2: Good addition of failOnDeprecation="true"
Adding this flag helps maintain code quality by catching deprecated feature usage early, which is especially important when dropping support for older PHP versions.
2-2:
Verify PHPUnit version compatibility
The PHPUnit schema version 11.3 requires PHP 8.2+, but according to the summary, this PR aims to set the minimum PHP version to 8.1. This might cause compatibility issues.
Let's verify the PHP version requirements:
Consider either:
- Using PHPUnit 10.x schema if PHP 8.1 support is needed, or
- Updating the minimum PHP requirement to 8.2+ to match PHPUnit 11.x requirements
✅ Verification successful
PHPUnit configuration is correctly set up
The configuration is valid as the project supports both PHPUnit 10.5 and 11.3 versions ("phpunit/phpunit": "^10.5|^11.3"). While PHPUnit 11.3 requires PHP 8.2+, the project also supports PHPUnit 10.5 which is compatible with PHP 8.1. The schema version in phpunit.xml.dist will be automatically adjusted based on the installed PHPUnit version during dependency resolution.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check composer.json for PHP version requirement
echo "Checking composer.json for PHP version requirement:"
rg '"php":\s*"[^"]+"' composer.json
# Check PHPUnit version requirement
echo -e "\nChecking PHPUnit version requirement:"
rg '"phpunit/phpunit":\s*"[^"]+"' composer.json
Length of output: 357
composer.json (2)
25-26: LGTM! Development dependencies updated appropriately.
Removing support for PHPUnit 9.6 and PHP Code Coverage 9.2 is consistent with dropping PHP 8.0 support, as these were the last versions supporting PHP 8.0.
20-20: LGTM! PHP version requirement update aligns with PR objective.
The change to require PHP >=8.1 is clear and consistent with the PR's goal to drop PHP 8.0 support.
Let's verify this change is consistent across the codebase:
.github/workflows/test.yml (2)
37-37: LGTM! Consistent version matrix
The static analysis job maintains consistency with the unit tests job, ensuring uniform PHP version support across all CI checks.
16-16: Verify PHP 8.4 compatibility
The removal of PHP 8.0 aligns with the PR objective. However, since PHP 8.4 is currently in development, we should verify that our codebase is compatible with its alpha/beta releases.
✅ Verification successful
PHP 8.4 compatibility is safe to include
The codebase appears compatible with PHP 8.4 based on:
- No usage of experimental PHP 8.4 features (readonly, match expressions)
- Simple class-based OOP code that's unlikely to be affected by PHP 8.4 changes
- Composer.json requires PHP >=8.1 with no upper bound constraint
- PHPUnit dependencies support future versions (^11.3)
- Static analysis tool (PHPStan) is at version 2.0, which typically maintains compatibility with newer PHP versions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check composer.json constraints and PHP 8.4 compatibility flags
# Check if composer.json has any PHP 8.4 specific constraints or flags
rg -A 5 "php.*8\.4|platform|require" composer.json
# Look for any PHP 8.4 specific code or compatibility checks
rg "php_version|PHP_VERSION|version_compare.*8\.4"
Length of output: 409
Script:
#!/bin/bash
# Check for any PHP version-specific features or syntax in the codebase
# Look for match expressions, readonly properties, and other PHP 8.4 features
# Search for potential PHP 8.4 features
rg "readonly\s+(class|property|function)" -A 2
rg "match\s*\(" -A 2
rg "#\[.*\]" -A 2 # Attributes
rg "::class" -A 2 # Class name resolution
# Check composer.json for platform requirements
cat composer.json
# Check if there are any version-specific tests
fd -e php -e phpt test
Length of output: 3506
tests/Unit/SampleModels/PdoQueries.php (3)
38-38: LGTM! Consistent arrow function formatting.
The formatting changes to arrow functions are consistent across all occurrences and align with modern PHP coding standards.
Also applies to: 49-49, 87-87, 98-98, 127-127
147-147: LGTM! Consistent type casting format.
The formatting change for type casting is consistent with the codebase style.
13-13: LGTM! Constructor property promotion with readonly modifier.
The addition of the readonly modifier is a good improvement for immutability and aligns with dropping PHP 8.0 support, as this feature was introduced in PHP 8.1.
✅ Verification successful
Constructor property promotion with readonly modifier is correctly aligned with PHP requirements
The use of the readonly modifier in the constructor property promotion is fully compatible with the project's PHP requirements as verified in composer.json, which explicitly requires PHP >= 8.1. This is correct since the readonly property modifier was introduced in PHP 8.1.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify PHP version requirement in composer.json
rg -A 5 '"require":\s*{' | rg '"php":'
Length of output: 132
Script:
#!/bin/bash
# Let's try a simpler approach to check composer.json
cat composer.json | grep -A 5 '"require":'
Length of output: 162
src/Pdo.php (1)
15-16: LGTM! Constructor signature updated to use PHP 8.1+ nullable type syntax
The change from ResultCollection $collection = null to ?ResultCollection $collection = null is a good improvement that better leverages PHP 8.1's type system. This new syntax is more explicit about nullability and aligns well with dropping PHP 8.0 support.
README.md (2)
107-108: LGTM! Fixed typo in documentation.
The correction from "associatve" to "associative" improves documentation clarity.
130-130: Verify PHP version consistency across configuration files.
The PHP version requirement update from 8.0 to 8.1 aligns with the PR objective. Let's verify this change is consistent across all configuration files.
✅ Verification successful
PHP version requirement is consistently set to 8.1+ across configuration files
The verification confirms that:
composer.jsonexplicitly requires PHP>=8.1- No other configuration files contain PHP version specifications
- The README statement about PHP 8.1 requirement aligns with the configuration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check PHP version requirements across configuration files
# Expected: All files should specify PHP 8.1 as minimum version
# Check composer.json for PHP version
echo "Checking composer.json..."
if [ -f "composer.json" ]; then
cat composer.json | jq -r '.require.php'
fi
# Check other potential configuration files
echo "Checking other configuration files..."
rg -l "php.*8\.[0-9]" --type yaml --type json
Length of output: 281
tests/Unit/PdoClassTest.php (3)
82-82: LGTM! Improved assertion specificity.
The change to assertCount is more semantically appropriate for testing collection sizes and provides better error messages when tests fail.
98-98: LGTM! Verify PHP version compatibility.
The explicit namespace resolution for \PDO::FETCH_ASSOC is a good practice to prevent potential namespace conflicts.
Let's verify if this change was necessary due to any PHP 8.0/8.1 differences:
Line range hint 1-214: Verify PHP 8.0 specific test scenarios.
While the changes improve test quality, I don't see any explicit removal of PHP 8.0 specific test cases. Consider if any PHP 8.1+ specific features or behaviors need additional test coverage.
Let's check for any PHP 8.0 specific test cases that might need updating:
src/PdoStatement.php (3)
65-65: LGTM: Execute method signature updated for better type safety.
The change to ?array $params = null improves type safety while maintaining backward compatibility with the existing implementation.
37-37: LGTM: Constructor signature updated to modern PHP nullable type syntax.
The change from QueryLog $queryLog = null to ?QueryLog $queryLog = null aligns with PHP 8.1+ type system best practices, making nullability explicit in the type signature.
Let's verify PHP version requirements are consistently updated:
✅ Verification successful
Constructor signature change is consistent with PHP 8.1+ requirement
The verification confirms that:
- The composer.json explicitly requires PHP >= 8.1
- No PHP 8.0 references found in YAML or JSON configuration files
- The nullable type syntax change in the constructor aligns with the project's minimum PHP version requirement
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check composer.json for PHP version constraint
rg '"php":\s*"[^"]*"' -g 'composer.json'
# Check for any remaining PHP 8.0 references
rg '(php|PHP)\s*8\.0' --type yaml --type json
Length of output: 126
280-284: Verify compatibility with PDO's setFetchMode signature.
The method signature has been updated to use variadic arguments (...$args), which is more flexible but should match PDO's native behavior exactly.
Let's verify the signature matches PHP's native PDO implementation:
✅ Verification successful
Let me gather more information about the native PDO implementation by searching for class-related code.
Signature change is compatible with PDO's native implementation
The updated signature setFetchMode($mode, ...$args): bool|int is correct and matches PHP's native PDO behavior:
- It properly handles the basic fetch mode setting as shown in tests (PDO::FETCH_ASSOC, PDO::FETCH_COLUMN)
- The variadic
...$argsparameter correctly supports additional arguments needed for special fetch modes - The return type
bool|intmatches the native behavior where it returnstrue/1for success andfalsefor failure, as verified by the test cases
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for test cases using setFetchMode to ensure all use cases are covered
rg "setFetchMode" -t php -A 5 -B 5
# Look for any PDO extension interface definitions that might specify the method signature
ast-grep --pattern 'class $_ extends \PDOStatement {
$$$
public function setFetchMode($$$) {
$$$
}
$$$
}'
Length of output: 2804
Script:
#!/bin/bash
# Search for the implementation details
ast-grep --pattern 'class PdoStatement {
$$$
}'
# Look for any fetch mode related constants or usages
rg "FETCH_" -t php
# Search for any documentation or comments about setFetchMode
rg "setFetchMode" -t php --no-filename -A 2 -B 2
Length of output: 2947
tests/Unit/PdoStatementTest.php (3)
6-6: LGTM: Correct PDO class import
The change to use the proper PDO class name aligns with PHP's native PDO extension naming conventions.
44-44: LGTM: Consistent usage of PDO constants
The changes consistently use the proper PDO class constants for fetch modes across all test methods. This improves type safety and maintains consistency with PHP's native PDO implementation.
Also applies to: 63-63, 82-82, 166-166, 192-192, 217-217, 232-232, 239-239
169-169: LGTM: Improved assertion methods
The changes replace generic assertEquals(true/false, ...) with more specific assertTrue()/assertFalse() methods. This improves test readability and provides clearer failure messages, following PHPUnit best practices.
Also applies to: 187-187, 284-285, 292-292, 301-301, 317-317
| @@ -29,7 +29,7 @@ public function testAddQuery() | |||
| $queryLog = new QueryLog(); | |||
| $queryLog->addQuery($sql); | |||
| $queries = $queryLog->getQueries(); | |||
| $this->assertEquals(1, count($queries)); | |||
| $this->assertCount(1, $queries); | |||
There was a problem hiding this comment.
💡 Codebase verification
Consider standardizing count assertions across test files
The codebase shows mixed usage of count assertions:
assertEquals($count, $obj->count())in ResultCollectionTestassertCount()in ResultTest, QueryLogTest, and PdoClassTest
Consider updating the remaining count assertion in ResultCollectionTest.php to use assertCount for consistency with the rest of the codebase:
tests/Unit/ResultCollectionTest.php: ReplaceassertEquals(1, $results->count())withassertCount(1, $results)
🔗 Analysis chain
LGTM! Improved assertion clarity
The change from assertEquals(1, count($queries)) to assertCount(1, $queries) is a good improvement as it:
- Makes the assertion's intent clearer
- Uses the more specific PHPUnit assertion method
- Provides better failure messages
Let's check if there are similar patterns in other test files that could benefit from this improvement:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find similar assertEquals(n, count()) patterns in tests that could be replaced with assertCount
# Expected results: List of test files containing the pattern to help standardize assertions
rg -l 'assertEquals\s*\(\s*\d+\s*,\s*count\s*\(' tests/
Length of output: 57
Script:
#!/bin/bash
# Let's try a broader search for assertEquals with count, including variations
rg -U 'assertEquals.*count\(' tests/ -A 2
# Also search for assertCount usage to understand current practices
rg -U 'assertCount' tests/ -A 2
Length of output: 1011
Updated the return type of the setFetchMode method to always return true and throw a PseudoException for invalid modes. Adjusted unit tests to account for new exception throwing behavior, ensuring robustness in fetch mode validation.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Tests
Chores