-
Notifications
You must be signed in to change notification settings - Fork 850
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
fix autopagination for Service #942
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, that looks like the proper fix. Thanks @pepin-stripe!
ptal @richardm-stripe |
Ok, will add codegen for this and release tomorrow. |
codegenned, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Should fix #939.
Briefly,
autoPagingIterator
expects thefilters
property onStripe\Collection
to be set from$params
.This was happening before in
\ApiOperations\All::all
https://github.com/stripe/stripe-php/blob/master/lib/ApiOperations/All.php#L33, but isn't done inBaseStripeClient#request
https://github.com/stripe/stripe-php/blob/master/lib/BaseStripeClient.php#L129-L140.
This PR introduces
BaseStripeClient#requestCollection
(andAbstractService#requestCollection
, to call it), which simply calls#request
and then->setFilters($params)
, and then we updated the "SubscriptionService" to use it.ptal @ob-stripe to see if this seems right (feel free to bikeshed on naming)
I prefer adding a separate #requestCollection method rather than adding the call conditionally to
#request
-- since->all
should always return a\Stripe\Collection
, it seems better that it call a method guaranteed to return the narrower\Stripe\Collection
rather than the broader\Stripe\Object