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

Deprecate non-APIClient sources of publishableKey, stripeAccount #1474

Merged
merged 21 commits into from
Jan 23, 2020

Conversation

yuki-stripe
Copy link
Collaborator

@yuki-stripe yuki-stripe commented Jan 15, 2020

Summary

Part 2 of #1469

  1. Preserve Stripe.setDefaultPublishableKey behavior

This is the default value (in the programmatic sense) of publishable key for any new APIClient instance.

  1. Deprecate STPPaymentConfiguration.{publishableKey, stripeAccount}

For legacy reasons, make this a proxy for STPAPIClient.shared.{publishableKey, stripeAccount} if using the singleton.

  1. Remove apiKey private property APIClient
  2. Simplify the APIClient initializers by removing the dependency on PaymentConfiguration. PaymentConfiguration is just another property in APIClient, instead of the sorta-source-of-truth of publishableKey. It defaults to [PaymentConfiguration sharedConfiguration].

Motivation

https://paper.dropbox.com/doc/psyduck-why-iOS-SDK-API-Configuration-LTvIMxj6CBy1Mw4ntaU0l#:uid=989210153385098517970797&h2=Risks

If users ignore the deprecation warnings, the updated behavior of these deprecated properties should result in no change for most (~everyone except for Connect users cloning PMs and direct charging their connected accounts).

Testing

Existing tests still pass.

Stripe/PublicHeaders/STPAPIClient.h Show resolved Hide resolved
Stripe/PublicHeaders/STPAPIClient.h Show resolved Hide resolved
Stripe/STPAPIClient.m Show resolved Hide resolved
Stripe/STPAPIClient.m Show resolved Hide resolved
@@ -126,7 +126,7 @@ + (NSDictionary *)parametersForPayment:(PKPayment *)payment {
payload[@"pk_token"] = paymentString;
payload[@"card"] = [self addressParamsFromPKContact:payment.billingContact];

NSCAssert(!(paymentString.length == 0 && [[Stripe defaultPublishableKey] hasPrefix:@"pk_live"]), @"The pk_token is empty. Using Apple Pay with an iOS Simulator while not in Stripe Test Mode will always fail.");
NSCAssert(!(paymentString.length == 0 && [[STPAPIClient sharedClient].publishableKey hasPrefix:@"pk_live"]), @"The pk_token is empty. Using Apple Pay with an iOS Simulator while not in Stripe Test Mode will always fail.");
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should still reference [Stripe defaultPublishableKey]`

@@ -410,7 +410,7 @@ + (NSMutableDictionary *)commonPayload {

+ (NSDictionary *)serializeConfiguration:(STPPaymentConfiguration *)configuration {
NSMutableDictionary *dictionary = [NSMutableDictionary dictionary];
dictionary[@"publishable_key"] = configuration.publishableKey ?: @"unknown";
dictionary[@"publishable_key"] = [STPAPIClient sharedClient].publishableKey ?: @"unknown";
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be [Stripe defulatPublishableKey]

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As it stands, STPAPIClient sharedClient is still more likely to be accurate of the two, since if they differ, it means the user set STPAPIClient.sharedClient.publishableKey.

e.g. You could never call Stripe.setDefaultPublishableKey and only set STPAPIClient.sharedClient.publishableKey.

Copy link
Contributor

Choose a reason for hiding this comment

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

mm, I have the opposite understanding. You should always call stripe.setDefaultPublishableKey and only set STPAPIClient.sharedClient.publishableKey for very specific use cases wher eyou know what you're doing. Otherwise what's the point of Stripe.defaultPublishableKey?

}

+ (NSString *)defaultPublishableKey {
return [STPPaymentConfiguration sharedConfiguration].publishableKey;
return _defaultPublishableKey;
Copy link
Contributor

Choose a reason for hiding this comment

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

I might actually go so far as to make this non-nil (initial value of empty string) so that we can have true null_resettable behavior in STPAPIClient. Thoughts @davidme-stripe ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's a bit weird to have that as an explicit default value, because an empty string isn't a valid value. Like, we fire an assertion if you try to set publishableKey to an empty string.

@yuki-stripe yuki-stripe changed the base branch from yuki/customercontext-use-shared-apiclient to master January 22, 2020 01:00
Usage | Before | After all configuration changes:
- PaymentContext configuration. This never worked and there's no use case. | Still doesn't work, still no use case.
- CustomerContext configuration. This worked, but there is no use case. | Now, it only works if you set before STPAPIClient.shared is initialized. There's still no use case.
- PaymentHandler, APIClient.shared configuration. This worked if you set before STPAPIClient.shared is initialized. | No change.
…f the sorta-source-of-truth of publishableKey.
This reverts commit 314256004a790edd3c35b090c3ed0b0b9506b84e.
…eKey only (this is the status quo behavior).
@yuki-stripe yuki-stripe force-pushed the yuki/deprecate-initwithconfiguration branch from 4af824b to 79c84f2 Compare January 22, 2020 01:12
MIGRATING.md Outdated Show resolved Hide resolved
MIGRATING.md Outdated Show resolved Hide resolved
MIGRATING.md Outdated Show resolved Hide resolved
MIGRATING.md Outdated Show resolved Hide resolved
MIGRATING.md Outdated
Comment on lines 20 to 26
For example, if you are a Connect user who stores Payment Methods on your platform, but clones PaymentMethods to a connected count and create direct charges on that connected account ie if:

1. You never set `STPPaymentConfiguration.shared.stripeAccount`
2. You set `STPAPIClient.shared.stripeAccount`

We recommend you do the following:

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe rewrite as:

For example, if you only want to use a Connected account for a single transaction, your integration could look like:

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That sounds different from the use case described. These users want to use a Connected account for all transactions, if transactions means payments, but they want to retrieve Customers and their PMs on the platform.

Copy link
Contributor

Choose a reason for hiding this comment

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

okay, as written it was confusing to me but I think you understand the use case better :)

yuki-stripe and others added 5 commits January 22, 2020 15:26
Co-Authored-By: Cameron <36750494+csabol-stripe@users.noreply.github.com>
Co-Authored-By: Cameron <36750494+csabol-stripe@users.noreply.github.com>
Co-Authored-By: Cameron <36750494+csabol-stripe@users.noreply.github.com>
Co-Authored-By: Cameron <36750494+csabol-stripe@users.noreply.github.com>
MIGRATING.md Outdated Show resolved Hide resolved
@yuki-stripe yuki-stripe merged commit d4c7a71 into master Jan 23, 2020
@yuki-stripe yuki-stripe deleted the yuki/deprecate-initwithconfiguration branch January 23, 2020 18:15
yuki-stripe pushed a commit that referenced this pull request Oct 4, 2022
StripeCore: added support for seeing HTTP response status code as part of StripeServiceError.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants