Add PHP 8.4 Compatibility #28
Conversation
Unified dataset usage with `AnyDataset`, streamlined iterator methods, and improved type compatibility. Removed redundant `__id` and `__key` fields from test assertions and enhanced performance by optimizing cursor handling. Bumped dependency versions to ensure compatibility and new features.
Replaced `hasNext` and `moveNext` methods with standardized `current`, `next`, and `valid` implementations. Streamlined row handling logic in `PreFetchTrait` and adjusted test cases for consistency in prefetch behavior. Removed redundant methods in `DbIterator`.
![korbit-ai[bot]](https://avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
| Category | Issue | Fix Detected |
|---|---|---|
 |
Mixed Cache and Business Logic ▹ view | ✅ |
|
Split validation logic ▹ view | ✅ |
 |
Inefficient array reindexing operation ▹ view | ✅ |
Files scanned
| File Path | Reviewed |
|---|---|
| src/DbIterator.php | ✅ |
| src/Oci8Iterator.php | ✅ |
| src/Traits/PreFetchTrait.php | ✅ |
| src/SqlStatement.php | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Need a new review? Comment
/korbit-reviewon this PR and I'll review your latest changes.Korbit Guide: Usage and Customization
Interacting with Korbit
- You can manually ask Korbit to review your PR using the
/korbit-reviewcommand in a comment at the root of your PR.- You can ask Korbit to generate a new PR description using the
/korbit-generate-pr-descriptioncommand in any comment on your PR.- Too many Korbit comments? I can resolve all my comment threads if you use the
/korbit-resolvecommand in any comment on your PR.- On any given comment that Korbit raises on your pull request, you can have a discussion with Korbit by replying to the comment.
- Help train Korbit to improve your reviews by giving a 👍 or 👎 on the comments Korbit posts.
Customizing Korbit
- Check out our docs on how you can make Korbit work best for you and your team.
- Customize Korbit for your organization through the Korbit Console.
Feedback and Support
|
/korbit-review |
There was a problem hiding this comment.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
| Category | Issue | Fix Detected |
|---|---|---|
 |
Missing Timeout in Mutex Lock Loop ▹ view | |
 |
Incomplete PrepareStatement Implementation ▹ view | |
|
OCI8 Row Prefetching Disabled ▹ view | |
 |
SQL Injection in LIMIT clause ▹ view | |
|
Overly permissive mixed type hint ▹ view | |
|
Redundant Database Roundtrip for Identity Retrieval ▹ view | |
|
SQL Injection in Table Metadata Query ▹ view | |
 |
Insufficient exception documentation in forUpdate ▹ view |
Files scanned
| File Path | Reviewed |
|---|---|
| src/PdoOdbc.php | ✅ |
| src/PdoSqlite.php | ✅ |
| src/PdoPgsql.php | ✅ |
| src/PdoPdo.php | ✅ |
| src/PdoLiteral.php | ✅ |
| src/PdoOci.php | ✅ |
| src/Oci8Iterator.php | ✅ |
| src/Helpers/DbSqlsrvFunctions.php | ✅ |
| src/PdoMysql.php | ✅ |
| src/DbIterator.php | ✅ |
| src/PdoSqlsrv.php | ✅ |
| src/PdoDblib.php | ✅ |
| src/Helpers/DbPdoFunctions.php | ✅ |
| src/IteratorFilterSqlFormatter.php | ✅ |
| src/Traits/PreFetchTrait.php | ✅ |
| src/Helpers/DbSqliteFunctions.php | ✅ |
| src/Helpers/DbMysqlFunctions.php | ✅ |
| src/Helpers/DbPgsqlFunctions.php | ✅ |
| src/Helpers/DbDblibFunctions.php | ✅ |
| src/Helpers/DbOci8Functions.php | ✅ |
| src/Helpers/DbBaseFunctions.php | ✅ |
| src/SqlStatement.php | ✅ |
| src/DbPdoDriver.php | ✅ |
| src/DbOci8Driver.php | ✅ |
| src/Route.php | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Need a new review? Comment
/korbit-reviewon this PR and I'll review your latest changes.Korbit Guide: Usage and Customization
Interacting with Korbit
- You can manually ask Korbit to review your PR using the
/korbit-reviewcommand in a comment at the root of your PR.- You can ask Korbit to generate a new PR description using the
/korbit-generate-pr-descriptioncommand in any comment on your PR.- Too many Korbit comments? I can resolve all my comment threads if you use the
/korbit-resolvecommand in any comment on your PR.- On any given comment that Korbit raises on your pull request, you can have a discussion with Korbit by replying to the comment.
- Help train Korbit to improve your reviews by giving a 👍 or 👎 on the comments Korbit posts.
Customizing Korbit
- Check out our docs on how you can make Korbit work best for you and your team.
- Customize Korbit for your organization through the Korbit Console.
Current Korbit Configuration
General Settings
Setting Value Review Schedule Automatic excluding drafts Max Issue Count 10 Automatic PR Descriptions ✅ Issue Categories
Category Enabled Documentation ✅ Logging ✅ Error Handling ✅ Readability ✅ Design ✅ Performance ✅ Security ✅ Functionality ✅ Feedback and Support
Note
Korbit Pro is free for open source projects 🎉
Looking to add Korbit to your team? Get started with a free 2 week trial here
| while ($this->mutexIsLocked($cacheKey) !== false) { | ||
| usleep(200); | ||
| } |
There was a problem hiding this comment.
Missing Timeout in Mutex Lock Loop 
Tell me more
What is the issue?
The while loop that checks for mutex lock has no timeout mechanism, potentially leading to an infinite loop if the lock is never released.
Why this matters
Without a timeout, if the lock file remains due to a system crash or bug, the application could hang indefinitely waiting for the lock to clear.
Suggested change ∙ Feature Preview
$timeout = time() + 300; // 5 minute timeout
while ($this->mutexIsLocked($cacheKey) !== false) {
if (time() > $timeout) {
throw new RuntimeException("Timeout waiting for mutex lock to clear after 5 minutes");
}
usleep(200);
}Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
| public function prepareStatement(string $sql, ?array $params = null, ?array &$cacheInfo = []): mixed | ||
| { | ||
| // TODO: Implement prepareStatement() method. | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Incomplete PrepareStatement Implementation 
Tell me more
What is the issue?
The prepareStatement method is incompletely implemented, returning null instead of preparing the SQL statement through the matched route.
Why this matters
This can cause failures in query execution and statement preparation, leading to runtime errors when clients attempt to use prepared statements.
Suggested change ∙ Feature Preview
Implement the method to route the prepared statement to the appropriate database driver:
public function prepareStatement(string $sql, ?array $params = null, ?array &$cacheInfo = []): mixed
{
$dbDriver = $this->matchRoute($sql);
return $dbDriver->prepareStatement($sql, $params, $cacheInfo);
}Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
src/DbOci8Driver.php
Outdated
| @@ -146,6 +150,7 @@ public function executeCursor(mixed $statement): void | |||
| * @param int $preFetch | |||
| * @return GenericIterator | |||
| */ | |||
| #[Override] | |||
| public function getIterator(mixed $sql, ?array $params = null, ?CacheInterface $cache = null, DateInterval|int $ttl = 60, int $preFetch = 0): GenericIterator | |||
There was a problem hiding this comment.
OCI8 Row Prefetching Disabled 
Tell me more
What is the issue?
The prefetch value is defaulted to 0, which disables OCI8's built-in row prefetching capability.
Why this matters
Not utilizing OCI8's row prefetching can significantly impact performance when fetching large result sets, as it requires more round trips to the database server.
Suggested change ∙ Feature Preview
Set a reasonable default prefetch value (e.g., 100-1000 rows) to optimize memory usage and performance:
public function getIterator(mixed $sql, ?array $params = null, ?CacheInterface $cache = null, DateInterval|int $ttl = 60, int $preFetch = 1000): GenericIteratorProvide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
| if (stripos($sql, ' LIMIT ') === false) { | ||
| $sql = $sql . " LIMIT x OFFSET y"; | ||
| return $sql . " LIMIT $qty OFFSET $start"; | ||
| } |
There was a problem hiding this comment.
SQL Injection in LIMIT clause 
Tell me more
What is the issue?
The SQL injection vulnerability in the limit method where user-provided values $qty and $start are directly concatenated into the SQL string without proper escaping or parameterization.
Why this matters
Malicious users could inject SQL code through the $qty and $start parameters, potentially leading to unauthorized data access or database manipulation.
Suggested change ∙ Feature Preview
Use prepared statements or parameter binding. Here's a safer approach:
if (stripos($sql, ' LIMIT ') === false) {
return $sql . ' LIMIT ? OFFSET ?';
}And then bind the $qty and $start parameters when executing the query.
Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
src/DbPdoDriver.php
Outdated
| * @return GenericIterator The iterator for the query results | ||
| * @throws InvalidArgumentException If $sql is not a supported type | ||
| */ | ||
| #[Override] | ||
| public function getIterator(mixed $sql, ?array $params = null, ?CacheInterface $cache = null, DateInterval|int $ttl = 60, int $preFetch = 0): GenericIterator |
There was a problem hiding this comment.
Overly permissive mixed type hint 
Tell me more
What is the issue?
The $sql parameter is typed as 'mixed' which obscures the expected input types, despite the PHPDoc clearly stating the allowed types (PDOStatement, string, or SqlStatement).
Why this matters
Using 'mixed' type makes it harder for developers to understand valid inputs without reading documentation and increases the chance of runtime type errors.
Suggested change ∙ Feature Preview
public function getIterator(PDOStatement|string|SqlStatement $sql, ?array $params = null, ?CacheInterface $cache = null, DateInterval|int $ttl = 60, int $preFetch = 0): GenericIteratorProvide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
| // For SQLSRV, we use a more efficient method to get the inserted ID | ||
| $dbdataset->execute($sql, $param); | ||
|
|
||
| return $dbdataset->getScalar("select SCOPE_IDENTITY() as id"); |
There was a problem hiding this comment.
Redundant Database Roundtrip for Identity Retrieval
Tell me more
What is the issue?
The use of a separate SQL query to get SCOPE_IDENTITY() creates an additional unnecessary database roundtrip.
Why this matters
Each additional database roundtrip adds network latency and server load, especially impactful when inserting multiple records in a loop.
Suggested change ∙ Feature Preview
Modify the INSERT query to return the identity value in the same query using OUTPUT clause or combining with SCOPE_IDENTITY(), for example:
INSERT INTO table (column1, column2)
OUTPUT INSERTED.ID
VALUES (value1, value2)or
INSERT INTO table (column1, column2) VALUES (value1, value2); SELECT SCOPE_IDENTITY() as id;Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
| public function hasForUpdate(): bool | ||
| { | ||
| return true; | ||
| } | ||
|
|
||
| #[Override] | ||
| public function getTableMetadata(DbDriverInterface $dbdataset, string $tableName): array | ||
| { | ||
| $tableName = strtolower($tableName); | ||
| $sql = "select column_name, data_type || '(' || coalesce(cast(character_maximum_length as varchar), cast(numeric_precision_radix as varchar) || ',' || numeric_scale) || ')' as type, column_default, is_nullable from INFORMATION_SCHEMA.COLUMNS where table_name = '$tableName' "; |
There was a problem hiding this comment.
SQL Injection in Table Metadata Query
Tell me more
What is the issue?
Direct string interpolation of $tableName in SQL query creates a SQL injection vulnerability.
Why this matters
An attacker could manipulate the tableName parameter to execute malicious SQL commands, potentially accessing or modifying sensitive database information.
Suggested change ∙ Feature Preview
Use parameterized queries instead:
$sql = "select column_name, data_type || '(' || coalesce(cast(character_maximum_length as varchar), cast(numeric_precision_radix as varchar) || ',' || numeric_scale) || ')' as type, column_default, is_nullable from INFORMATION_SCHEMA.COLUMNS where table_name = $1";
return $this->getTableMetadataFromSql($dbdataset, $sql, [$tableName]);Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
| /** | ||
| * @param string $sql | ||
| * @return string | ||
| * @throws \ByJG\AnyDataset\Core\Exception\NotAvailableException | ||
| * @throws NotAvailableException | ||
| */ |
There was a problem hiding this comment.
Insufficient exception documentation in forUpdate 
Tell me more
What is the issue?
The forUpdate() method's docblock doesn't explain why NotAvailableException is thrown or the method's intended purpose.
Why this matters
Developers need to understand why the operation is not available to handle the exception appropriately.
Suggested change ∙ Feature Preview
/**
* Adds FOR UPDATE clause to SQL query for row locking
* @param string $sql The SQL query to modify
* @return string Modified SQL with FOR UPDATE clause
* @throws NotAvailableException SQLite does not support FOR UPDATE locking
*/
Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
Description by Korbit AI
What change is being made?
Enhance the codebase to ensure compatibility with PHP 8.4 by updating dependencies, modifying iterator classes and traits, and adjusting test cases accordingly.
Why are these changes being made?
To maintain the software's functionality and performance in PHP 8.4 by ensuring compatibility, which necessitated updates to dependencies and existing code that use features deprecated in PHP 8.4 or require updates to leverage new features. Testing modifications ensure the test suite accurately reflects the improved codebase capabilities.