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 to missing scheme handling (defaults to http) #620

Closed
wants to merge 3 commits into from

Conversation

jonathon-love
Copy link

Fixes #619
Fixes #607

@cderv
Copy link
Contributor

cderv commented Oct 15, 2019

Thanks !
Two questions I have on this:

I am just wondering if we should assume that any parse url without scheme should be http. why not https ?

Also, I wonder if this type of fix (if scheme is null, assume http) should be better in request_perform directly.
I see parse_url as a utility function that helps get the pieces that compose a url. Currently, It could be used to know if a url as a scheme or not by testing if it returns null - with this fix, it can't. Do we want this change in httr::parse_url... 🤔

Just sharing thoughts to help decide what is best.

@jonathon-love
Copy link
Author

I am just wondering if we should assume that any parse url without scheme should be http. why not https ?

the standard behaviour of web-browsers et al. is to default to http when no scheme is provided. and it would be consistent with the earlier versions of httr built against curl 3. (additionally, most https sites issue a redirect from http:// to https:// anyway)

Also, I wonder if this type of fix (if scheme is null, assume http) should be better in request_perform directly.

oh yeah, that's a fair point. i'll make that change.

with thanks

Copy link
Member

@hadley hadley left a comment

Choose a reason for hiding this comment

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

Can you please add a bullet to the top of NEWS.md? It should briefly describe the change and end with (@yourname, #issuenumber).

R/request.R Outdated Show resolved Hide resolved
@petermeissner
Copy link

I have an issue reported in {robotstxt} directly related to this PR and also solved by this PR.

Will this PR be included in httr at some time in the future?

@hadley
Copy link
Member

hadley commented May 1, 2020

We don't have any plans to spin off a httr release at the moment.

@petermeissner
Copy link

Thanks, for the heads up.

@hadley
Copy link
Member

hadley commented Oct 31, 2023

httr has been superseded in favour of httr2, so is no longer under active development. Thanks for using httr, thanks for contributing, and my apologies that your contribution never made it into the package.

@hadley hadley closed this Oct 31, 2023
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.

URLs without protocol causes error in windows but not linux Error if request url without scheme
4 participants