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
WalkthroughUpgrades the Crate DBAL driver to Doctrine DBAL 3: replaces inheritance with composition in driver/statement/connection layers, adds DBAL3 interfaces (VersionAwarePlatformDriver, ExceptionConverter), introduces Result wrapper, adds return types and public constants, updates types/platform APIs, tests, docs, and tooling for DBAL 3 compatibility. (≤50 words) Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Conn as PDOConnection
participant PDO as PDOCrateDB
participant Stmt as CrateStatement
participant Res as Result
App->>Conn: prepare(sql, options)
Conn->>PDO: prepare(sql, options)
PDO-->>Conn: PDOStatement
Conn->>Stmt: new CrateStatement(PDOInterface, sql, options)
Conn-->>App: CrateStatement
App->>Stmt: execute(params?)
Note right of Stmt: deprecation notice if params provided
Stmt->>PDO: $stmt->execute()
Stmt->>Res: new Result(Stmt)
Res-->>App: ResultInterface
App->>Res: fetchAssociative()
Res->>Stmt: fetch(FETCH_ASSOC)
Stmt->>PDO: fetch(FETCH_ASSOC)
PDO-->>Res: row|false
Res-->>App: row|false
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
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.
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.
|
@seut: Thanks for your diligent review. I've integrated the secondary branch, applied updates according to your suggestions, went into a few conversations with CodeRabbit, and also tried to improve accordingly. I've not resolved corresponding conversation threads for your information and acknowledgement. Please resolve and collapse them as you go at your disposal. |
|
Hi @seut. Thanks for more review procedures. Those patches aim to improve two more details we talked about here.
This ticket tracks the item about implementing Please resolve and collapse pending conversation threads after reading up. |
| public function getDatabasePlatform() | ||
| public function getDatabasePlatform(): AbstractPlatform | ||
| { | ||
| return new CratePlatform(); | ||
| return new CratePlatform4(); |
There was a problem hiding this comment.
This needs to pull in the logic from createDatabasePlatformForVersion.
There was a problem hiding this comment.
It did not quite work like I thought it would work, obviously because getDatabasePlatform() does not have connection options available, so it can't connect to CrateDB to inquire its version number. This resembles a traditional and solid chicken-and-egg problem.
As a consequence, I made this, to at least improve the canonical example program a bit more, in order to demonstrate proper(?) use of createDatabasePlatformForVersion().
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Crate/DBAL/Driver/PDOCrate/PDOConnection.php (1)
74-95: Consider tightening exec() error handling and simplifying query()Two small, non‑blocking improvements:
- exec(): rely on explicit error handling instead of
assert
assert($result !== false);is often disabled in production, so afalseresult could silently pass through and be coerced to0by theintreturn type. Since you already use exception mode, it’s safer and clearer to treatfalseas an error:public function exec($sql): int { - try { - $result = $this->connection->exec($sql); - assert($result !== false); - return $result; - } catch (PDOException $exception) { - throw Exception::new($exception); - } + try { + $result = $this->connection->exec($sql); + if ($result === false) { + // Defensive: should not happen with ERRMODE_EXCEPTION, but guard anyway. + throw new PDOException('Execution failed without throwing an exception.'); + } + + return $result; + } catch (PDOException $exception) { + throw Exception::new($exception); + } }
- query(): reuse
CrateStatement::execute()result and drop redundant catch
CrateStatement::execute()already convertsPDOExceptionto a Doctrine driver exception and returns aResultInterface. You can avoid creating a secondResultand the now‑redundantPDOExceptioncatch:- public function query(string $sql): ResultInterface - { - try { - $stmt = $this->prepare($sql); - $stmt->execute(); - return new Result($stmt); - } catch (PDOException $exception) { - throw Exception::new($exception); - } - } + public function query(string $sql): ResultInterface + { + $stmt = $this->prepare($sql); + + return $stmt->execute(); + }This keeps error behavior identical while shaving off one allocation and some unreachable exception handling.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
docs/index.rst(2 hunks)examples/objects.php(1 hunks)src/Crate/DBAL/Driver/PDOCrate/Driver.php(6 hunks)src/Crate/DBAL/Driver/PDOCrate/PDOConnection.php(3 hunks)test/Crate/Test/DBAL/Functional/ConnectionTest.php(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- docs/index.rst
🚧 Files skipped from review as they are similar to previous changes (1)
- examples/objects.php
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-19T23:30:55.289Z
Learnt from: amotl
Repo: crate/crate-dbal PR: 122
File: src/Crate/DBAL/Driver/PDOCrate/PDOConnection.php:48-48
Timestamp: 2025-11-19T23:30:55.289Z
Learning: In crate-dbal's PDOConnection.php, PDO::ATTR_STATEMENT_CLASS is set using a bare class string (PDOStatement::class) rather than the standard PDO array format ([ClassName::class, [args]]) because PDOCrateDB is a custom PDO implementation that accepts a class name string directly for this attribute.
Applied to files:
src/Crate/DBAL/Driver/PDOCrate/Driver.phptest/Crate/Test/DBAL/Functional/ConnectionTest.phpsrc/Crate/DBAL/Driver/PDOCrate/PDOConnection.php
🧬 Code graph analysis (3)
src/Crate/DBAL/Driver/PDOCrate/Driver.php (6)
src/Crate/DBAL/Platforms/CratePlatform.php (2)
CratePlatform(42-888)getName(476-479)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-152)src/Crate/DBAL/Driver/PDOCrate/ExceptionConverter.php (1)
ExceptionConverter(23-58)
test/Crate/Test/DBAL/Functional/ConnectionTest.php (3)
test/Crate/Test/DBAL/DBALFunctionalTest.php (1)
DBALFunctionalTest(32-124)src/Crate/DBAL/Driver/PDOCrate/PDOConnection.php (6)
getNativeConnection(61-64)prepare(74-81)beginTransaction(126-133)commit(135-142)rollBack(144-151)getServerVersion(52-59)src/Crate/DBAL/Driver/PDOCrate/CrateStatement.php (1)
getWrappedStatement(147-150)
src/Crate/DBAL/Driver/PDOCrate/PDOConnection.php (3)
src/Crate/DBAL/Driver/PDOCrate/Driver.php (1)
Driver(36-131)src/Crate/DBAL/Driver/PDOCrate/Result.php (1)
Result(12-108)src/Crate/DBAL/Driver/PDOCrate/CrateStatement.php (2)
CrateStatement(39-151)execute(57-74)
🔇 Additional comments (12)
test/Crate/Test/DBAL/Functional/ConnectionTest.php (7)
24-34: LGTM! Clean migration to DBAL 3 structure.The imports, class naming, and trait usage are well-aligned with Doctrine DBAL 3 patterns. The
VerifyDeprecationstrait is appropriate for tracking any deprecation warnings during the migration.
48-63: Well-structured authentication test.The explicit
connect()call followed by credential verification viagetNativeConnection()properly validates that HTTP Basic Auth credentials are correctly propagated to the native PDO connection.
76-92: Comprehensive statement wrapper validation.The three-layer assertion chain correctly verifies the DBAL 3 statement wrapping architecture, from the outer
Doctrine\DBAL\Statementthrough the driver-specific layers down to the native PDO statement.
105-114: LGTM! Transaction happy path properly tested.These tests correctly verify that
beginTransaction()andcommit()return expected boolean values when used in the correct sequence.
138-162: Excellent server version validation.Both tests comprehensively verify server version retrieval across both connection layers (native and wrapped). The semver regex pattern correctly allows version suffixes (pre-release/build metadata), which is appropriate for real-world version strings.
These tests properly validate the
getServerVersion()implementation referenced in the PR objectives (GH-154).
116-121: Verify exception message matches DBAL 3 behavior: CONFIRMED ✓The test correctly expects
ConnectionExceptionwith the message "There is no active transaction." which matches DBAL'sConnectionException::noActiveTransaction()method. The test structure and expectations align with Doctrine DBAL 3 behavior.
123-136: Verify rollback exception handling behavior.The tests correctly expect different exceptions based on transaction state:
- With active transaction:
Doctrine\DBAL\Driver\Exceptionwith "Unsupported functionality"- Without active transaction:
ConnectionExceptionwith "There is no active transaction."Please verify these exception types and messages match the actual implementation behavior when CrateDB's unsupported rollback operation is invoked.
src/Crate/DBAL/Driver/PDOCrate/PDOConnection.php (3)
37-50: Connection wiring and PDO attributes look correctThe encapsulation of
PDOCrateDBplus settingATTR_STATEMENT_CLASSandATTR_ERRMODEon the inner connection is consistent with the crate‑pdo standardization work and gives you predictable exception behavior from the outset. I don’t see issues here.
52-64: getServerVersion() / getNativeConnection() integration is soundDelegating
getServerVersion()andgetNativeConnection()to the innerPDOCrateDBwhile wrappingPDOExceptionkeeps the driver surface aligned with Doctrine’s expectations without exposing raw PDO errors. This also addresses the earlier concern about hard‑coded server versions.
108-151: Quote, lastInsertId, and transaction delegation are consistent
quote(),lastInsertId(), and the transaction methods now uniformly delegate toPDOCrateDBand wrapPDOExceptioninto Doctrine’s driver exception type, matching the rest of the class and Doctrine’s expectations. No further changes needed here from my side.src/Crate/DBAL/Driver/PDOCrate/Driver.php (2)
63-121: Platform selection and schema/exception plumbing look correct
getDatabasePlatform(): AbstractPlatformreturningCratePlatform4is a sensible default for modern CrateDB deployments.createDatabasePlatformForVersion()still encapsulates the version‑based selection betweenCratePlatform,CratePlatform1, andCratePlatform4, which Doctrine will use when server version information is available.getSchemaManager()andgetExceptionConverter()cleanly hook in the Crate‑specific schema manager and exception converter.I don’t see any behavioral regressions here; this wiring should work as intended with DBAL 3.
48-56: connect() and DSN construction align with DBAL 3.x/4.x driver expectationsThe
connect(array $params): PDOConnectionsignature correctly extracts credentials and driver options from the$paramsarray (keysuser,password,driver_options), which matches Doctrine DBAL's standard approach for custom drivers. The#[SensitiveParameter]annotation appropriately marks sensitive credentials, and the DSN construction passes all driver options through, keeping the implementation flexible.
|
^^ @seut: What do you think about the two most recent nitpick comments by CodeRabbit? They don't sound too bad, let's try? |
I do not agree on the 1st one as the result is an The 2nd one sounds very valid and I agree to apply the suggestion. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Crate/DBAL/Driver/PDOCrate/PDOConnection.php (1)
119-144: Transaction methods correctly delegate to internal connection.All three transaction methods properly delegate to
PDOCrateDBwith consistent exception wrapping. The implementation aligns with the documented approach: CrateDB's eventual consistency model is documented (docs/index.rst per learnings), and the methods perform real operations rather than being silent no-ops.Optional suggestion for consistency: Consider adding return type hints (likely
bool) to these three methods to match the style of all other methods in the class:public function beginTransaction(): bool public function commit(): bool public function rollBack(): boolBased on learnings, documentation was added to docs/index.rst explaining CrateDB's eventual consistency model and transaction behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Crate/DBAL/Driver/PDOCrate/PDOConnection.php(3 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: amotl
Repo: crate/crate-dbal PR: 122
File: src/Crate/DBAL/Driver/PDOCrate/PDOConnection.php:48-48
Timestamp: 2025-11-19T23:30:55.289Z
Learning: In crate-dbal's PDOConnection.php, PDO::ATTR_STATEMENT_CLASS is set using a bare class string (PDOStatement::class) rather than the standard PDO array format ([ClassName::class, [args]]) because PDOCrateDB is a custom PDO implementation that accepts a class name string directly for this attribute.
📚 Learning: 2025-11-19T23:30:55.289Z
Learnt from: amotl
Repo: crate/crate-dbal PR: 122
File: src/Crate/DBAL/Driver/PDOCrate/PDOConnection.php:48-48
Timestamp: 2025-11-19T23:30:55.289Z
Learning: In crate-dbal's PDOConnection.php, PDO::ATTR_STATEMENT_CLASS is set using a bare class string (PDOStatement::class) rather than the standard PDO array format ([ClassName::class, [args]]) because PDOCrateDB is a custom PDO implementation that accepts a class name string directly for this attribute.
Applied to files:
src/Crate/DBAL/Driver/PDOCrate/PDOConnection.php
📚 Learning: 2025-11-21T18:32:35.719Z
Learnt from: amotl
Repo: crate/crate-dbal PR: 122
File: src/Crate/DBAL/Driver/PDOCrate/PDOConnection.php:0-0
Timestamp: 2025-11-21T18:32:35.719Z
Learning: Documentation update: docs/index.rst now explicitly states CrateDB implements eventual consistency and does not support traditional ACID transactions (commit 171116851). Preference in this repo is to document this limitation rather than raise runtime exceptions in transaction methods.
Applied to files:
src/Crate/DBAL/Driver/PDOCrate/PDOConnection.php
🧬 Code graph analysis (1)
src/Crate/DBAL/Driver/PDOCrate/PDOConnection.php (3)
src/Crate/DBAL/Driver/PDOCrate/Driver.php (1)
Driver(36-131)src/Crate/DBAL/Driver/PDOCrate/Result.php (1)
Result(12-108)src/Crate/DBAL/Driver/PDOCrate/CrateStatement.php (2)
CrateStatement(39-151)execute(57-74)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build docs on ubuntu-latest
🔇 Additional comments (3)
src/Crate/DBAL/Driver/PDOCrate/PDOConnection.php (3)
45-50: LGTM! Constructor properly configured for DBAL 3 integration.The composition pattern is well-implemented: internal
PDOCrateDBinstance is created and configured with correct PDO attributes. TheATTR_STATEMENT_CLASSarray format[PDOStatement::class, []]is now supported after crate-pdo PR #192 was merged, resolving the earlier standardization discussion.Based on learnings, the ATTR_STATEMENT_CLASS format was standardized in crate-pdo PR #192.
52-99: Excellent delegation and error handling implementation.All core connection methods properly delegate to the internal
PDOCrateDBinstance with consistent exception wrapping:
getServerVersion(),prepare(),exec()wrapPDOExceptioninto DBAL exceptionsgetNativeConnection()provides clean access to underlying connectionquery()uses the standard prepare-then-execute patternThe
prepare()method correctly passes$optionstoCrateStatementconstructor, resolving the earlier parameter mismatch issue.
101-117: LGTM!quote()andlastInsertId()properly implemented.Both methods follow the consistent exception-handling pattern established throughout the class, with proper delegation to the internal connection and return type hints.
seut
left a comment
seut
left a comment
There was a problem hiding this comment.
Thanks for all the iterations!
Looks good to me now.
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