Skip to content
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

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

Merged
merged 1 commit into from
Mar 21, 2024

Conversation

adamziel
Copy link
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
Copy link
Member

@brandonpayton brandonpayton left a comment

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
5 checks passed
@brandonpayton brandonpayton deleted the remove-hash-from-url branch March 21, 2024 16:25
@brandonpayton
Copy link
Member

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

Copy link
Member

@dmsnell dmsnell left a comment

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.

@@ -119,7 +119,8 @@ export class PHPRequestHandler implements RequestHandler {
request.url.startsWith('http://') ||
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
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
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
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to Access Pages Due to Anchor URL Structure in React Based Plugin
3 participants