Feat: Improve domain validator #174
Conversation
WalkthroughDomain validator now uses FILTER_VALIDATE_DOMAIN with FILTER_FLAG_HOSTNAME and explicitly rejects values ending with a dot or hyphen. Tests were expanded with many negative cases for malformed domains. Composer dev constraints for two tools were loosened. No public signatures changed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Domain as Domain::isValid
participant PHPFilter as filter_var
Caller->>Domain: isValid(value)
alt Guard: empty or non-string
Domain-->>Caller: false
else Guard: trailing dot or hyphen
Note right of Domain: New checks: str_ends_with(value, ".") or str_ends_with(value, "-")
Domain-->>Caller: false
else Validate hostname
Domain->>PHPFilter: filter_var(value, FILTER_VALIDATE_DOMAIN, FILTER_FLAG_HOSTNAME)
PHPFilter-->>Domain: (bool)
alt Valid
Domain-->>Caller: true
else Invalid
Domain-->>Caller: false
end
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
tests/Validator/DomainTest.php (1)
16-47: Update expectations for underscore-containing hostnames; this causes the PHPUnit failureWith FILTER_VALIDATE_DOMAIN + FILTER_FLAG_HOSTNAME, host labels must be alphanumeric plus hyphens; underscores are invalid. The current positive assertions for domains with underscores now fail (see pipeline error pointing at Line 22). Adjust those expectations (and consider removing duplicated 'example.org' as a minor cleanup).
Apply this diff to align tests with the new validator semantics and remove a duplicate:
$this->assertEquals(true, $this->domain->isValid('subdomain.example-app.com')); - $this->assertEquals(true, $this->domain->isValid('subdomain.example_app.com')); + $this->assertEquals(false, $this->domain->isValid('subdomain.example_app.com')); $this->assertEquals(true, $this->domain->isValid('subdomain-new.example.com')); - $this->assertEquals(true, $this->domain->isValid('subdomain_new.example.com')); + $this->assertEquals(false, $this->domain->isValid('subdomain_new.example.com')); $this->assertEquals(true, $this->domain->isValid('localhost')); $this->assertEquals(true, $this->domain->isValid('example.io')); $this->assertEquals(true, $this->domain->isValid('example.org')); - $this->assertEquals(true, $this->domain->isValid('example.org'));Notes:
- Keeping 'localhost' as valid is consistent with FILTER_FLAG_HOSTNAME behavior in recent PHP versions. If your minimum PHP version treats it differently, we can revisit.
🧹 Nitpick comments (1)
tests/Validator/DomainTest.php (1)
30-40: Great negative coverage; remove duplicate assertion and keep cases concise
- Duplicate assertion for the same input appears twice ('.api.appwrite.io'). Remove one to avoid redundancy.
Apply this diff to remove the duplicate on Line 32:
- $this->assertEquals(false, $this->domain->isValid('.api.appwrite.io'));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/Http/Validator/Domain.php(1 hunks)tests/Validator/DomainTest.php(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/Validator/DomainTest.php (1)
src/Http/Validator/Domain.php (1)
isValid(38-57)
🪛 GitHub Actions: Tests
tests/Validator/DomainTest.php
[error] 22-22: PHPUnit failure: DomainTest::testIsValid - Failed asserting that false matches expected true. Step: docker compose exec fpm vendor/bin/phpunit --configuration phpunit.xml
🪛 GitHub Actions: Linter
src/Http/Validator/Domain.php
[error] 1-1: Pint lint failed (vendor/bin/pint --test). PSR-12 violation detected: 'single_space_around_construct' in src/Http/Validator/Domain.php.
🔇 Additional comments (1)
src/Http/Validator/Domain.php (1)
48-50: Switch to FILTER_VALIDATE_DOMAIN with FILTER_FLAG_HOSTNAME looks goodThis tightens validation to hostname semantics, aligning with RFC-compliant host labels (disallowing underscores, etc.). This change is consistent with the new negative tests you added.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
composer.json (1)
39-41: Looser dev constraints are fine; consider keeping a known floor to avoid older 1.x installsComposer will typically resolve to the latest 1.x, but "1.*" also permits early 1.0 releases when a lock exists or mirrors lag, which may lack features you rely on. If the intent is to broaden to 1.x while preserving the previous minimums, consider adding an explicit lower bound.
Apply this diff to keep the broadened range but retain prior minimums:
- "phpbench/phpbench": "^1.2", - "laravel/pint": "1.*", - "phpstan/phpstan": "1.*" + "phpbench/phpbench": "^1.2", + "laravel/pint": ">=1.2 <2.0", + "phpstan/phpstan": ">=1.10 <2.0"tests/Validator/DomainTest.php (2)
30-40: Strong negative coverage; remove the duplicate leading-dot case or replace itGreat set of malformed examples. Lines 31 and 32 duplicate the same assertion for '.api.appwrite.io'. Remove the duplicate or replace it with another common invalid form to increase coverage (e.g., a scheme-prefixed string).
Replace the duplicate with a scheme-prefixed case:
- $this->assertEquals(false, $this->domain->isValid('.api.appwrite.io')); + $this->assertEquals(false, $this->domain->isValid('http://appwrite.io'));
22-22: Optional: prefer assertTrue/assertFalse and/or a data provider for readabilityUsing assertFalse/assertTrue improves readability and failure messages. With many cases, a data provider or iterating over arrays of inputs would make this test easier to maintain. Not blocking.
Also applies to: 24-24, 30-40
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
composer.json(1 hunks)src/Http/Validator/Domain.php(1 hunks)tests/Validator/DomainTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Http/Validator/Domain.php
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/Validator/DomainTest.php (1)
src/Http/Validator/Domain.php (1)
isValid(38-57)
🔇 Additional comments (1)
tests/Validator/DomainTest.php (1)
22-22: Good catch: underscore-containing labels marked invalidUnderscores are not permitted in hostnames per the LDH rule; aligning the tests with the stricter validator is correct.
Also applies to: 24-24
Summary by CodeRabbit
Bug Fixes
Tests
Chores