Fix: issue with matching null path and relevant test #173
Conversation
WalkthroughThe Changes
Poem
✨ Finishing Touches
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Http/Http.php(1 hunks)tests/HttpTest.php(1 hunks)
🔇 Additional comments (4)
src/Http/Http.php (1)
387-389: LGTM! Well-implemented defensive programming.This change correctly handles edge cases where
parse_urlreturnsnull(when the path component is missing) orfalse(when the URL is severely malformed). Defaulting to the root path/is a sensible fallback that maintains consistent routing behavior.The implementation is clean and the comment clearly explains the intent.
tests/HttpTest.php (3)
611-622: LGTM! Good test coverage for null path handling.This test correctly verifies that when the URL has only a query string (no path component),
parse_urlreturnsnullfor the path component, which should trigger the fallback to root path/.
624-635: LGTM! Covers the no-path-component scenario.This test handles the case where a full URL is provided but has no path component, which should also trigger the fallback behavior.
650-661: LGTM! Comprehensive test for query-string-only URLs.This test correctly covers the scenario where the URI contains only a query string, which should result in
parse_urlreturningnullfor the path component.
| public function testMatchWithMalformedURL(): void | ||
| { | ||
| // Create a route for root path | ||
| $expected = Http::get('/'); | ||
|
|
||
| // Test case where parse_url returns false (severely malformed URL) | ||
| $_SERVER['REQUEST_METHOD'] = 'GET'; | ||
| $_SERVER['REQUEST_URI'] = '#fragment'; // Malformed scheme | ||
|
|
||
| $matched = $this->http->match(new Request()); | ||
| $this->assertEquals($expected, $matched); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify that this URI actually causes parse_url to return false.
The test intent is good, but the URI '#fragment' might not actually cause parse_url to return false. It would more likely return null for the path component since fragments are valid URL components.
Let me verify the behavior of parse_url with this URI:
🏁 Script executed:
#!/bin/bash
# Description: Test parse_url behavior with the fragment-only URI to verify it returns false vs null
php -r "
echo 'Testing parse_url behavior with fragment-only URI:' . PHP_EOL;
echo 'URI: #fragment' . PHP_EOL;
\$result = parse_url('#fragment', PHP_URL_PATH);
echo 'parse_url result: ';
var_dump(\$result);
echo 'Result === null: ' . ((\$result === null) ? 'true' : 'false') . PHP_EOL;
echo 'Result === false: ' . ((\$result === false) ? 'true' : 'false') . PHP_EOL;
echo PHP_EOL;
echo 'Testing with a truly malformed URL:' . PHP_EOL;
\$malformed = 'http://[invalid-ipv6';
echo 'URI: ' . \$malformed . PHP_EOL;
\$result2 = parse_url(\$malformed, PHP_URL_PATH);
echo 'parse_url result: ';
var_dump(\$result2);
echo 'Result === null: ' . ((\$result2 === null) ? 'true' : 'false') . PHP_EOL;
echo 'Result === false: ' . ((\$result2 === false) ? 'true' : 'false') . PHP_EOL;
"Length of output: 1151
Use a URI that actually makes parse_url() return false
PHP’s parse_url('#fragment') does not return false (it returns null for the path component), so this test never hits the “false” branch. To cover a truly malformed URI, update the test in tests/HttpTest.php (lines 637–648) to something like:
$_SERVER['REQUEST_METHOD'] = 'GET';
$_SERVER['REQUEST_URI'] = 'http://[invalid-ipv6';
$matched = $this->http->match(new Request());
$this->assertEquals(Http::get('/'), $matched);Alternatively, if you want to keep '#fragment', adjust the assertion to reflect that parse_url() returns null/empty path rather than false.
🤖 Prompt for AI Agents
In tests/HttpTest.php around lines 637 to 648, the test uses '#fragment' as the
REQUEST_URI, but parse_url('#fragment') does not return false, so the false
branch is never tested. To fix this, replace the REQUEST_URI value with a truly
malformed URI like 'http://[invalid-ipv6' that causes parse_url() to return
false, ensuring the test covers the intended case.
Related: #171
Summary by CodeRabbit
Bug Fixes
Tests