Skip to content

Conversation

@amotl
Copy link
Member

@amotl amotl commented Nov 16, 2025

About

A few more updates by @JulianMar from his work on DBAL4 support (GH-136). Thank you so much.

Details

This patch is stacked upon GH-122. It was conceived after finishing the main patch and kept separate from it to reduce noise by that very delta.

@coderabbitai
Copy link

coderabbitai bot commented Nov 16, 2025

Walkthrough

This PR modernizes the Crate DBAL driver for Doctrine DBAL 3 by adding return type declarations throughout the codebase, updating exception handling from DBALException to Exception, deprecating bindParam in favor of bindValue, and refactoring test infrastructure with a new base class and updated fetch patterns.

Changes

Cohort / File(s) Summary
Build & Documentation
.php-cs-fixer.php, CHANGES.txt
Added exclude directives for build and vendor paths; updated changelog to specify "Doctrine DBAL 3" instead of generic "Doctrine 3".
Core Driver & Connection
src/Crate/DBAL/Driver/PDOCrate/CrateStatement.php, src/Crate/DBAL/Driver/PDOCrate/PDOConnection.php, src/Crate/DBAL/Driver/PDOCrate/Result.php
Added return type hints; changed execute() to wrap PDOException and rethrow as Exception; deprecated bindParam() with guidance to bindValue(); updated transaction methods (beginTransaction, commit, rollBack) to return void; getServerVersion() now returns string.
Platform Classes
src/Crate/DBAL/Platforms/CratePlatform.php, src/Crate/DBAL/Platforms/CratePlatform4.php, src/Crate/DBAL/Platforms/Keywords/CrateKeywords.php
Comprehensive return type annotations (string, bool, array, mixed, void) added across SQL declaration and utility methods; replaced DBALException with Exception; no behavioral changes to method logic.
Type Classes
src/Crate/DBAL/Types/ArrayType.php, src/Crate/DBAL/Types/MapType.php, src/Crate/DBAL/Types/TimestampType.php
Added return type hints to convert methods (mixed) and SQL declaration methods (string); updated exception references from DBALException to Exception in docblocks.
Schema Management
src/Crate/DBAL/Schema/CrateSchemaManager.php
Added return type hints to portable table methods (array, Column); made flatten() static; no call-site changes.
Test Base Class
test/Crate/Test/DBAL/DBALFunctionalTest.php
Renamed from DBALFunctionalTestCase; replaced execute() with run_sql() returning Result; added return type hints to prepareStatement() (Statement) and refresh() (void).
Functional Tests
test/Crate/Test/DBAL/Functional/BindingTest.php, test/Crate/Test/DBAL/Functional/DataAccessTest.php, test/Crate/Test/DBAL/Functional/ModifyLimitQueryTest.php, test/Crate/Test/DBAL/Functional/NamedParametersTest.php, test/Crate/Test/DBAL/Functional/TypeConversionTest.php, test/Crate/Test/DBAL/Functional/WriteTest.php, test/Crate/Test/DBAL/Functional/ConnectionTest.php, test/Crate/Test/DBAL/Functional/Schema/SchemaManagerTest.php, test/Crate/Test/DBAL/Functional/TableOptionsTest.php, test/Crate/Test/DBAL/Functional/Types/MapTypeTest.php
All test classes renamed (TestCase → Test suffix convention); updated to extend DBALFunctionalTest; migrated from bindParam() + execute() to bindValue() + executeQuery(); replaced fetch(PDO::FETCH_*) patterns with high-level DBAL methods (fetchAssociative, fetchAllNumeric, fetchFirstColumn, etc.); updated execute/executeUpdate calls to executeQuery/executeStatement.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    participant Test
    participant CrateStatement
    participant PDO
    
    Note over Test,PDO: Old Flow (bindParam + execute)
    Test->>CrateStatement: bindParam($param, $var, ParameterType)
    CrateStatement->>PDO: bindParam(...)
    Test->>CrateStatement: execute()
    CrateStatement->>PDO: execute()
    
    Note over Test,PDO: New Flow (bindValue + executeQuery)
    Test->>CrateStatement: bindValue($param, $value, ParameterType)
    CrateStatement->>PDO: bindValue(...)
    Test->>CrateStatement: executeQuery()
    CrateStatement->>PDO: execute()
    rect rgb(200, 220, 255)
        CrateStatement->>CrateStatement: try/catch PDOException
    end
    CrateStatement-->>Test: Exception (on error)
Loading
sequenceDiagram
    autonumber
    participant Test
    participant DBALFunctionalTest
    participant Connection
    
    Note over Test,Connection: Old Flow
    Test->>DBALFunctionalTest: execute($sql)
    DBALFunctionalTest->>Connection: query($sql)
    
    Note over Test,Connection: New Flow
    Test->>DBALFunctionalTest: run_sql($sql): Result
    DBALFunctionalTest->>Connection: executeQuery($sql)
    Connection-->>DBALFunctionalTest: Result
    DBALFunctionalTest-->>Test: Result
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

  • CrateStatement exception handling & deprecation: Verify try/catch logic in execute() correctly wraps PDOException and rethrows; confirm bindParam deprecation messaging is clear and complete.
  • Platform class consistency: Review return type annotations across CratePlatform and CratePlatform4 to ensure alignment with parent/interface contracts; validate all DBALException→Exception replacements are appropriate.
  • Test migration patterns: Spot-check several test files to confirm bindParam→bindValue and fetch pattern migrations are consistent and correct (executeQuery usage, fetch method equivalence).
  • Exception type changes: Verify DBALException removal doesn't break exception catching in caller code; check docblock @throws references are updated throughout.
  • Type compatibility: Ensure return type hints (especially mixed, array, void) align with actual return values and don't introduce new type incompatibilities.

Poem

🐰 Hop along with types so clear,
mixed and array now appear!
bindParam fades, bindValue stays,
void returns light the ways.
DBAL 3 springs to life complete! 🌱

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Title check ❓ Inconclusive The title 'DBAL3: More updates' is vague and generic, using non-descriptive terms that don't convey meaningful information about the specific changes in the changeset. Consider using a more specific title that describes the main changes, such as 'Add return type declarations and upgrade to Doctrine DBAL 3' or similar to better convey the primary purpose.
✅ Passed checks (2 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description check ✅ Passed The PR description references DBAL4 support work and is directly related to the comprehensive changes across multiple files including driver implementations, type declarations, and test updates.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch dbal3-more

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@amotl amotl marked this pull request as ready for review November 16, 2025 16:36
coderabbitai[bot]

This comment was marked as resolved.

- Remove redundant test function `testPrepareWithBindParam`
- Remove deprecated test function about `PDO::FETCH_CLASS`
- Complete function signature type hinting
- Improve MapType::convertToPHPValue about accounting for `null` values
@crate crate deleted a comment from coderabbitai bot Nov 18, 2025
@crate crate deleted a comment from coderabbitai bot Nov 18, 2025
@amotl amotl merged commit e15924c into amo/doctrine3 Nov 19, 2025
16 of 17 checks passed
@amotl amotl deleted the dbal3-more branch November 19, 2025 18:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants