Update deployment target to iOS 13 #430
Conversation
(replace with SecTrustEvaluateWithError + SecTrustGetTrustResult)
# Conflicts: # WordPressKit.podspec
ealeksandrov
force-pushed
the
update/deployment-target
branch
from
7bcf924 to
474654f
Compare
There was a problem hiding this comment.
Thank you @ealeksandrov 🙌
I particularly appreciate how you went ahead and updated everything that was necessary and like your 1 warning fix per commit approach 👍
Sorry for not getting onto this earlier. Now you have a little conflict on the pod version because I released 4.39.0 stable as part of the WordPress iOS 18.0 code freeze.
| DYLIB_INSTALL_NAME_BASE = "@rpath"; | ||
| INFOPLIST_FILE = WordPressKit/Info.plist; | ||
| INSTALL_PATH = "$(LOCAL_LIBRARY_DIR)/Frameworks"; | ||
| IPHONEOS_DEPLOYMENT_TARGET = 11.0; |
There was a problem hiding this comment.
It's not clear in this diff, but by opening the project in Xcode I saw you decided to remove the target-level deployment setting, making them all use the project-level one.
Great idea 👍
| _ = scanUpToString(quoteString) | ||
| _ = scanString(quoteString) | ||
| quote = scanUpToString(quoteString) | ||
| _ = scanUpToString(quoteString) |
There was a problem hiding this comment.
This change looks good.
It's understandably out of scope for this PR, I wish there had been already a test to verify it 😞
There was a problem hiding this comment.
I agree. I've spent some time digging in and:
- Looks like this extension is broken (or I don't understand the logic behind it in my tests).
- WP-iOS has its own very similar extensions for NSScanner and NSAttributedString.
- This
Scannerextension used only inNSMutableAttributedStringextension which is unused in library and declared internal.
I've decided to decouple this issue into separate branch (and remove duplicated code from WP there):
https://github.com/wordpress-mobile/WordPressKit-iOS/tree/update/quotes-attributes-extension
There was a problem hiding this comment.
Great digging! Keep me posted on in please. Now I'm curious to see how the cleanup will evolve 🍿
There was a problem hiding this comment.
Latest update: unused extension removed from lib in #449 + WP part refactored with tests in wordpress-mobile/WordPress-iOS#17259
| var evaluationError: CFError? | ||
| _ = SecTrustEvaluateWithError(serverTrust, &evaluationError) | ||
| var result = SecTrustResultType.invalid | ||
| let certificateStatus = SecTrustEvaluate(serverTrust, &result) | ||
| let certificateStatus = SecTrustGetTrustResult(serverTrust, &result) |
There was a problem hiding this comment.
Again, I don't think improving this code is necessarily in scope for this PR, but I noticed two things.
evaluationErroris unused. I guess we don't get a warning from the compiler because of the assignment in line 334, but the code itself never reads it. Is there any kind of error handling that we should do here? Maybe previous authors have more insight on the kind of treatment this code deserves? cc @jleandroperez @astralbodies- While there is a test for the class (🎉) it might not be enough to cover this scenario. I overrode
resultwith.invalidafter this line and the test still passed.
There was a problem hiding this comment.
Right, I was also experimenting with it, but decided to keep exact same logic as before. Any additional input on this would be nice!
Removed unused error var in 4669e9f.
There was a problem hiding this comment.
Hey there folks! I'm afraid I can't recall working on this code, too many years :(
That being said, if there's a Trust evaluation error, I doubt there's much we can do. Fallback to "default handling", and the URL Request should fail IMO
# Conflicts: # WordPressKit.podspec
Description
This PR updates deployment target to iOS 13 in line with other libraries, WP-iOS and WC-iOS.
The main reason for immediate upgrade is CocoaPods issue here - wordpress-mobile/WordPressAuthenticator-iOS#612 (comment)
It also fixes all new deprecation warnings.
Due to issues with "boot simulator" CI step I had to also update Xcode and iOS versions in CircleCI config.
Testing Details
Build and run unit tests. Please check code for deprecation updates closely since they are not covered by unit tests.