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

Bump macOS minimum deployment target to 10.15 #1925

Closed
wants to merge 5 commits into from

Conversation

calvincestari
Copy link
Member

@calvincestari calvincestari commented Aug 31, 2021

This PR is a DRAFT:

  • TODO: validation test for nil URLSessionTask.sendRequest return

This change is in preparation for being able to target SQLite.swift version 0.13.0 which requires a minimum of macOS 10.15.

Deprecations as a result of dropping 10.14:

  • Breaking: URLSessionTask.init is no longer available so URLSessionTask.sendRequest now returns an optional task with nil being returned when the request could not be sent.

Closes #1868

@calvincestari calvincestari mentioned this pull request Aug 31, 2021
6 tasks
@hwillson hwillson added this to the Release 0.49.0 milestone Sep 16, 2021
@calvincestari calvincestari force-pushed the macos-10.15-deployment-target branch from 8b251d7 to c0523d0 Compare September 17, 2021 16:41
var result: SecTrustResultType = .unspecified
SecTrustEvaluate(trust,&result)
if result == .unspecified || result == .proceed {
if SecTrustEvaluateWithError(trust,nil) {
Copy link
Member Author

@calvincestari calvincestari Sep 17, 2021

Choose a reason for hiding this comment

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

This logic loses the .unspecified (default) behaviour but I think it's better now that we take action based only on the return. We don't gather any error because it was not being logged before and I don't see any good way in this module to output it in a way that is actually useful to a production app.

SecTrustEvaluateWithError(:,:) is the synchronous replacement of the deprecated SecTrustEvaluate(:,:). It is now a simple boolean return.

@@ -205,8 +203,8 @@ open class SSLSecurity : SSLTrustValidator {
SecTrustCreateWithCertificates(cert, policy, &possibleTrust)

guard let trust = possibleTrust else { return nil }
var result: SecTrustResultType = .unspecified
SecTrustEvaluate(trust, &result)
_ = SecTrustEvaluateWithError(trust, nil)
Copy link
Member Author

Choose a reason for hiding this comment

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

The result was not being used here so we don't store it either with the new call. Again, no error was being reported perviously.

SecTrustEvaluateWithError(:,:) is the synchronous replacement of the deprecated SecTrustEvaluate(:,:). It is now a simple boolean return.

@calvincestari
Copy link
Member Author

This is on hold while #1943 is evaluated.

@calvincestari
Copy link
Member Author

I'm closing this PR since #1943 has been accepted. I debated pulling 1df7341 into #1940 but since all apps implementing apollo-ios must support iOS 12 there is no real need to break behaviour that existing users might be relying on; empty task vs. nil task.

@calvincestari calvincestari deleted the macos-10.15-deployment-target branch September 20, 2021 20:37
@ghost
Copy link

ghost commented Jun 5, 2022

@calvincestari My company's app has iOS 14 as its deployment target, so it's getting a deprecation warning on return URLSessionTask(). It sounds like you can't remove this because iOS 12, but would it be possible to use alternate code for higher iOS versions? I'd be happy to raise a PR if that would be helpful.

@calvincestari
Copy link
Member Author

Hi @OrindaCoder 👋🏻 - correct, we still support iOS 12 which is why the code still uses that deprecated URLSessionTask initializer. We intend to keep iOS 12 as the minimum for the upcoming 1.0 release but 2.0 will certainly bump that to iOS 13 or higher; see details in our Roadmap.

The solution here could be to simply merge the changes in 1df7341 into main as I mentioned above. We're still in 0.x releases so making that kind of breaking change now is somewhat acceptable. If you have an alternate solution though please feel free to submit a PR and we can certainly review and discuss.

@ghost
Copy link

ghost commented Jun 6, 2022

@calvincestari I will wait for a future version of Apollo that fixes this warning. Thanks for all the work you do!

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.

Deprecated initialization of URLSessionTask
2 participants