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

Use GET method to fetch the GHontent's content #247

Closed
wants to merge 1 commit into from

Conversation

Shredder121
Copy link
Contributor

This is a little more cache-friendly.

Let me know what you think.

@kohsuke
Copy link
Collaborator

kohsuke commented Mar 1, 2016

I believe this is already addressed in e0b109c

@Shredder121
Copy link
Contributor Author

It unfortunately isn't.
Only the HttpURLConnection.method was set by that change. Not the Requester.method.

This means that Requester.method is still set to POST, isMethodWithBody will return true, and uc.setDoOutput(true) will be called.

I use Okhttp, and their HttpURLConnectionImpl's method changes to POST if you tell it to open a stream to write to.

292  if (doOutput) {
293 if (method.equals("GET")) {
294 // they are requesting a stream to write to. This implies a POST method
295 method = "POST";

kohsuke added a commit that referenced this pull request Mar 2, 2016
From Shredder121,
--------------------
Only the HttpURLConnection.method was set by that change. Not the
Requester.method.

This means that Requester.method is still set to POST, isMethodWithBody
will return true, and uc.setDoOutput(true) will be called.

I use Okhttp, and their HttpURLConnectionImpl's method changes to POST
if you tell it to open a stream to write to.
@kohsuke
Copy link
Collaborator

kohsuke commented Mar 2, 2016

Thanks, that explains. Now I claim dbc79f8 fixes it!

@Shredder121
Copy link
Contributor Author

I indeed verified that it fixed it, thanks!
So I take it my PR is no longer necessary?

When can I expect to see this released?

@kohsuke kohsuke closed this Mar 12, 2016
@Shredder121 Shredder121 deleted the get-method-content branch March 12, 2016 21:21
@Shredder121
Copy link
Contributor Author

@kohsuke when can I expect to see this released?

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