Skip to content

Request handler: Remove everything after # from the URL - #1126

Merged
brandonpayton merged 1 commit into
trunkfrom
remove-hash-from-url
Mar 21, 2024
Merged

Request handler: Remove everything after # from the URL#1126
brandonpayton merged 1 commit into
trunkfrom
remove-hash-from-url

Conversation

@adamziel

Copy link
Copy Markdown
Collaborator

Closes #1120

Fixes a bug in the URL parsing where #foo is passed to PHP when requesting /index.php#foo.

Testing instructions

Confirm the tests passed

Closes #1120

Fixes a bug in the URL parsing where `#foo` is passed to PHP when requesting `/index.php#foo`.

 ## Testing instructions

Confirm the tests passed
@adamziel adamziel added [Type] Bug An existing feature does not function as intended [Aspect] Browser labels Mar 21, 2024
@adamziel
adamziel requested a review from a team March 21, 2024 13:15

@brandonpayton brandonpayton left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The change makes sense, and the tests pass. And the new test fails without the fix to php-request-handler.ts.

@brandonpayton
brandonpayton merged commit fefd010 into trunk Mar 21, 2024
@brandonpayton
brandonpayton deleted the remove-hash-from-url branch March 21, 2024 16:25
@brandonpayton

Copy link
Copy Markdown
Member

I went ahead and merged this since it is a simple change.

@dmsnell dmsnell left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Very nice change; in hindsight I realize we all should have noticed this.

request.url.startsWith('https://');
const requestedUrl = new URL(
request.url,
// Remove the hash part of the URL as it's meant for the server.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There's something wrong with this comment. It should either say "it's not meant for the server" or "it's meant only for the client."

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Good spot!

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in 06f27eb

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Aspect] Browser [Type] Bug An existing feature does not function as intended

3 participants