Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Add support for including the query string#2

Closed
briandela wants to merge 1 commit into
bendrucker:masterbendrucker/hapi-require-https:masterfrom
briandela:masterbriandela/hapi-require-https:masterCopy head branch name to clipboard
Closed

Add support for including the query string#2
briandela wants to merge 1 commit into
bendrucker:masterbendrucker/hapi-require-https:masterfrom
briandela:masterbriandela/hapi-require-https:masterCopy head branch name to clipboard

Conversation

@briandela

Copy link
Copy Markdown
Contributor

Adds an option which allows for having the redirect include the query string.

@bendrucker

Copy link
Copy Markdown
Owner

Is there a compelling reason why it would ever be omitted? Not including it was pure oversight on my part.

@briandela

Copy link
Copy Markdown
Contributor Author

As it is today it's not included. request.path doesn't include the query string, request.url.path does. I figured to not break the current behaviour, I'd make it optional to include the path.

Essentially, I didn't want to make the new default behaviour a breaking change.

Comment thread README.md Outdated

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Didn't know if/how you wanted this filled in so I left it blank for now. Can fill it in with details as needed.

@briandela

Copy link
Copy Markdown
Contributor Author

I could just change the default to include it - it's a lot smaller change :-) Only one line.

@bendrucker

Copy link
Copy Markdown
Owner

I'd actually prefer to brainstorm any other breaking changes and roll them out in a 2.0 that gets things right. An option to disable qs inclusion is a non-breaking change after that but I'd rather not include an option if there's not a good reason.

@briandela

Copy link
Copy Markdown
Contributor Author

I did just update this PR to be the 1 liner fix before I saw your response.

bendrucker pushed a commit that referenced this pull request Nov 13, 2014
@bendrucker

Copy link
Copy Markdown
Owner

Awesome! Released as 1.0.1 since not passing the query string was a bug.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.