Skip to content

Conversation

@amotl
Copy link
Member

@amotl amotl commented Nov 20, 2025

About

getServerVersion() was a dummy before, now it's real.

Detail

However, one is still advised to select the platform manually. I didn't find any connection between getServerVersion() and the platform selection process -- there isn't any.

It is highly encouraged to use the platform class that matches your database vendor and version best.
-- https://www.doctrine-project.org/projects/doctrine-dbal/en/3.10/reference/platforms.html

References

@coderabbitai
Copy link

coderabbitai bot commented Nov 20, 2025

Warning

Rate limit exceeded

@amotl has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 22 minutes and 50 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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.

📥 Commits

Reviewing files that changed from the base of the PR and between 04a9e9d and 93c12f7.

📒 Files selected for processing (4)
  • examples/objects.php (2 hunks)
  • src/Crate/DBAL/Driver/PDOCrate/Driver.php (1 hunks)
  • src/Crate/DBAL/Driver/PDOCrate/PDOConnection.php (1 hunks)
  • test/Crate/Test/DBAL/Functional/ConnectionTest.php (1 hunks)

Walkthrough

Replaces manual DSN/platform setup with a DSN-driven DriverManager connection in the example, updates the driver to implement VersionAwarePlatformDriver, changes PDOConnection::getServerVersion to delegate to the underlying connection, and adds two tests asserting server version retrieval.

Changes

Cohort / File(s) Summary
Driver Interface
src/Crate/DBAL/Driver/PDOCrate/Driver.php
Driver now implements VersionAwarePlatformDriver (imported) instead of the legacy DBAL driver interface.
Connection implementation
src/Crate/DBAL/Driver/PDOCrate/PDOConnection.php
getServerVersion() now delegates to the wrapped/native connection ($this->connection->getServerVersion()) and wraps PDOExceptions.
Examples: connection flow
examples/objects.php
Switched to DSN-driven flow: uses DsnParser to build options, sets options['platform'] = new CratePlatform4(), and obtains a connection via DriverManager::getConnection($options); schema provisioning remains but is performed after connection.
Tests: server-version assertions
test/Crate/Test/DBAL/Functional/ConnectionTest.php
Added testGetServerVersionNativeConnection() and testGetServerVersionWrappedConnection() asserting the reported server version is non-empty/valid (>= 0.0.0).

Sequence Diagram(s)

sequenceDiagram
    participant Example as examples/objects.php
    participant DsnParser as DsnParser
    participant DriverMgr as DriverManager
    participant Connection as DBAL Connection
    participant PDOConn as PDOCrate\PDOConnection
    participant Native as Native CrateDB Conn
    participant SchemaMgr as SchemaManager

    Note over Example,DsnParser: New DSN-driven connection flow
    Example->>DsnParser: parse(crate://...)
    DsnParser-->>Example: options
    Example->>DriverMgr: getConnection(options)
    DriverMgr-->>Connection: returns DBAL Connection (with CratePlatform4)
    Connection->>PDOConn: wraps native connection
    PDOConn->>Native: getServerVersion() (delegated)
    Connection->>SchemaMgr: create SchemaManager(connection)
    SchemaMgr->>SchemaMgr: drop/create schema
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Check Driver implements all required methods for VersionAwarePlatformDriver.
  • Verify delegation in PDOConnection::getServerVersion() handles exception and return formats safely.
  • Validate example DSN parsing produces correct connection options and platform injection.
  • Ensure new tests' environment assumptions (reachable CrateDB) are documented or guarded.

Possibly related issues

Poem

🐰
I parsed a crate-URL, hopped into place,
The driver put on a version-aware face.
I asked the server, it answered clear,
I twitched my nose and gave a cheer! 🥕

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: implementing the getServerVersion() method to forward to PDOCrateDB in a DBAL3 context.
Description check ✅ Passed The description clearly explains that getServerVersion() was previously a dummy and is now implemented, and provides context about platform selection guidance.

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.

coderabbitai[bot]

This comment was marked as resolved.

coderabbitai[bot]

This comment was marked as resolved.

@amotl amotl requested review from seut and surister November 21, 2025 10:51
@amotl amotl requested a review from seut November 21, 2025 13:08
coderabbitai[bot]

This comment was marked as resolved.

// Retrieve server version.
$serverVersion = $this->_conn->getNativeConnection()->getServerVersion();
$this->assertTrue(
version_compare($serverVersion, '0.0.0', '>='),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When could it be 0.0.0? Like why not asserting that it must be > '0.0.0'

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, thanks. CodeRabbit's suggestion also isn't too bad, so I've just used it right away, see 3d0a784.

@amotl amotl requested a review from seut November 21, 2025 13:26
Copy link
Member

@seut seut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thx!

@amotl amotl merged commit cf37370 into amo/doctrine3 Nov 21, 2025
11 checks passed
@amotl amotl deleted the server-version branch November 21, 2025 17:36
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.

2 participants