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

Change omit_default_port to be default enabled #635

Closed
SamSaffron opened this issue Jul 3, 2017 · 1 comment
Closed

Change omit_default_port to be default enabled #635

SamSaffron opened this issue Jul 3, 2017 · 1 comment

Comments

@SamSaffron
Copy link
Contributor

SamSaffron commented Jul 3, 2017

Regarding: 7933037

We recently spent quite a few hours at Discourse debugging this finally arriving at:

discourse/discourse@0ba3910

I think a log of the reasoning for going for verbose by default

https://stackoverflow.com/a/3364396/17174

See section 14.23 of the HTTP spec which specifies that the port # should be included if its not the default port of 80.

The "canonical" answer there is not really correct, the spec says:

A "host" without any trailing port information implies the default port for the service requested (e.g., "80" for an HTTP URL).

Every browser on the market operates with this assumption and omits default port for HTTPS/443 and HTTP/80. This is also the behavior of net/http in Ruby and almost every other client out there.

There are quite a few proxies in the public web that "forget" to implement hostname:80 and hostname:443 in the host rules. This can (and has) lead to cases where excon will fail and other libraries work.

I can not see any advantage in having omit_default_port default off, having excon behave very differently to default web browsers just leads to enormous amounts of painful debugging when you hit edges.

Would you consider changing the default here?

@geemus
Copy link
Contributor

geemus commented Jul 6, 2017

Hey, sorry to hear about your troubles here. I've also been bit by that more times than I would like. I definitely think omit is the right default (and that we might not even need the option at all). That said, I've been concerned that dropping it could also break things for current usage, so I had deferred to a major release (which I never quite seem to get to). There is another issue with some earlier discussion around this here: #357

Which is all to say, I agree with your assessment, I've just struggled with the right framing/timing of releasing such a change. I certainly welcome your feedback, and would just ask for us to consolidate discussion on the existing issue to keep things together if that's alright with you?

Thanks!

@geemus geemus closed this as completed Jul 6, 2017
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

No branches or pull requests

2 participants