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
[TIMOB-19154]: iOS9 Deprecate NSURLConnection and replace with NSURLSession API #40
Conversation
@hansemannn Ping! |
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.
Left some comments. Please also ensure to validate this with both client- and server-authentication once the https-PR is updated.
APSHTTPClient/APSHTTPRequest.m
Outdated
useSubDelegate = [self.connectionDelegate willHandleChallenge:challenge forSession:session]; | ||
} | ||
|
||
if(useSubDelegate) { |
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.
Formatting for all if-statements.
if (exception.userInfo) { | ||
dictionary = [NSMutableDictionary dictionaryWithDictionary:exception.userInfo]; | ||
} else { | ||
dictionary = [NSMutableDictionary dictionary]; |
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.
The dictionary should remain nil
if there is no user-info.
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.
This is copied from previous NSURLConnection related implementation.
if (exception.userInfo) { | ||
dictionary = [NSMutableDictionary dictionaryWithDictionary:exception.userInfo]; | ||
} else { | ||
dictionary = [NSMutableDictionary dictionary]; |
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.
Same here, don't set it if empty.
APSHTTPClient/APSHTTPRequest.m
Outdated
|
||
|
||
|
||
[self URLSession:self.session didBecomeInvalidWithError:error]; |
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.
Remove empty lines.
if (challenge.previousFailureCount) { | ||
[challenge.sender cancelAuthenticationChallenge:challenge]; | ||
completionHandler(NSURLSessionAuthChallengeCancelAuthenticationChallenge, nil); |
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.
This end of in handling the auth-challenge multiple times. See the below lines.
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.
Similar implementation was there in NSURConnection too. I think it will not create any problem as completionHandler is passing NSURLSessionAuthChallengeCancelAuthenticationChallenge, which will cancel the authentication challenge.
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.
Mostly looking good, left another one comment for clarification.
@@ -342,8 +240,7 @@ -(void)URLSession:(NSURLSession *)session task:(NSURLSessionTask *)task didRecei | |||
if(self.requestPassword != nil && self.requestUsername != nil) { | |||
handled = YES; | |||
NSURLCredential *credential = [NSURLCredential credentialWithUser:self.requestUsername | |||
password:self.requestPassword | |||
persistence:NSURLCredentialPersistenceForSession]; |
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.
Can you explain the line change #346? The default is NSURLCredentialPersistenceNone
which will now be used. If using NSURLCredentialPersistenceForSession
fails now, it should be fixed in a different place, in this case the appcelerator.https module.
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.
I am not sure why it is showing like this here. The actual code, if you see in file is -
NSURLCredential * credential = [NSURLCredential credentialWithUser:self.requestUsername
password:self.requestPassword persistence:NSURLCredentialPersistenceForSession];
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.
Haha. It's the formatting that makes it look like this. Please just break the line and it will be fine.
APSHTTPClient/APSHTTPRequest.m
Outdated
@@ -325,8 +325,7 @@ -(void)URLSession:(nonnull NSURLSession *)session didReceiveChallenge:(nonnull N | |||
if (self.requestPassword != nil && self.requestUsername != nil) { | |||
handled = YES; | |||
NSURLCredential *credential = [NSURLCredential credentialWithUser:self.requestUsername | |||
password:self.requestPassword | |||
persistence:NSURLCredentialPersistenceForSession]; | |||
password:self.requestPassword persistence:NSURLCredentialPersistenceForSession]; |
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.
Break instead of inline
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.
@vijaysingh-axway Can you fix this last one?
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.
I think now it got Fixed :). Ideally we should lint this module source code also.
FR Passed. Tested using all of the sample and referenced tickets in the issue ticket as well as the module examples and http test suite |
JIRA - https://jira.appcelerator.org/browse/TIMOB-19154