-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[SR-7338] Handle cookies in URLSession #1542
[SR-7338] Handle cookies in URLSession #1542
Conversation
@swift-ci please test |
@ianpartridge Could you please review this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few suggestions.
let r = request.createMutableURLRequest() | ||
_configuration.configure(request: r) | ||
var r = request.createMutableURLRequest() | ||
r = _configuration.configure(request: r) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just return _configuration.configure(request: r)
?
var request = request | ||
httpAdditionalHeaders?.forEach { | ||
guard request.value(forHTTPHeaderField: $0.0) == nil else { return } | ||
request.setValue($0.1, forHTTPHeaderField: $0.0) | ||
} | ||
request = setCookies(on: request) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return setCookies(on: request)
?
if httpShouldSetCookies { | ||
//TODO: Ask the cookie storage what cookie to set. | ||
if let cookieStorage = self.httpCookieStorage, let cookies = cookieStorage.cookies(for: request.url!) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove the force unwrap, e.g.
if let cookieStorage = self.httpCookieStorage,
let url = request.url,
let cookies = cookieStorage.cookies(for: url) {
...
}
e9201e1
to
41c914a
Compare
@ianpartridge Thanks for the suggestions. I have made the necessary changes and updated the branch. |
@swift-ci please test |
@spevans @mamabusi @ianpartridge Would it be possible to get this merged into 4.2 branch? I think this is an important piece of functionality that's missing from Swift 4.2 release. Also, looks like this was partially already merged in via this PR: I think it was by accident? The new test cases made it to Swift 4.2 branch, but not the actual code... |
@maksimorlovich Sure, I'll be creating a PR soon for the Swift 4.2 branch. |
This PR implements storage of HTTP cookies received as part of the response from the server and setting of appropriate cookies on the URLRequest of the URLSession.