Change minimum requirements to Xcode9 / iOS 9 #851

bdorfman-stripe · 2017-12-12T01:03:41Z

This is a pretty monster change because so many things needed to change to get it working, but I have tried to split each commit up into a logical chunk so you may want to review them one at a time.

cc @danj-stripe (in case you also wanted to skim the changes).

Summary of changes:

Change all our settings to min ios version 9
Remove all AddressBook framework references in favor of Contacts framework
Turn FauxPas back on for static framework only (it seems to only be broken for checking dynamic frameworks and this gives us some linting at least)
Replace all respondsToSelector checks and FauxPas APIAvailability rule with Xcode9's @available function and warning.
Replace out custom scroll view insetting code with the system default (the custom code was broken on iOS 11 and there didn't appear to be a clear reason for not using the default behavior).
Cleanup snapshot test logic. Made a new helper for preparing a VC for being snapshotted which works reliably on iOS11 and made everything use that.

The faux pas 1.7.2 problem seems to only be with the dynamic framework target, so we can continue to lint the static lib target to get some checking done. Also disable the APIAvailability rule since with xcode 9 this is built in with `@available` (which FauxPas doesn't recognize)

Audited all our `respondsToSelector` and FauxPas `APIAvailability` checks. Removed any unnecessary ones (now that we are on 9.0) and replaced all the relevant ones with `@available` checks instead.

Our custom version doesn't work correctly on iOS11 and I don't see a reason to not just use the default system behavior for this.

Make it public correctly. Replace all existing manual fauxpas defines with header import. Remove unnecssary defines/imports.

bg-stripe

just one question about the STPCoreScrollViewController change, otherwise looks good!

bg-stripe · 2017-12-12T18:46:47Z

ci_scripts/check_fauxpas.sh

-  --target "StripeiOS" \
-  --configFile "${config_path}" \
-  --minErrorStatusSeverity "${min_severity}"
+# Fauxpas 1.7.2 does not build our framework target correctly https://bitbucket.org/ali_rantakari/faux-pas/issues/117/unrecoverable-compiler-error-translation


ah, interesting that it's only the dynamic framework. guess we'll probably get most of the same warnings from linting just the static framework so that's good.

bg-stripe · 2017-12-12T18:48:25Z

Stripe/PublicHeaders/FauxPasAnnotations.h

@@ -0,0 +1,40 @@
+//
+//  FauxPasAnnotations.h


bg-stripe · 2017-12-12T19:04:24Z

Stripe/STPCoreScrollViewController.m

@@ -46,21 +54,4 @@ - (void)updateAppearance {
    }
 }

- (void)viewWillAppear:(BOOL)animated {


does removing this break things on iOS < 11?

Not that I can tell. iOS 9&10 still have automatic content insetting (automaticallyAdjustsScrollViewInsets). I changed this setting from off to on in STPCoreViewController. I'm not clear on why we had custom code to do this. Possibly the auto insetting was not present or buggy at some point? iOS 7 or 8? It seems to work correctly now, I played around in the 10.3.1 simulator. I can test in a 9.x simulator as well.

The new iOS11 replacement for automaticallyAdjustsScrollViewInsets is contentInsetAdjustmentBehavior and it is on the UIScrollView itself, which is why the check is in STPCoreScrollViewController's viewDidLoad. Whereas the call to automaticallyAdjustsScrollViewInsets is on UIViewController so it is in STPCoreViewController.

I just ran it in the 9.3 simulator and it seems to work fine there also. I thought perhaps the reason we had custom code was because of our custom insetting logic around keyboard insetting, but that all seems to work fine too with using the built in Apple insetting behavior, so I have no idea.

cool, sgtm – thanks for the explanation!

bg-stripe · 2017-12-12T19:06:56Z

Tests/Tests/FBSnapshotTestCase+STPViewControllerLoading.m

+#import "STPCoreScrollViewController+Private.h"
+
+@implementation FBSnapshotTestCase (STPViewControllerLoading)
+- (UIView *)stp_preparedAndSizedViewForSnapshotTestFromViewController:(UIViewController *)viewController {


bg-stripe · 2017-12-12T19:07:22Z

MIGRATING.md

@@ -1,5 +1,9 @@
 ## Migration Guides

+### Migrating from versions < 12.0.0
+* The SDK now requires iOS 9+ andXcode version is now 9+. If you need to support iOS 8 or Xcode 8, the last supported version is [11.5.0](https://github.com/stripe/stripe-ios/releases/tag/v11.5.0)


typo: andXcode

Despite Xcode recommending this change, it does not work correctly for some of our integration patterns. Thankfully our static framework validator CI script specifically checks for this case and yelled at me.

bg-stripe

lgtm!

bdorfman-stripe added 13 commits December 11, 2017 12:10

Bump minimum versions to 9

557a8bf

Remove all references to AddressBook framework

41522f5

Remove iOS 9 availability guard

707608a

Change test platform version

f4bbc22

Fix availability for geocoder

b41f649

Re-enable availability warnings

aa96b4e

Fixup availability checks

2da9894

Audited all our `respondsToSelector` and FauxPas `APIAvailability` checks. Removed any unnecessary ones (now that we are on 9.0) and replaced all the relevant ones with `@available` checks instead.

Change travis/tests to run on Xcode9/iOS11.2

2e06036

Use auto scroll content insetting

8a41e94

Our custom version doesn't work correctly on iOS11 and I don't see a reason to not just use the default system behavior for this.

Extract working ViewController snapshot setup code into a helper

91add43

Update view controller snapshot tests

ae5899b

Update migrating

39d344f

bdorfman-stripe requested a review from bg-stripe December 12, 2017 01:03

bdorfman-stripe added 2 commits December 11, 2017 17:05

Merge branch 'master' into bdorfman-xcode9-ios9

26641ba

Update to Xcode 9.2 recommended project settings

9cd8541

bdorfman-stripe assigned bg-stripe Dec 12, 2017

Fix FauxPas header

3ef1ebb

Make it public correctly. Replace all existing manual fauxpas defines with header import. Remove unnecssary defines/imports.

bg-stripe approved these changes Dec 12, 2017

View reviewed changes

stripe-ci added the approved label Dec 12, 2017

stripe-ci assigned bdorfman-stripe and unassigned bg-stripe Dec 12, 2017

bdorfman-stripe added 3 commits December 12, 2017 11:59

Update changelog to mention AddressBook

0788c85

Fix fauxpas warning

ba65f9e

Reverse product bundle identifier changes.

8bcc8cb

Despite Xcode recommending this change, it does not work correctly for some of our integration patterns. Thankfully our static framework validator CI script specifically checks for this case and yelled at me.

stripe-ci removed the approved label Dec 12, 2017

bg-stripe mentioned this pull request Dec 12, 2017

add largeTitleDisplayMode to PaymentContext #849

Merged

bg-stripe approved these changes Dec 12, 2017

View reviewed changes

stripe-ci added the approved label Dec 12, 2017

Fix typo in MIGRATING

c52130c

stripe-ci removed the approved label Dec 12, 2017

bdorfman-stripe merged commit 1cf5109 into master Dec 12, 2017

danj-stripe deleted the bdorfman-xcode9-ios9 branch January 12, 2018 19:02

mludowise-stripe pushed a commit that referenced this pull request Mar 13, 2022

🎨 Update close and back icon and make delete button red (#851)

373509a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change minimum requirements to Xcode9 / iOS 9 #851

Change minimum requirements to Xcode9 / iOS 9 #851

bdorfman-stripe commented Dec 12, 2017

bg-stripe left a comment

bg-stripe Dec 12, 2017

bg-stripe Dec 12, 2017

bg-stripe Dec 12, 2017

bdorfman-stripe Dec 12, 2017

bdorfman-stripe Dec 12, 2017

bg-stripe Dec 12, 2017

bg-stripe Dec 12, 2017

bg-stripe Dec 12, 2017

bg-stripe left a comment

Change minimum requirements to Xcode9 / iOS 9 #851

Change minimum requirements to Xcode9 / iOS 9 #851

Conversation

bdorfman-stripe commented Dec 12, 2017

bg-stripe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bg-stripe left a comment

Choose a reason for hiding this comment