-
Notifications
You must be signed in to change notification settings - Fork 51
Don't omit "/" if it has QUERY_STRING. #5
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
base: master
Are you sure you want to change the base?
Conversation
We can also omit "/" when <path> and <searchpart> are empty, but it makes t/old-base.t fail.
RFC 2396 does not agree on this AFAICT. |
RFC3986 says that RFC2396 was obsolete. So we should follow RFC3986(and RFC1738), shouldn't we ?? |
RFC3986:
This does allow "http://localhost?p=1". Does compatiblity with other stuff break with this, or do you want to argue the aesthetics of of this as canonical form? |
We still have this inconsistency:
It's better to be consistent, so one of these ought to change. |
5dcfddf
to
3a2965a
Compare
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.
Pull Request Overview
This PR fixes the canonical URL formatting to comply with RFC 1738, which requires the "/" path separator to be retained when a query string is present. The change ensures that URLs like "http://localhost?p=1" are canonicalized to "http://localhost/?p=1" instead of omitting the slash.
- Modifies the canonical method logic to always add "/" when path is empty, regardless of query presence
- Adds a test case to verify the fix works correctly for URLs with query strings
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
URI/http.pm | Simplified slash_path condition to always add "/" when path is empty and authority is defined |
t/http.t | Added test case for URL canonicalization with query string and updated test count |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
@@ -61,3 +61,8 @@ $u = URI->new("http://%77%77%77%2e%70%65%72%6c%2e%63%6f%6d/%70%75%62/%61/%32%30% | |||
print "not " unless $u->canonical eq "http://www.perl.com/pub/a/2001/08/27/bjornstad.html"; | |||
print "ok 15\n"; | |||
|
|||
# RFC 1738: "/" can't be ommited when <searchpart> is present. |
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.
There's a typo in the comment: 'ommited' should be 'omitted'.
# RFC 1738: "/" can't be ommited when <searchpart> is present. | |
# RFC 1738: "/" can't be omitted when <searchpart> is present. |
Copilot uses AI. Check for mistakes.
RFC 1738 says, `If neither <path> nor <searchpart> is present, the "/" may also be omitted.'.
http://www.rfc-editor.org/rfc/rfc1738.txt
So we can't omit "/" if the URL has '?' and QUERY_STRINGs.
I thought that the following patch is better than this pull-req, but it makes t/old-base.t broken.