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

Revert back to initialize_from from Util.convert_to_stripe_object #806

Merged
merged 3 commits into from Jul 5, 2019

Conversation

rattrayalex-stripe
Copy link
Contributor

@rattrayalex-stripe rattrayalex-stripe commented Jul 4, 2019

r? @ob-stripe
cc @stripe/api-libraries @brandur-stripe

In ea736eb, on #790, we changed most uses of initialize_from to be Util.convert_to_stripe_object. We believed this to be equivalent, but this was incorrect; as @AnotherJoSmith pointed out, this returned new objects without mutating instances in-place (or, more precisely, copying over previous attributes).

For example, I believe this would have broken flows like this:

charge = Stripe::Charge.retrieve id
charge.capture
assert charge.captured == true # broken!

I hope to implement a smoother fix to this in a follow-on, but for now, I reverted the offending commit, and tracked down the only new instances of Util.convert_to_stripe_object that have happened since (see this diff and grep `Util.convert_to_stripe_object).

I also hope to add tests shortly, but want to bias to getting this out.

@brandur-stripe
Copy link
Contributor

Thanks Alex. I think a pretty good case could be made that we actually want a new version of the object instead of refreshing the current instance, but big +1 to reverting to what we had previously while working on something else.

LGTM.

@brandur-stripe brandur-stripe merged commit d1ccff0 into master Jul 5, 2019
@brandur-stripe
Copy link
Contributor

Released as 4.21.2.

@ob-stripe ob-stripe deleted the ralex/fix-initialize-from branch January 6, 2020 19:53
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.

None yet

3 participants