Support for Doctrine DBAL 3 #122
Conversation
amotl
force-pushed
the
amo/doctrine3
branch
3 times, most recently
from
c0dddcc to
f1a7a40
Compare
|
Other than many deprecation warnings, because crate/crate-pdo#146 has not been merged and released yet, running the tests here 1 raise this fatal error 2: Fatal error: Declaration of Crate\PDO\PDO::prepare($statement, $options = null) must be compatible
with Doctrine\DBAL\Driver\Connection::prepare(string $sql): Doctrine\DBAL\Driver\Statement
in /path/to/crate-dbal/vendor/crate/crate-pdo/src/Crate/PDO/PDO.php on line 193Footnotes |
|
It will make no sense to continue working on this before crate/crate-pdo#146 has been merged. Otherwise, there are too many type errors to work around. In other words, fa8ef49 should be removed again. |
|
Hi @amotl I came across this and was wondering if this will be merge soon now that PR holding you back is merged? |
|
Dear Rico, thank you for writing in. I am not sure if this patch in its current state is enough to add support for Doctrine 3. Is it?
Ah, I see. Yeah, this would probably need a refresh. Thanks for the reminder! With kind regards, |
|
Hi @amotl, regards, |
|
Hi @andythedandy. Thank you for writing in and for offering support. This patch certainly needs more love to satisfy CI. Would you be able to spend a few cycles on it? On the other hand, GH-136 by @JulianMar may also be very relevant in this regard, and could make this patch obsolete, by skipping support for Doctrine 3 altogether. |
amotl
added
help wanted
important
WalkthroughMigrates the project to Doctrine DBAL 3: updates composer and changelog, refactors PDOCrate driver from inheritance to composition (connection, statement, result), adds Exception/ExceptionConverter types, adapts Driver/Platform/Schema APIs and types, and updates tests, docs, and examples. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant PDOConnection
participant CrateStatement
participant PDOStatement
participant Result
participant ExceptionConverter
Client->>PDOConnection: prepare(sql)
PDOConnection->>PDOStatement: native prepare(sql)
PDOConnection->>CrateStatement: wrap(native PDO, PDOStatement)
PDOConnection-->>Client: CrateStatement
Client->>CrateStatement: execute(params?)
CrateStatement->>PDOStatement: execute(params?)
alt success
PDOStatement-->>CrateStatement: success
CrateStatement->>Result: new Result(this)
CrateStatement-->>Client: Result
else failure
PDOStatement-->>CrateStatement: PDOException
CrateStatement->>ExceptionConverter: convert(PDOException, Query?)
ExceptionConverter-->>CrateStatement: DriverException
CrateStatement-->>Client: throws DriverException
end
Client->>Result: fetchAssociative()
Result->>CrateStatement: fetch(PDO::FETCH_ASSOC)
CrateStatement->>PDOStatement: fetch(...)
PDOStatement-->>CrateStatement: row|null
CrateStatement-->>Result: row|null
Result-->>Client: row|null
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| $row = $stmt->fetch(\PDO::FETCH_ASSOC); | ||
| $row = $result->fetchAssociative(); |
There was a problem hiding this comment.
The new variant reads much better.
NB: Please resolve after reading.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
amotl
changed the title
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/connect.rst (1)
68-72: Fix syntax error in ORM example: missing comma.The code example is missing a comma after the
'user'parameter, which will cause a PHP syntax error if copied directly.Apply this diff to fix the syntax error:
$params = array( 'driverClass' => 'Crate\DBAL\Driver\PDOCrate\Driver', 'user' => 'crate' + , 'host' => 'localhost',Or more clearly:
'user' => 'crate' - 'host' => 'localhost', + , 'host' => 'localhost',
♻️ Duplicate comments (1)
src/Crate/DBAL/Platforms/CratePlatform.php (1)
798-802: String length fallback broken - already flagged.This comparison between the DBAL type name and the SQL snippet will never match, breaking the 255 fallback for varchar columns without an explicit length. This issue was already identified in a previous review.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
docs/connect.rst(1 hunks)examples/objects.php(1 hunks)src/Crate/DBAL/Driver/PDOCrate/Driver.php(5 hunks)src/Crate/DBAL/Platforms/CratePlatform.php(7 hunks)test/Crate/Test/DBAL/Functional/ConnectionTest.php(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- examples/objects.php
- src/Crate/DBAL/Driver/PDOCrate/Driver.php
- test/Crate/Test/DBAL/Functional/ConnectionTest.php
🧰 Additional context used
🧬 Code graph analysis (1)
src/Crate/DBAL/Platforms/CratePlatform.php (3)
src/Crate/DBAL/Types/MapType.php (1)
getName(49-52)src/Crate/DBAL/Types/TimestampType.php (1)
getName(42-45)src/Crate/DBAL/Platforms/CratePlatform4.php (1)
getVarcharTypeDeclarationSQLSnippet(81-84)
🔇 Additional comments (6)
docs/connect.rst (1)
32-41: Doctrine DBAL 3 API changes correctly applied.The documentation has been properly updated to reflect DBAL 3 API changes:
- The
DriverManager::getConnection()invocation is the correct DBAL 3 API- The
createSchemaManager()method replaces the deprecatedgetSchemaManager()from DBAL 2- Import statement and reference links are updated to target DBAL 3 documentation
src/Crate/DBAL/Platforms/CratePlatform.php (5)
29-29: LGTM! Correct Doctrine 3 exception import.The import correctly updates to the new
Doctrine\DBAL\Exceptionclass while maintaining theDBALExceptionalias for backward compatibility within the file.
435-439: LGTM! Appropriate code style suppressions.The phpcs:ignore comments are correctly added for underscore-prefixed methods that override parent class methods from Doctrine's AbstractPlatform. These suppressions are necessary to maintain compatibility with the parent class signatures.
Also applies to: 674-699
49-59: No issues with removed parent constructor call.In Doctrine 3, AbstractPlatform::__construct() is an empty method with no initialization logic. Removing the parent constructor call in CratePlatform::__construct() has no functional impact and is appropriate for Doctrine 3 compatibility.
864-871: CURRENT_DATABASE() is correctly supported in CrateDB.The implementation of
getCurrentDatabaseExpression()is correct. CrateDB implements CURRENT_DATABASE() and returns the current database name, making this the appropriate function to return for the Doctrine 3 abstract method.
313-324: No issues found — code change is correct.The Doctrine 3 AbstractPlatform method signature accepts both array and Index types (
array|Index $columnsOrIndex), so passing the$indexobject directly is valid and properly aligned with the API.
| /** | ||
| * @group DBAL-228 | ||
| */ | ||
| public function testPrepareWithFetchAllBoth() | ||
| { | ||
| $paramInt = 1; | ||
| $paramStr = 'foo'; | ||
|
|
||
| $sql = "SELECT test_int, test_string FROM fetch_table WHERE test_int = ? AND test_string = ?"; | ||
| $stmt = $this->_conn->prepare($sql); | ||
| $this->assertInstanceOf('Doctrine\DBAL\Statement', $stmt); | ||
|
|
||
| $stmt->bindParam(1, $paramInt, PDO::PARAM_INT); | ||
| $stmt->bindParam(2, $paramStr, PDO::PARAM_STR); | ||
| $stmt->execute(); | ||
|
|
||
| $rows = $stmt->fetchAll(\PDO::FETCH_BOTH); | ||
| $rows[0] = array_change_key_case($rows[0], \CASE_LOWER); | ||
| $this->assertEquals(array('test_int' => 1, 'test_string' => 'foo', 0 => 1, 1 => 'foo'), $rows[0]); | ||
| } | ||
|
|
There was a problem hiding this comment.
FYI: Apparently, there is no longer support for the PDO::FETCH_BOTH fetch mode in Doctrine 3. It's a good idea to leave it behind, because it is one of the strangest fetch modes on earth.
NB: Please resolve after reading.
There was a problem hiding this comment.
FETCH_BOTH is not supported and I guess will not be supported. I don't know the usecase for this fetch mode--maybe someone can enlighten me and tell me why it's needed? Maybe we don't even need it anymore.
@JulianMar: I think DBAL dropped support for PDO::FETCH_BOTH for good reasons. There is no need to look back. 🌻
| Unreleased | ||
| ========== | ||
|
|
||
| - Added support for Doctrine 3, dropped support for Doctrine 2. |
There was a problem hiding this comment.
The versioning of Doctrine ORM is different, so let's be closer to the truth here, by discriminating between Doctrine ORM and Doctrine DBAL.
| - Added support for Doctrine 3, dropped support for Doctrine 2. | |
| - Added support for Doctrine DBAL 3, dropped support for Doctrine DBAL 2. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src/Crate/DBAL/Driver/PDOCrate/CrateStatement.php (1)
57-70: WrapPDOExceptioninexecute()to keep driver exceptions consistent
execute()currently delegates directly to$this->stmt->execute($params)without catchingPDOException. This is inconsistent withPDOConnectionandResult, which both wrapPDOExceptionvia the driver’sException::new()helper, and it breaks the expectation that driver implementations surfaceDoctrine\DBAL\Driver\Exceptioninstances.Align
execute()with the rest of the driver by catching and convertingPDOException:+use PDOException; @@ public function execute($params = null): ResultInterface { - if ($params !== null) { Deprecation::trigger( 'doctrine/dbal', 'https://github.com/doctrine/dbal/pull/5556', 'Passing $params to Statement::execute() is deprecated. Bind parameters using' . ' Statement::bindParam() or Statement::bindValue() instead.', ); } - $this->stmt->execute($params); - return new Result($this); + try { + $this->stmt->execute($params); + } catch (PDOException $exception) { + throw Exception::new($exception); + } + + return new Result($this); }This mirrors Doctrine’s own PDO statement implementation and keeps your
ExceptionConverterworking as intended.To confirm the expected behavior, please compare against the
execute()implementation in Doctrine DBAL’sDoctrine\DBAL\Driver\PDO\Statementfor the DBAL 3.x version you target:Doctrine DBAL 3 PDO Statement execute() implementation exception handlingsrc/Crate/DBAL/Platforms/CratePlatform.php (1)
799-803: String length fallback no longer triggers.This issue was flagged in a previous review:
$columnData['type']->getName()returns the DBAL type name (e.g.,string), whilegetVarcharTypeDeclarationSQLSnippet()returns the SQL keyword (STRING). Because these values are now compared directly, the condition is never true and varchar columns without an explicit length lose the historic 255 fallback.Apply this diff to restore the fallback:
- if (strtolower($columnData['type']->getName()) == - strtolower($platform->getVarcharTypeDeclarationSQLSnippet(0, false)) + if (strtolower($columnData['type']->getName()) === 'string' && $columnData['length'] === null) { $columnData['length'] = 255;Based on past review comments.
test/Crate/Test/DBAL/Functional/DataAccessTest.php (2)
116-118: Critical: Result::fetch(PDO::FETCH_ASSOC) not supported in DBAL 3.This was flagged in a previous review: the
Resultclass implementsDoctrine\DBAL\Driver\Resultand only exposes typed fetch methods (fetchAssociative(),fetchNumeric(),fetchOne(), etc.). It has no publicfetch(int $mode)method for PDO fetch modes. This call will fail at runtime with "Call to undefined method".- $row = $result->fetch(PDO::FETCH_ASSOC); + $row = $result->fetchAssociative();Based on past review comments.
212-215: Critical: Result::fetch(PDO::FETCH_ASSOC) not supported in DBAL 3.This was flagged in a previous review: the
Resultclass implementsDoctrine\DBAL\Driver\Resultand only exposes typed fetch methods. This call will fail at runtime.- $row = $result->fetch(PDO::FETCH_ASSOC); + $row = $result->fetchAssociative();Based on past review comments.
🧹 Nitpick comments (3)
.php-cs-fixer.php (2)
6-8: Consider excluding vendor and other non-source directories.The Finder currently scans the entire directory without exclusions, which will include
vendor/, build artifacts, and other directories that shouldn't be checked for coding standards.Apply this diff to exclude common directories:
$finder = (new PhpCsFixer\Finder()) ->in(__DIR__) + ->exclude('vendor') + ->exclude('build') ;
10-22: Minimal ruleset configuration.The configuration enables only
ordered_importswhile standard rulesets like@PSR12and@Symfonyare commented out. This appears intentional, but you may want to consider enabling additional rules in the future for more comprehensive code style enforcement.Additionally,
setUnsupportedPhpVersionAllowed(true)on line 11 allows PHP-CS-Fixer to run on unsupported PHP versions. While this may be necessary for your environment, it's worth verifying this is required.src/Crate/DBAL/Schema/CrateSchemaManager.php (1)
119-129: Avoid static calls to non‑staticflatten()
flatten()is declared as an instance method but invoked statically viaself::flatten(...). That works but is a bit awkward and may confuse tools/static analysis.You could either mark it static or call it via
$this:- private function flatten(array $array, string $prefix = ''): array + private static function flatten(array $array, string $prefix = ''): array @@ - foreach ($array as $key => $value) { - if (is_array($value)) { - $result = $result + self::flatten($value, $prefix . $key . '.'); - } else { - $result[$prefix . $key] = $value; - } - } + foreach ($array as $key => $value) { + if (is_array($value)) { + $result += self::flatten($value, $prefix . $key . '.'); + } else { + $result[$prefix . $key] = $value; + } + }(or keep it non‑static and call
$this->flatten(...)instead).Also applies to: 146-147
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
.gitignore(1 hunks).php-cs-fixer.php(1 hunks)composer.json(3 hunks)examples/objects.php(1 hunks)src/Crate/DBAL/Driver/PDOCrate/CrateStatement.php(2 hunks)src/Crate/DBAL/Driver/PDOCrate/Driver.php(6 hunks)src/Crate/DBAL/Driver/PDOCrate/ExceptionConverter.php(1 hunks)src/Crate/DBAL/Driver/PDOCrate/PDOConnection.php(3 hunks)src/Crate/DBAL/Platforms/CratePlatform.php(11 hunks)src/Crate/DBAL/Platforms/CratePlatform1.php(2 hunks)src/Crate/DBAL/Platforms/CratePlatform4.php(1 hunks)src/Crate/DBAL/Platforms/Keywords/CrateKeywords.php(2 hunks)src/Crate/DBAL/Schema/CrateSchemaManager.php(6 hunks)src/Crate/DBAL/Types/ArrayType.php(3 hunks)src/Crate/DBAL/Types/MapType.php(3 hunks)src/Crate/DBAL/Types/TimestampType.php(4 hunks)test/Crate/Test/DBAL/Functional/ConnectionTest.php(3 hunks)test/Crate/Test/DBAL/Functional/DataAccessTest.php(26 hunks)test/Crate/Test/DBAL/Functional/TypeConversionTest.php(6 hunks)test/Crate/Test/DBAL/Platforms/CratePlatformTest.php(8 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/Crate/DBAL/Platforms/Keywords/CrateKeywords.php
🚧 Files skipped from review as they are similar to previous changes (10)
- test/Crate/Test/DBAL/Platforms/CratePlatformTest.php
- test/Crate/Test/DBAL/Functional/ConnectionTest.php
- examples/objects.php
- src/Crate/DBAL/Types/ArrayType.php
- src/Crate/DBAL/Types/TimestampType.php
- src/Crate/DBAL/Types/MapType.php
- src/Crate/DBAL/Driver/PDOCrate/ExceptionConverter.php
- src/Crate/DBAL/Platforms/CratePlatform4.php
- .gitignore
- src/Crate/DBAL/Driver/PDOCrate/PDOConnection.php
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-14T11:14:50.150Z
Learnt from: amotl
Repo: crate/crate-dbal PR: 122
File: docs/appendices/table-options.rst:21-21
Timestamp: 2025-11-14T11:14:50.150Z
Learning: In Doctrine DBAL 3, the Table constructor signature changed from DBAL 2. The `int $idGeneratorType` parameter (previously 5th position) was removed and replaced with `array $uniqueConstraints` as the 4th parameter. The new signature is: __construct($name, array $columns = [], array $indexes = [], array $uniqueConstraints = [], array $fkConstraints = [], array $options = [])
Applied to files:
src/Crate/DBAL/Platforms/CratePlatform.php
🧬 Code graph analysis (6)
src/Crate/DBAL/Schema/CrateSchemaManager.php (2)
src/Crate/DBAL/Platforms/CratePlatform.php (2)
CratePlatform(41-873)getTableOptionsSQL(858-863)src/Crate/DBAL/Driver/PDOCrate/Result.php (1)
fetchAssociative(33-36)
test/Crate/Test/DBAL/Functional/TypeConversionTest.php (1)
src/Crate/DBAL/Types/TimestampType.php (1)
TimestampType(34-82)
src/Crate/DBAL/Driver/PDOCrate/CrateStatement.php (3)
src/Crate/DBAL/Driver/PDOCrate/Exception.php (1)
Exception(11-26)src/Crate/DBAL/Driver/PDOCrate/Result.php (5)
Result(12-108)__construct(17-20)columnCount(79-86)rowCount(70-77)fetch(100-107)src/Crate/DBAL/Driver/PDOCrate/PDOConnection.php (2)
__construct(45-50)prepare(71-78)
src/Crate/DBAL/Platforms/CratePlatform.php (3)
src/Crate/DBAL/Types/MapType.php (2)
MapType(37-117)getName(49-52)src/Crate/DBAL/Types/TimestampType.php (2)
TimestampType(34-82)getName(44-47)src/Crate/DBAL/Platforms/CratePlatform4.php (1)
getVarcharTypeDeclarationSQLSnippet(82-85)
test/Crate/Test/DBAL/Functional/DataAccessTest.php (4)
src/Crate/DBAL/Driver/PDOCrate/Result.php (6)
fetchAssociative(33-36)fetch(100-107)fetchAllAssociative(57-60)fetchNumeric(25-28)fetchOne(41-44)fetchAllNumeric(49-52)src/Crate/DBAL/Driver/PDOCrate/CrateStatement.php (6)
fetch(107-113)getWrappedStatement(145-148)fetchColumn(127-130)execute(57-70)bindValue(91-94)fetchAll(122-125)test/Crate/Test/DBAL/DBALFunctionalTestCase.php (2)
execute(108-111)refresh(113-116)src/Crate/DBAL/Driver/PDOCrate/PDOConnection.php (2)
prepare(71-78)query(94-103)
src/Crate/DBAL/Driver/PDOCrate/Driver.php (6)
src/Crate/DBAL/Platforms/CratePlatform.php (2)
CratePlatform(41-873)getName(461-464)src/Crate/DBAL/Platforms/CratePlatform1.php (1)
CratePlatform1(26-46)src/Crate/DBAL/Platforms/CratePlatform4.php (1)
CratePlatform4(26-94)src/Crate/DBAL/Schema/CrateSchemaManager.php (1)
CrateSchemaManager(37-151)src/Crate/DBAL/Driver/PDOCrate/PDOConnection.php (1)
PDOConnection(35-129)src/Crate/DBAL/Driver/PDOCrate/ExceptionConverter.php (1)
ExceptionConverter(23-58)
🔇 Additional comments (9)
src/Crate/DBAL/Platforms/CratePlatform1.php (1)
28-28: LGTM! Explicit visibility improves code clarity and aligns with the codebase pattern.Making the constant visibility explicitly
publicaligns with modern PHP best practices and is already established in the parentCratePlatformclass, confirming this is the appropriate pattern for the codebase.test/Crate/Test/DBAL/Functional/TypeConversionTest.php (1)
28-31: Doctrine 3 timestamp type migration here looks correctUsing
Types::DATETIMETZ_MUTABLE,Types::DATETIME_MUTABLE,Types::DATE_MUTABLE, andTypes::TIME_MUTABLEplus the importedTimestampTypekeeps the tests aligned with DBAL 3’s type system while preserving the original expectations and round‑trip semantics. No issues on this segment.Also applies to: 47-84, 99-103
src/Crate/DBAL/Schema/CrateSchemaManager.php (1)
26-37: Doctrine 3 alignment and Crate 6.1 compatibility look good
- Adding
CratePlatformand the@extends AbstractSchemaManager<CratePlatform>generic makes the schema manager play nicely with DBAL 3’s typed schema managers and static analysis.- The
is_nullablenormalization in_getPortableTableColumnDefinition()correctly handles both legacy boolean and newer string values frominformation_schema, and keeps thenotnulloption stable.- Switching to
$this->_conn->fetchAssociative()and returningnew Table($name, …)with$namethreaded through is consistent with DBAL 3 APIs and the updated driver’sgetSchemaManager().No functional issues spotted in this block.
Also applies to: 43-45, 63-96, 101-111, 119-150
composer.json (1)
19-21: The original review comment is incorrect—php-cs-fixer checkis a valid command.The
checkcommand is a valid php-cs-fixer subcommand that acts as a shorthand forfix --dry-run. The current composer.json script definition using"check-style": "php-cs-fixer check"is correct and will not fail. No changes are needed.Likely an incorrect or invalid review comment.
src/Crate/DBAL/Driver/PDOCrate/Driver.php (1)
26-39: Review comment is incorrect—code violates Doctrine DBAL 3 Driver interface contractThe actual implementation has critical mismatches with Doctrine DBAL 3 interface requirements:
connect()signature mismatch: DBAL 3 interface requiresconnect(array $params, ?string $username = null, ?string $password = null, array $driverOptions = []): Connection, but Driver.php implementsconnect(array $params): PDOConnection. The DBAL 3 interface expects separate parameters; this implementation extracts them from the$paramsarray.
getSchemaManager()interface violation: DBAL 3 interface requiresgetSchemaManager(Connection $conn): AbstractSchemaManager(1 parameter), but Driver.php implementsgetSchemaManager(Connection $conn, AbstractPlatform $platform): AbstractSchemaManager(2 parameters). The extraAbstractPlatformparameter violates the interface contract.
getExceptionConverter()not in interface: The method at line 125 is not part of the standard Doctrine DBAL 3 Driver interface per official documentation.The review comment's claims about interface compliance are unsupported by verification.
Likely an incorrect or invalid review comment.
src/Crate/DBAL/Platforms/CratePlatform.php (2)
865-872: LGTM: New DBAL 3 method correctly implemented.The
getCurrentDatabaseExpression()method is a new requirement in Doctrine DBAL 3's platform API. The implementation returning'CURRENT_DATABASE()'aligns with CrateDB's SQL dialect.
50-60: Review comment is incorrect.There is no special required constructor work in DBAL 3 — AbstractPlatform itself doesn't mandate parameters. Platform initialization happens via initialization methods like
initializeDoctrineTypeMappings()andregisterDoctrineTypeMapping(). The code correctly implements this pattern by callinginitializeDoctrineTypeMappings()and registering custom types. The removal ofparent::__construct()is safe and aligns with DBAL 3 best practices.Likely an incorrect or invalid review comment.
test/Crate/Test/DBAL/Functional/DataAccessTest.php (2)
48-67: LGTM: Schema manager API correctly migrated to DBAL 3.The change from
getSchemaManager()tocreateSchemaManager()and directTableinstantiation align with Doctrine DBAL 3's updated schema management API. The table definition correctly uses DBAL types and platform options.
277-280: LGTM: Date-time type constant correctly updated.The migration from
Type::DATETIMEtoTypes::DATETIME_MUTABLEis correct for DBAL 3, which introduced a newTypesclass with explicit mutable/immutable datetime type constants.
| private PDOInterface $pdo; | ||
| private PDOStatement $stmt; | ||
|
|
||
| /** | ||
| * @param string $sql | ||
| * @param array<string,mixed> $options | ||
| */ | ||
| public function __construct(PDOInterface $pdo, $sql, $options = []) | ||
| { | ||
| $this->pdo = $pdo; | ||
| $this->stmt = $pdo->prepare($sql, $options); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Constructor now correctly forwards options to the underlying PDO connection
Using composition with PDOInterface/PDOStatement and updating the constructor to accept $options and call $pdo->prepare($sql, $options) aligns CrateStatement with PDOConnection::prepare() and preserves driver options such as bulk mode. This is the right shape for DBAL 3’s PDO‑backed driver.
If you want an extra sanity check, you can grep for all new CrateStatement( call sites to ensure they always pass the expected three arguments:
🏁 Script executed:
#!/bin/bash
rg -n "new\s+CrateStatement\(" -C2Length of output: 443
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for execute() method in CrateStatement
ast-grep --pattern $'class CrateStatement {
$$$
execute($$$) {
$$$
}
$$$
}'
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Also check with rg to see the actual execute() implementation
rg -n "public\s+function\s+execute" src/Crate/DBAL/Driver/PDOCrate/CrateStatement.php -A10Length of output: 461
---
🏁 Script executed:
```shell
#!/bin/bash
# Get more context on execute() method — show full implementation
rg -n "public\s+function\s+execute" src/Crate/DBAL/Driver/PDOCrate/CrateStatement.php -A30
Length of output: 859
Constructor correctly forwards options, but execute() must catch and wrap PDOException
Constructor alignment with PDOConnection::prepare() is correct. However, execute() directly delegates to $this->stmt->execute($params) without catching PDOException. For consistency with PDOConnection and the DBAL driver contract, wrap the call:
public function execute($params = null): ResultInterface
{
if ($params !== null) {
Deprecation::trigger(
'doctrine/dbal',
'https://github.com/doctrine/dbal/pull/5556',
'Passing $params to Statement::execute() is deprecated. Bind parameters using'
. ' Statement::bindParam() or Statement::bindValue() instead.',
);
}
try {
$this->stmt->execute($params);
} catch (PDOException $exception) {
throw Exception::new($exception);
}
return new Result($this);
}This ensures PDOException is converted to the driver-level Exception before reaching the caller, maintaining abstraction consistency.
🤖 Prompt for AI Agents
In src/Crate/DBAL/Driver/PDOCrate/CrateStatement.php around lines 41 to 52, the
constructor is fine but the execute() method currently calls
$this->stmt->execute($params) without catching PDOException; update execute() to
(1) keep the existing deprecation trigger when $params !== null, (2) wrap the
call to $this->stmt->execute($params) in a try/catch that catches PDOException
and rethrows it as the driver-level Exception via Exception::new($exception),
and (3) return the Result as before — this ensures PDOExceptions are converted
to the DBAL driver Exception before propagating.
It also has a ruleset to order `use` statements. - https://github.com/PHP-CS-Fixer/PHP-CS-Fixer - https://cs.symfony.com/doc/rules/import/ordered_imports.html Before: PHP_CodeSniffer - https://github.com/PHPCSStandards/PHP_CodeSniffer
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
test/Crate/Test/DBAL/Functional/Schema/SchemaManagerTest.php (1)
76-118: Inconsistent migration to Types constants.The migration to
Typesconstants is incomplete:
- Line 85 still uses string
'integer'instead ofTypes::INTEGER- Lines 93-94 use string literals
'integer'and'string'inType::getType()calls instead ofTypes::INTEGERandTypes::STRINGWhile Lines 79, 81-84, and 106 correctly use
Typesconstants, the inconsistent usage creates confusion and doesn't fully align with Doctrine DBAL 3 best practices.Apply this diff to ensure consistent usage:
- $table->addColumn('id', 'integer', array('notnull' => true)); + $table->addColumn('id', Types::INTEGER, array('notnull' => true));- // Those intentionally use the `Type::getType()` notation and resolve to DBAL types. $mapOpts = array( 'type' => MapType::STRICT, 'fields' => array( - new Column('id', Type::getType('integer'), array()), - new Column('name', Type::getType('string'), array()), + new Column('id', Type::getType(Types::INTEGER), array()), + new Column('name', Type::getType(Types::STRING), array()), ), );src/Crate/DBAL/Schema/CrateSchemaManager.php (1)
119-129: Makeflatten()static to match its usage pattern.Verified:
flatten()is declared as a non-static method (line 119) but is called statically viaself::flatten()at lines 124 and 146. The method contains no$thisusage, so declaring it static aligns with actual usage and eliminates potential warnings.Apply the suggested fix:
- private function flatten(array $array, string $prefix = ''): array + private static function flatten(array $array, string $prefix = ''): arraytest/Crate/Test/DBAL/Functional/NamedParametersTest.php (1)
168-178: Update line 171 to use DBAL 3fetchAllAssociative()method.The codebase requires DBAL 3 (as specified in composer.json:
"doctrine/dbal": "^3"), which removed thefetchAll($mode)API. TheResultobject returned byexecuteQuery()only exposesfetchAllAssociative()and similar explicit fetch methods. Other tests in the same directory (ModifyLimitQueryTest.php, BindingTest.php) and examples already use the correct API. The suggested fix is correct:- $result = $stmt->fetchAll(\PDO::FETCH_ASSOC); + $result = $stmt->fetchAllAssociative();
♻️ Duplicate comments (6)
CHANGES.txt (1)
8-8: Clarify that this refers specifically to Doctrine DBAL.To avoid confusion with Doctrine ORM versioning, please spell out that this is about DBAL:
- - Added support for Doctrine 3, dropped support for Doctrine 2. + - Added support for Doctrine DBAL 3, dropped support for Doctrine DBAL 2.test/Crate/Test/DBAL/Platforms/CratePlatformTest.php (1)
106-109: Align skip messages for index-renaming tests.
testAlterTableChangeQuotedColumn()and the new rename-index tests correctly mark index-renaming paths as unsupported. For consistency with previous review feedback,testAlterTableRenameIndex()/testQuotesAlterTableRenameIndex()should also mention “renaming indexes” rather than genericALTER TABLEin the skip message:- $this->markTestSkipped('Platform does not support ALTER TABLE.'); + $this->markTestSkipped('Platform does not support renaming indexes.');Also applies to: 174-185, 516-523, 525-533
src/Crate/DBAL/Platforms/CratePlatform.php (1)
799-801: Varchar length fallback broken in CratePlatform4.This condition compares the DBAL type name ('string') against the platform's SQL snippet ('TEXT' in CratePlatform4), so it never matches and varchar columns without an explicit length lose the 255 fallback.
Apply this diff to fix the comparison:
- if (strtolower($columnData['type']->getName()) == - strtolower($platform->getVarcharTypeDeclarationSQLSnippet(0, false)) + if (strtolower($columnData['type']->getName()) === 'string' && $columnData['length'] === null) {test/Crate/Test/DBAL/Functional/DataAccessTest.php (2)
116-116: Fix runtime error: Replacefetch(PDO::FETCH_ASSOC)withfetchAssociative().
Result::fetch(PDO::FETCH_ASSOC)is not part of the Doctrine DBAL 3 Result API. TheResultclass only exposes typed methods likefetchAssociative(),fetchNumeric(), etc. This call will fail at runtime with "Call to undefined method".Apply this diff:
- $row = $result->fetch(PDO::FETCH_ASSOC); + $row = $result->fetchAssociative();
212-212: Fix runtime error: Replacefetch(PDO::FETCH_ASSOC)withfetchAssociative().Same issue as line 116:
Result::fetch(PDO::FETCH_ASSOC)does not exist in Doctrine DBAL 3's Result API. Use the typedfetchAssociative()method instead.Apply this diff:
- $row = $result->fetch(PDO::FETCH_ASSOC); + $row = $result->fetchAssociative();src/Crate/DBAL/Driver/PDOCrate/CrateStatement.php (1)
57-70: Missing PDOException wrapping inexecute().The
execute()method calls$this->stmt->execute($params)without catchingPDOException. This is inconsistent with other driver methods (PDOConnection::prepare(),PDOConnection::exec(), etc.) which wrap PDO exceptions viaException::new(). RawPDOExceptioninstances will bypass the driver's exception converter.Apply this diff to wrap the exception:
+use PDOException; + public function execute($params = null): ResultInterface { if ($params !== null) { Deprecation::trigger( 'doctrine/dbal', 'https://github.com/doctrine/dbal/pull/5556', 'Passing $params to Statement::execute() is deprecated. Bind parameters using' . ' Statement::bindParam() or Statement::bindValue() instead.', ); } - $this->stmt->execute($params); - return new Result($this); + try { + $this->stmt->execute($params); + } catch (PDOException $exception) { + throw Exception::new($exception); + } + + return new Result($this); }
🧹 Nitpick comments (3)
test/Crate/Test/DBAL/Functional/ModifyLimitQueryTest.php (1)
68-82: Limit/offset tests and result fetching remain consistent after API changes.The additional inserts in
testModifyLimitQuerySimpleQuery(), the added: voidhints, and the switch tofetchAllAssociative()inassertLimitResult()preserve the original expectations while using the DBAL 3 result API.As a small cleanup, you could also add a
: voidreturn type totestModifyLimitQueryOrderBy()for consistency with the other test methods.Also applies to: 100-114, 116-125
test/Crate/Test/DBAL/Functional/WriteTest.php (1)
80-97: Use ofexecuteStatement()andfetchAllAssociative()aligns with DBAL 3 behavior.
executeStatement()is correctly used to assert affected row counts for prepared statements with various binding styles (PDO types, DBAL type objects, and DBAL type names).- In
testDelete(), switching tofetchAllAssociative()when checking the remaining rows is consistent with the new DBAL result API.As a minor readability tweak, you could replace
count($this->_conn->fetchAllAssociative(...))withassertCount():- $this->assertEquals(1, count($this->_conn->fetchAllAssociative('SELECT * FROM write_table'))); + $this->assertCount(1, $this->_conn->fetchAllAssociative('SELECT * FROM write_table')); - $this->assertEquals(0, count($this->_conn->fetchAllAssociative('SELECT * FROM write_table'))); + $this->assertCount(0, $this->_conn->fetchAllAssociative('SELECT * FROM write_table'));Also applies to: 115-152, 154-167, 200-205
test/Crate/Test/DBAL/Platforms/CratePlatformTest.php (1)
508-515: New data provider and type-mapping tests look good, but type override leaks globally.
- Making
asciiStringSqlDeclarationDataProvider()public staticwith an explicitarrayreturn type fits PHPUnit’s expectations and DBAL’s test style.testRegistersCommentedDoctrineMappingTypeImplicitly()andtestCaseInsensitiveDoctrineTypeMappingFromType()add valuable coverage for type mapping and case-insensitivity.However,
testCaseInsensitiveDoctrineTypeMappingFromType()overrides the global Doctrine type'map'without restoring it, which can affect subsequent tests that rely on the crate-specificMapType.Consider restoring the original mapping after the assertion, for example:
- self::assertSame($type->getName(), $this->platform->getDoctrineTypeMapping('ObJecT')); + self::assertSame($type->getName(), $this->platform->getDoctrineTypeMapping('ObJecT')); + + // Restore original CrateDB "map" type to avoid leaking into other tests. + Type::overrideType('map', MapType::class);Or alternatively, register the anonymous test type under a unique name instead of
'map'.Also applies to: 535-575
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (34)
.gitignore(1 hunks).php-cs-fixer.php(1 hunks)CHANGES.txt(1 hunks)composer.json(3 hunks)docs/appendices/data-types.rst(1 hunks)docs/appendices/table-options.rst(2 hunks)docs/connect.rst(1 hunks)examples/objects.php(1 hunks)src/Crate/DBAL/Driver/PDOCrate/CrateStatement.php(2 hunks)src/Crate/DBAL/Driver/PDOCrate/Driver.php(6 hunks)src/Crate/DBAL/Driver/PDOCrate/Exception.php(1 hunks)src/Crate/DBAL/Driver/PDOCrate/ExceptionConverter.php(1 hunks)src/Crate/DBAL/Driver/PDOCrate/PDOConnection.php(3 hunks)src/Crate/DBAL/Driver/PDOCrate/Result.php(1 hunks)src/Crate/DBAL/Platforms/CratePlatform.php(11 hunks)src/Crate/DBAL/Platforms/CratePlatform1.php(2 hunks)src/Crate/DBAL/Platforms/CratePlatform4.php(1 hunks)src/Crate/DBAL/Platforms/Keywords/CrateKeywords.php(2 hunks)src/Crate/DBAL/Schema/CrateSchemaManager.php(6 hunks)src/Crate/DBAL/Types/ArrayType.php(3 hunks)src/Crate/DBAL/Types/MapType.php(3 hunks)src/Crate/DBAL/Types/TimestampType.php(4 hunks)test/Crate/Test/DBAL/DBALFunctionalTestCase.php(1 hunks)test/Crate/Test/DBAL/Functional/BindingTest.php(5 hunks)test/Crate/Test/DBAL/Functional/ConnectionTest.php(3 hunks)test/Crate/Test/DBAL/Functional/DataAccessTest.php(26 hunks)test/Crate/Test/DBAL/Functional/ModifyLimitQueryTest.php(6 hunks)test/Crate/Test/DBAL/Functional/NamedParametersTest.php(3 hunks)test/Crate/Test/DBAL/Functional/Schema/SchemaManagerTest.php(6 hunks)test/Crate/Test/DBAL/Functional/TableOptionsTest.php(4 hunks)test/Crate/Test/DBAL/Functional/TypeConversionTest.php(6 hunks)test/Crate/Test/DBAL/Functional/Types/MapTypeTest.php(2 hunks)test/Crate/Test/DBAL/Functional/WriteTest.php(7 hunks)test/Crate/Test/DBAL/Platforms/CratePlatformTest.php(8 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/Crate/DBAL/Platforms/CratePlatform4.php
- src/Crate/DBAL/Platforms/Keywords/CrateKeywords.php
🚧 Files skipped from review as they are similar to previous changes (13)
- test/Crate/Test/DBAL/Functional/BindingTest.php
- examples/objects.php
- .gitignore
- src/Crate/DBAL/Types/ArrayType.php
- src/Crate/DBAL/Types/MapType.php
- composer.json
- test/Crate/Test/DBAL/Functional/Types/MapTypeTest.php
- test/Crate/Test/DBAL/DBALFunctionalTestCase.php
- docs/appendices/table-options.rst
- docs/connect.rst
- test/Crate/Test/DBAL/Functional/ConnectionTest.php
- src/Crate/DBAL/Driver/PDOCrate/Result.php
- .php-cs-fixer.php
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-14T11:14:50.172Z
Learnt from: amotl
Repo: crate/crate-dbal PR: 122
File: docs/appendices/table-options.rst:21-21
Timestamp: 2025-11-14T11:14:50.172Z
Learning: In Doctrine DBAL 3, the Table constructor signature changed from DBAL 2. The `int $idGeneratorType` parameter (previously 5th position) was removed and replaced with `array $uniqueConstraints` as the 4th parameter. The new signature is: __construct($name, array $columns = [], array $indexes = [], array $uniqueConstraints = [], array $fkConstraints = [], array $options = [])
Applied to files:
test/Crate/Test/DBAL/Functional/TableOptionsTest.phptest/Crate/Test/DBAL/Functional/DataAccessTest.phpsrc/Crate/DBAL/Platforms/CratePlatform.phptest/Crate/Test/DBAL/Functional/Schema/SchemaManagerTest.phptest/Crate/Test/DBAL/Functional/NamedParametersTest.php
🧬 Code graph analysis (13)
test/Crate/Test/DBAL/Functional/TypeConversionTest.php (1)
src/Crate/DBAL/Types/TimestampType.php (1)
TimestampType(34-82)
src/Crate/DBAL/Driver/PDOCrate/Exception.php (1)
src/Crate/DBAL/Driver/PDOCrate/Driver.php (1)
Driver(35-130)
src/Crate/DBAL/Driver/PDOCrate/ExceptionConverter.php (3)
src/Crate/DBAL/Driver/PDOCrate/Driver.php (1)
Driver(35-130)src/Crate/DBAL/Driver/PDOCrate/Exception.php (1)
Exception(11-26)src/Crate/DBAL/Driver/PDOCrate/PDOConnection.php (1)
query(94-103)
src/Crate/DBAL/Driver/PDOCrate/PDOConnection.php (5)
src/Crate/DBAL/Driver/PDOCrate/Driver.php (1)
Driver(35-130)src/Crate/DBAL/Driver/PDOCrate/Exception.php (2)
Exception(11-26)new(13-25)src/Crate/DBAL/Driver/PDOCrate/Result.php (1)
Result(12-108)src/Crate/DBAL/Driver/PDOCrate/CrateStatement.php (2)
CrateStatement(39-149)execute(57-70)test/Crate/Test/DBAL/DBALFunctionalTestCase.php (1)
execute(108-111)
test/Crate/Test/DBAL/Platforms/CratePlatformTest.php (3)
src/Crate/DBAL/Platforms/CratePlatform.php (2)
getDecimalTypeDeclarationSQL(396-399)getDoctrineTypeMapping(548-555)src/Crate/DBAL/Types/ArrayType.php (1)
getSQLDeclaration(81-86)src/Crate/DBAL/Types/MapType.php (1)
getSQLDeclaration(87-93)
test/Crate/Test/DBAL/Functional/DataAccessTest.php (5)
src/Crate/DBAL/Types/MapType.php (1)
MapType(37-117)src/Crate/DBAL/Driver/PDOCrate/Result.php (6)
fetchAssociative(33-36)fetch(100-107)fetchAllAssociative(57-60)fetchNumeric(25-28)fetchOne(41-44)fetchAllNumeric(49-52)src/Crate/DBAL/Driver/PDOCrate/CrateStatement.php (6)
fetch(107-113)getWrappedStatement(145-148)fetchColumn(127-130)execute(57-70)bindValue(91-94)fetchAll(122-125)test/Crate/Test/DBAL/DBALFunctionalTestCase.php (2)
execute(108-111)refresh(113-116)src/Crate/DBAL/Driver/PDOCrate/PDOConnection.php (2)
prepare(71-78)query(94-103)
src/Crate/DBAL/Platforms/CratePlatform.php (2)
src/Crate/DBAL/Types/MapType.php (2)
MapType(37-117)getName(49-52)src/Crate/DBAL/Platforms/CratePlatform4.php (1)
getVarcharTypeDeclarationSQLSnippet(82-85)
test/Crate/Test/DBAL/Functional/Schema/SchemaManagerTest.php (1)
src/Crate/DBAL/Types/TimestampType.php (1)
TimestampType(34-82)
src/Crate/DBAL/Schema/CrateSchemaManager.php (2)
src/Crate/DBAL/Platforms/CratePlatform.php (2)
CratePlatform(41-873)getTableOptionsSQL(858-863)src/Crate/DBAL/Driver/PDOCrate/Result.php (1)
fetchAssociative(33-36)
test/Crate/Test/DBAL/Functional/WriteTest.php (4)
src/Crate/DBAL/Driver/PDOCrate/PDOConnection.php (1)
prepare(71-78)src/Crate/DBAL/Driver/PDOCrate/CrateStatement.php (4)
getWrappedStatement(145-148)execute(57-70)fetchColumn(127-130)bindValue(91-94)test/Crate/Test/DBAL/DBALFunctionalTestCase.php (2)
execute(108-111)refresh(113-116)src/Crate/DBAL/Driver/PDOCrate/Result.php (1)
fetchAllAssociative(57-60)
src/Crate/DBAL/Driver/PDOCrate/CrateStatement.php (3)
src/Crate/DBAL/Driver/PDOCrate/Exception.php (1)
Exception(11-26)src/Crate/DBAL/Driver/PDOCrate/Result.php (5)
Result(12-108)__construct(17-20)columnCount(79-86)rowCount(70-77)fetch(100-107)src/Crate/DBAL/Driver/PDOCrate/PDOConnection.php (2)
__construct(45-50)prepare(71-78)
test/Crate/Test/DBAL/Functional/ModifyLimitQueryTest.php (1)
src/Crate/DBAL/Driver/PDOCrate/Result.php (1)
fetchAllAssociative(57-60)
src/Crate/DBAL/Driver/PDOCrate/Driver.php (6)
src/Crate/DBAL/Platforms/CratePlatform.php (2)
CratePlatform(41-873)getName(461-464)src/Crate/DBAL/Platforms/CratePlatform1.php (1)
CratePlatform1(26-46)src/Crate/DBAL/Platforms/CratePlatform4.php (1)
CratePlatform4(26-94)src/Crate/DBAL/Schema/CrateSchemaManager.php (1)
CrateSchemaManager(37-151)src/Crate/DBAL/Driver/PDOCrate/PDOConnection.php (1)
PDOConnection(35-129)src/Crate/DBAL/Driver/PDOCrate/ExceptionConverter.php (1)
ExceptionConverter(23-58)
🔇 Additional comments (32)
docs/appendices/data-types.rst (1)
92-93: Good cleanup of formatting.The removal of extra spaces improves readability and consistency in the code examples without altering functionality or meaning.
src/Crate/DBAL/Platforms/CratePlatform1.php (1)
28-28: LGTM: Explicit visibility improves clarity.Making the constant visibility explicitly
publicis good practice and aligns with the Doctrine DBAL 3 upgrade. Since PHP class constants are public by default, this is a non-breaking change that improves code clarity.test/Crate/Test/DBAL/Functional/TableOptionsTest.php (4)
35-38: LGTM: Correct migration to DBAL 3 SchemaManager API.The changes from
getSchemaManager()tocreateSchemaManager()correctly implement the Doctrine DBAL 3 API. In DBAL 3,getSchemaManager()is deprecated andcreateSchemaManager()is the recommended approach.
57-57: LGTM: Table constructor correctly updated for DBAL 3.The Table instantiation correctly follows the Doctrine DBAL 3 signature with six parameters:
(name, columns, indexes, uniqueConstraints, fkConstraints, options). The oldidGeneratorTypeparameter has been properly replaced with the newuniqueConstraintsandfkConstraintsarray parameters.
81-86: LGTM: Consistent DBAL 3 migration in test method.Both the Table constructor (line 81) and SchemaManager usage (line 86) correctly follow the Doctrine DBAL 3 patterns established elsewhere in this test file.
107-107: LGTM: Table constructor consistently updated.The Table instantiation in the exception test follows the same correct DBAL 3 pattern used throughout this test file.
src/Crate/DBAL/Types/TimestampType.php (3)
2-28: LGTM! Formatting aligns with php-cs-fixer rules.The blank lines and import ordering changes are appropriate formatting improvements applied by php-cs-fixer.
36-37: LGTM! Public visibility is required for Doctrine 3.Making these constants public is necessary for Doctrine 3 compatibility, allowing external code to reference
TimestampType::NAMEandTimestampType::S_TO_MS(as seen in the test file at line 88).
52-52: LGTM! Consistent operator spacing.The added spacing around arithmetic operators follows standard PHP formatting conventions.
Also applies to: 66-66
test/Crate/Test/DBAL/Functional/TypeConversionTest.php (2)
28-30: LGTM! Required imports added for Doctrine 3.The
TimestampTypeandTypesimports are necessary for referencing type constants used throughout the test methods.
47-47: LGTM! Correct migration to mutable type variants.The switch to
_MUTABLEtype constants is the correct approach for Doctrine 3 compatibility. Doctrine 3 distinguishes between mutable and immutable DateTime types, and using the mutable variants preserves backward compatibility with PHP's mutableDateTimeclass (used in the tests at line 44).Also applies to: 57-57, 67-67, 77-77, 99-105
test/Crate/Test/DBAL/Functional/Schema/SchemaManagerTest.php (4)
31-31: LGTM!The import of
Typesis correct for Doctrine DBAL 3, where type constants were moved from theTypeclass to the dedicatedTypesclass.
40-44: LGTM!The migration from
getSchemaManager()tocreateSchemaManager()is correct for Doctrine DBAL 3 compatibility.
212-221: LGTM!The
Tableconstructor signature has been correctly updated for Doctrine DBAL 3, replacing the removed$idGeneratorTypeparameter with the proper parameter order.Based on learnings.
223-233: LGTM!The
Tableconstructor signature has been correctly updated for Doctrine DBAL 3.Based on learnings.
src/Crate/DBAL/Driver/PDOCrate/ExceptionConverter.php (1)
22-57: Exception conversion logic matches Doctrine DBAL 3 expectations.The converter cleanly maps Crate error codes to the appropriate Doctrine exceptions, preserves the PHP 64705 workaround for connection errors, and falls back to a generic
DriverExceptionfor unknown codes. This looks correct and compatible with DBAL 3’sExceptionConverterInterface.test/Crate/Test/DBAL/Functional/ModifyLimitQueryTest.php (1)
26-51: Schema setup/teardown updated correctly for DBAL 3.Using
Tabledirectly and switching to$this->_conn->createSchemaManager()in bothsetUp()andtearDown()is aligned with Doctrine DBAL 3’s API and keeps the tests’ behavior intact.Also applies to: 55-65
test/Crate/Test/DBAL/Functional/NamedParametersTest.php (1)
7-9: Table construction and schema manager usage are in line with DBAL 3.Using the imported
Tableclass and replacinggetSchemaManager()withcreateSchemaManager()in bothsetUp()andtearDown()is consistent with Doctrine DBAL 3 and matches the updatedTableconstructor signature.
Based on learningsAlso applies to: 110-121, 148-158
src/Crate/DBAL/Driver/PDOCrate/Exception.php (1)
7-25: PDOException wrapper correctly normalizes SQLSTATE and error code.
Exception::new()handles both theerrorInfoand legacygetCode()paths and always produces a driver exception with a well-defined SQLSTATE and numeric code, while preserving the original PDOException as the previous exception. This matches Doctrine’s expectations for driver exceptions.src/Crate/DBAL/Schema/CrateSchemaManager.php (1)
135-150:listTableDetails()matches DBAL 3 APIs for fetch and Table construction.Using
$this->_conn->fetchAssociative($this->_platform->getTableOptionsSQL($name))and constructingnew Table($name, $columns, $indexes, [], [], $options)aligns with Doctrine DBAL 3’sConnectionandTableAPIs and keeps the existing Crate-specific table options wiring intact.
Based on learningstest/Crate/Test/DBAL/Functional/WriteTest.php (1)
28-63: Schema setup now uses DBAL 3 types and schema manager correctly.The switch to
Table,Types::*constants for column definitions,Type::getType(Types::...)insideplatformOptions, and$this->_conn->createSchemaManager()is consistent with Doctrine DBAL 3 and keeps the previous semantics of the test table.test/Crate/Test/DBAL/Platforms/CratePlatformTest.php (3)
32-41: Platform test now targets Doctrine DBAL 3 test base and exception type.Using
Doctrine\DBAL\ExceptionasDBALExceptionand extendingAbstractPlatformTestCase, plus importingTypes, brings this test in line with the DBAL 3 testing infrastructure and type system.
374-384: Alter-table tests updated to use DBAL 3 type constants.Using
Type::getType('integer')(andTypes::*elsewhere) when constructingColumninstances forTableDiffkeeps these tests compatible with DBAL 3’s type system while preserving the original expectations for generatedALTER TABLESQL and event dispatching.Also applies to: 386-410
460-467: Object/array SQL declaration tests are consistent with new Types usage.Switching nested field types and array platform options to
Types::INTEGER/Types::STRINGwhile still relying onArrayType::NAMEandMapType::NAMEpreserves the semantics of the OBJECT/ARRAY SQL declaration tests and matches the broader migration away from deprecatedType::constants.Also applies to: 474-481
src/Crate/DBAL/Platforms/CratePlatform.php (2)
43-45: LGTM: Constants made public for external access.Making these constants public allows external code to reference timestamp and table clause formats, which aligns with Doctrine 3's expectations for platform configuration.
858-872: LGTM: Doctrine 3 API additions.Both
getTableOptionsSQLandgetCurrentDatabaseExpressionare properly implemented to support Doctrine 3's enhanced platform API.test/Crate/Test/DBAL/Functional/DataAccessTest.php (1)
27-33: LGTM: Imports updated for Doctrine 3 compatibility.The import changes correctly align with Doctrine 3's API, including
Connection,Table, andTypesconstants.src/Crate/DBAL/Driver/PDOCrate/CrateStatement.php (1)
48-52: LGTM: Constructor correctly forwards options.The updated constructor now accepts and forwards the
$optionsparameter to the underlying PDOprepare()call, resolving theArgumentCountErrorand preserving driver-specific options likebulkMode.src/Crate/DBAL/Driver/PDOCrate/PDOConnection.php (2)
35-61: LGTM: Composition-based design properly implemented.The shift from inheritance to composition (holding an internal
PDOCrateDBinstance) aligns with Doctrine 3's driver architecture. ThegetNativeConnection()accessor andgetServerVersion()stub are correctly implemented.
71-103: LGTM: Consistent exception wrapping across connection methods.All methods (
prepare(),exec(),query()) consistently wrapPDOExceptionviaException::new(), ensuring uniform driver-level exception handling throughout the connection lifecycle.src/Crate/DBAL/Driver/PDOCrate/Driver.php (2)
47-55: LGTM: Driver::connect() signature updated for Doctrine 3.The method now correctly accepts only
array $params(with#[SensitiveParameter]) and extractsuser,password, anddriver_optionsfrom the array, matching Doctrine DBAL 3's driver interface requirements.
125-129: LGTM: Exception converter properly added.The
getExceptionConverter()method is a required addition for Doctrine 3, enabling proper mapping of CrateDB-specific error codes to Doctrine exception types.
About
Doctrine DBAL 3.0 was released on November 17, 2020.
Details
-- https://www.doctrine-project.org/2020/11/17/dbal-3.0.0.html