Added support for auto pagination

[olivierbellone]

This PR adds support for auto pagination. The following code:

$charges = \Stripe\Charge::all();
foreach ($charges->autoPagingIterator() as $charge) {
    // Do something with $charge
}

will iterate over all charges. No more need to fiddle with pagination parameters! Yay!

The autoPagingIterator() method and its associated tests are basically a port of stripe-python's auto_paging_iter().

r? @brandur. I'm not super happy with the $_lastParams attribute and accessors I had to add to StripeObject, maybe there's a better way to retrieve those? Happy to update the PR if so.

[olivierbellone]

Aww, looks like yield was introduced in PHP 5.5 :(

I'll see if I can update the PR to make it compatible with PHP 5.3 and 5.4.

[olivierbellone]

There we go! I replaced the function with yield with a new AutoPagingIterator class that implements Iterator. The code is a bit clunkier but I guess that can't be helped.

[brandur]

Nitpick, but maybe just clean up the whitespace around these parameters and alphabetized them?

[brandur]

Left a few minor comments, but this looks great! Awesome work @olivierbellone!

r? @brandur. I'm not super happy with the $_lastParams attribute and accessors I had to add to StripeObject, maybe there's a better way to retrieve those? Happy to update the PR if so.

Hmm, it probably isn't much better, but do you think it would be better if we moved the request parameters over to Collection instead? That's the only place we really need to use them and it might keep the interface a little more minimal. For what it's worth, it's also how we ended up doing it in Java's pagination implementation.

I can go either way on that one though.

Aww, looks like yield was introduced in PHP 5.5 :(

PHP has every programming language feature ever invented and yield didn't make it until 5.5?! :/ Thanks for updating the implementation though!

[olivierbellone]

@brandur Thanks for the feedback! I updated the PR with your remarks:

• the request parameters are now saved in Collection, and StripeObject and ApiResource are untouched.
• I added a short documentation to autoPagingIterator() (stolen from the Java bindings)

[brandur]

Hey @olivierbellone! Great fixes. This is looking really good to me.

Sorry to push it, but can I make one more requested it change? It might be advantageous if we could send in an additional request option in the initial collection initialization here, and just verify that it appears in the second request below. This has the advantage of checking that parameters that were given to the original collection will be sent along in subsequent requests as expected.

I remember that I messed up something along these lines in the Java bindings and any additional sanity checks that we can put in would probably be a reasonably good idea.

[olivierbellone]

@brandur Right, this crossed my mind as well, but I forgot to add a test for it.

While testing this I also realized the AutoPagingIterator class tried to directly access _opts on the collection. This worked in my first version that used yield because it was directly implemented in Collection, but now that the code is in a separate class, I had to add a getter in StripeObject to access it. (PHP is stupid only raises a notice and not an exception when you try to access a protected attribute from outside a class 😠)

I'm not too sure how to test this though -- I think it would have to be tested into the request mocker. Thoughts?

EDIT: on second thought, I think the getter might be unnecessary and simply not passing an options argument would work equally well. Still not sure how to test it though.

EDIT2: okay, I tested it manually and the _opts getter is indeed unnecessary. However, the parameters are not carried over now that I'm storing them directly in Collection. I think it's because the creation of the first Collection object does not call any method in Collection itself. It's getting late here, I'll give it another shot tomorrow.

[brandur]

While testing this I also realized the AutoPagingIterator class tried to directly access _opts on the collection. This worked in my first version that used yield because it was directly implemented in Collection, but now that the code is in a separate class, I had to add a getter in StripeObject to access it. (PHP is stupid only raises a notice and not an exception when you try to access a protected attribute from outside a class 😠)

Would it be possible to avoid the getter and just have Collection instantiate AutoPagingIterator with a reference to its internal request options?

return new Util\AutoPagingIterator($this, $this->_requestParams);

[olivierbellone]

Alright, we're getting there. Thanks for bearing with me! :)

I added a call to Collection::setRequestParams() in ApiResource::_all() in order to save the initial parameters. This should work because Util\Util::convertToStripeObject() should always return a Collection in this case. Nevertheless, do you think I should add a test for this, and maybe raise an exception if it's not the case?

[brandur]

Great changes! This is looking really solid now.

I added a call to Collection::setRequestParams() in ApiResource::_all() in order to save the initial parameters. This should work because Util\Util::convertToStripeObject() should always return a Collection in this case. Nevertheless, do you think I should add a test for this, and maybe raise an exception if it's not the case?

I guess I could see it either way. A custom exception is always a little nice because if it does blow up, you get a more helpful message, but on the other hand, we may be planning for a very unlikely contingency here. I'll leave it up to your good judgement :)

[olivierbellone]

Alright. Even though this should never happen, I think it's better to break things cleanly with an exception rather than let PHP do whatever PHP does (probably output a notice and go on its merry way, grr). So I added a check that should hopefully be entirely unnecessary :)

[brandur]

Awesome! Let's try this out.

[brandur]

Released in 3.9.0.