-
Notifications
You must be signed in to change notification settings - Fork 2
feat: support parsing scheme-only DSNs #8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughThe changes modify the DSN class to support scheme-only DSN strings (e.g., "local://", "memory://"). The host property and its getter method are made nullable. The constructor is updated to short-circuit parsing for scheme-only DSNs, initializing all fields appropriately. Additionally, a new test method is added to verify DSN parsing behavior for scheme-only cases, and an existing test input is updated to use a different invalid DSN string. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/DSN/DSNTest.php (1)
151-179: Scheme-only DSN tests align well with the new behaviorAssertions for scheme, null user/password/host/port, empty path, and null query for
local://,memory://, andfile://match the constructor’s scheme-only initialization and give good coverage. If you ever add more such schemes, a data provider could de-duplicate this, but it’s not required.src/DSN/DSN.php (1)
58-69: Scheme-only DSN handling works but could be more version-robustThe early-return branch cleanly initializes all fields for scheme-only DSNs and matches the new tests. Two things to verify/consider:
- PHP version constraint –
\str_ends_with()is PHP 8.0+, so this effectively requires PHP ≥ 8 unless you already enforce that elsewhere or provide a polyfill.- Reliance on
parse_url()returningfalse– behavior for inputs like'local://'is quirky across PHP versions; tying scheme-only detection to!$partsmay be brittle.A more robust pattern would be to detect scheme-only DSNs before calling
parse_url(), e.g. via a simple regex, and bypassparse_url()entirely in that case:- $parts = \parse_url($dsn); - - // Handle scheme-only DSNs like "local://" which parse_url returns false for - if (!$parts && \str_ends_with($dsn, '://') && \strlen($dsn) > 3) { + // Handle scheme-only DSNs like "local://", "memory://", "file://" + if (\preg_match('~^[A-Za-z][A-Za-z0-9+\-.]*://$~', $dsn) === 1) { $this->scheme = \substr($dsn, 0, -3); $this->user = null; $this->password = null; $this->host = null; $this->port = null; $this->path = ''; $this->query = null; return; } + $parts = \parse_url($dsn); + if (!$parts) { throw new \InvalidArgumentException("Unable to parse DSN: $dsn"); }This avoids depending on the exact
parse_url()behavior and removes the need forstr_ends_with().Please verify against your supported PHP versions that
parse_url()behaves as assumed today, and decide whether you want to adopt this more explicit detection to future‑proof the behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/DSN/DSN.php(4 hunks)tests/DSN/DSNTest.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/DSN/DSNTest.php (1)
src/DSN/DSN.php (1)
DSN(5-177)
🔇 Additional comments (2)
tests/DSN/DSNTest.php (1)
181-185: Updated failing DSN keeps negative coverage consistent with new semanticsSwitching
testFail()to use'invalid-dsn-without-scheme'correctly targets the “missing scheme” path while allowing scheme-only DSNs likemariadb://to be valid under the new rules.src/DSN/DSN.php (1)
23-26: Host nullability is wired through correctly; confirm BC/semver expectationsMaking
$hostnullable and propagating that through assignment andgetHost(): ?stringis internally consistent and matches the scheme-only DSN behavior where there genuinely is no host. This does, however, change the public API contract ofgetHost()fromstringto?string, which can break strict-typed consumers expecting a non-nullable string.Please double‑check that:
- The package versioning (and changelog) explicitly call out this return-type change, or
- All known consumers can handle
nullfromgetHost().Also applies to: 82-82, 121-125
Summary
This PR adds support for parsing scheme-only DSNs like
local://,memory://,file://etc.Changes
hostproperty nullable to support DSNs without a hostgetHost()return type fromstringto?stringlocal://) which PHP'sparse_urlreturnsfalseforExample Usage
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.