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

Alphabetize list and add missing objects #391

Merged
merged 1 commit into from
Nov 28, 2017
Merged

Conversation

ob-stripe
Copy link
Contributor

r? @dpetrovics-stripe (DP run, feel free to re-assign!)
cc @stripe/api-libraries @remi-stripe

Alphabetize the list of API resources in Util::convertToStripeObject() and add missing resources:

  • application_fee
  • balance

(This was not a big issue as the PHP library does not rely on this method to instantiate resource classes when retrieving a resource, but we might as well be comprehensive.)

@remi-stripe
Copy link
Contributor

This was not a big issue as the PHP library does not rely on this method to instantiate resource classes when retrieving a resource

Why didn't this properly work for ApplicationFee in that case? See my related commit 64e909b which had to fix this to ensure we did get an ApplicationFee instance instead of StripeObject

@ob-stripe
Copy link
Contributor Author

ob-stripe commented Nov 14, 2017

Why didn't this properly work for ApplicationFee in that case? See my related commit 64e909b which had to fix this to ensure we did get an ApplicationFee instance instead of StripeObject

Right, so the test that failed is testIsListable. The all() method does rely on Util::convertToStringObject() to instantiate list elements as the proper resource class.

Before the fix, objects would be returned as StripeObjects. You were still able to access any attribute, e.g. this worked:

$fees = \Stripe\ApplicationFee::all();
$fee = $fees->data[0];
$amount = $fee->amount;

However, you would not be able to use ApplicationFee methods, e.g. this would have failed:

$fee->metadata["key"] = "value";
$fee->save();

because $fee was a StripeObject and not an ApplicationFee.

@remi-stripe
Copy link
Contributor

Understood! I assumed that your PR description implied it was not an issue though but I see how limited the impact is ultimately.

@grey-stripe
Copy link
Contributor

LGTM

@ob-stripe ob-stripe merged commit deafdc5 into master Nov 28, 2017
@ob-stripe ob-stripe deleted the ob-fix-convert branch November 28, 2017 17:13
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.

4 participants