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

Fix several request.url property issues on setUrl() #3441

Merged
merged 3 commits into from May 30, 2017

Conversation

@kanongil
Copy link
Contributor

@kanongil kanongil commented Feb 21, 2017

This PR fixes this following issues in the request.setUrl() method:

  • The passed url object is not cloned.
  • Trailing slash stripping should be done after normalization (important if hapijs/call#29 is implemented).
  • Always update pathname derived url properties when path is modified.

This should make for a safer and more consistent request.setUrl() experience.

@kanongil
Copy link
Contributor Author

@kanongil kanongil commented Mar 1, 2017

I added another commit to fix the issues in #3430.

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented May 29, 2017

Why are you preserving the port number when the new host doesn't have one?

@kanongil
Copy link
Contributor Author

@kanongil kanongil commented May 29, 2017

I'm not sure. It must have seemed sensible at the time, but using the default port for the scheme also makes sense. This way it is more similar to a redirect.

kanongil added 2 commits May 29, 2017
 * The passed url object is not cloned.
 * Trailing slash stripping should be done after normalization (important if hapijs/call#29 is implemented).
 * Always update pathname derived url properties when path is modified.
@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented May 29, 2017

If you are going to provide a new full URI, it should override everything including the port.

@kanongil kanongil force-pushed the revise-normalize branch from 221c158 to 081f365 May 29, 2017
@kanongil
Copy link
Contributor Author

@kanongil kanongil commented May 29, 2017

Ok, I have rebased and revised the info update.

@hueniverse hueniverse added this to the 16.3.0 milestone May 30, 2017
@hueniverse hueniverse merged commit ba95396 into hapijs:master May 30, 2017
2 checks passed
@lock
Copy link

@lock lock bot commented Jan 9, 2020

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants