Change STPPaymentCardTextField to copy the cardParams object when it's set or read.
#1031
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Using the runtime to iterate through all the properties and make sure they're equal.
…orked. Using the runtime to iterate through all the properties and make sure they're equal, or not, as appropriate.
Also changed the `setName:`/`setAddress:` overrides slightly to avoid calling `setName:` from `setAddress:`
… strong. Fixes issue from #1027
…ternalCardParams` ref
…extField` and `STPCardParams`
There are a bunch of places where we have `STPAddress` properties, and none of them copy the object. Instead of changing `STPCardParams.address` as a one-off to be `copy`, keep this `strong`. I think the right thing to do would be to add a `STPMutableAddress` subclass, and take a pass through the codebase. There are places where the STPAddress *should* be mutable, like `STPCardParams.address`, allowing caller to build up the params to send to Stripe. There are other places, like objects returned from the API, where it should be immutable. Ex: `STPCard.address`. This also makes the migrating instructions a lot simpler to understand.
Stripe/STPPaymentCardTextField.m
Outdated
| @@ -638,7 +638,7 @@ behaves identically to user entering text (and will have the same forwarding | |||
| */ | |||
| STPFormTextField *originalSubResponder = self.currentFirstResponderField; | |||
|
|
|||
| self.internalCardParams = cardParams; | |||
| self.internalCardParams = [cardParams copy]; | |||
| [self setText:cardParams.number inField:STPCardFieldTypeNumber]; | |||
There was a problem hiding this comment.
might be worth adding a comment here highlighting that it's important to use cardParams and not internalCardParams for the sets
csabol-stripe
approved these changes
Oct 24, 2018
…bout the need for it Per comment in `setCardParams:`, `setText:inField` calls trigger `paymentCardTextFieldDidChange:` and `UIControlEventValueChanged`. This passes control back to the application code. If the app code calls the `STPPaymentCardTextField.cardParams` property accessor, the `internalCardParams` object is mutated to match the UI's state, and that breaks the behavior of this method (motivating reason for this patch) Similarly, if the app code changes the `STPCardParams` object that they own & passed into this method during the delegate or value changed event, those changes could unexpectedly show up. The `desiredCardParams` copy is used so that at the end of the method's execution, its done what was expected.
|
re-review? @csabol-stripe |
|
I'm investigating the failure of |
csabol-stripe
approved these changes
Nov 6, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Exposing the internal
STPCardParamsobject led to weird interactions, like #1027. Italso doesn't match our desired information hiding. Setting some fields on the
cardParamswould persist (name, address, etc), while setting others had no effect (card number, etc).
By copying in both setter & getter, the internal state of this view is protected, and
must be changed by setting a new object on the property.
We expect well-behaved code to be backwards compatible, but this is a breaking change
that'll require a major version bump because it changes the interface & behavior.
Motivation
#1027 and IOS-958
Testing
Tests were written that illustrate the problem & current behavior, and then updated as the
behavior changed.
Sample projects were inspected and tested to make sure they weren't using the payment text
field in an unsupported way.