GoCardlessPro\Core\Paginator does not work if a $params["before"] filter is used #32
Comments
oschonrock
changed the title
|
As you've worked out, the function of You're going beyond the use case which the library is designed to serve, which assumes that you might specify filters, but not set the pagination options yourself. Have you thought about filtering events using |
|
Hi Tim Thanks for your response This is the primary of the 3 issues I filed, the other 2 are just related thoughts which I came across across while implementing my own de-pagination What I would "expect it to do" is:
that all works fine .. However
The problem gets worse when you use ->all() with [before => someid] which should "logically" (?) give me "all after someid" ..but it doesn't ...it gives me "50 most recent" and then stops (so it is the same as ->list(before=>50) which is the same as ->list() ..making filters all a bit pointless? As far as I can see all() and list() are broken with any filter (the reason is to do with the paginator and the way it tries to "fetch next page". Please try it to see what I mean) Using "created_at" doesn't change anything, has the same problems, and has the added disadvantage that it is potentially non-deterministic (ie there could be 2 with same timestamp). really if list() and all() in your client are not usable with [somefilter => value] then the code should throw at least an Exception('filters do not work with all() and list()'). (where do they work?) That would be truthful. Or find a way to make it work. When I implemented my own de-paginator (ie ->all()) i found I needed to handle the "no filter" and "some filter" cases completely different. They need to use inverted logic in 2 or 3 places. That's why you Paginator fails, because it does not understand this. |
|
I've just been having a play around myself to investigate a bit further the issues you're raising. I'm starting with listing the customers in my account. Here's a copy of my scratchpad: <?php
require 'vendor/autoload.php';
$gocardless = new \GoCardlessPro\Client(['access_token' => 'redacted', 'environment' => \GoCardlessPro\Environment::LIVE]);
// As expected, I see outputted the emails of my most recent 50 customers, since
// `list()` does not automatically paginate.
$customers = $gocardless->customers()->list();
foreach ($customers->records as $customer) {
echo $customer->email . "\n";
}
// I can add a `limit` and some filtering to that, and all works as expected
$customers = $gocardless->customers()->list(["params" => ["created_at[lt]" => "2017-05-01T09:30:00Z", "limit" => 5]]);
foreach ($customers->records as $customer) {
echo $customer->email . "\n";
}
// If I use a `before` with `list()` to get records earlier in the list (i.e. more
// recent records, since we use reverse-chronological ordering), I get the
expected results (i.e. I see the emails of the five customers created most
// recently after CU000728W3MBQ0)
$customers = $gocardless->customers()->list(["params" => ["limit" => 5, "before" => "CU000728W3MBQ0"]]);
foreach ($customers->records as $customer) {
echo $customer->email . "\n";
}
// Now, let's try auto-paginating with `all()`. I see all of my customers emails
// outputted.
$customers = $gocardless->customers()->all();
foreach ($customers as $customer) {
echo $customer->email . "\n";
}
// With a `limit` specified, I get the same output, but it takes a bit longer
// to generate.
$customers = $gocardless->customers()->all(["params" => ["limit" => 5]]);
foreach ($customers as $customer) {
echo $customer->email . "\n";
}
// Let's start experimenting with filters. If I set a `created_at[gt]` filter, the
// automatic pagination still works, and I see all of the matching customers.
$customers = $gocardless->customers()->all(["params" => ["created_at[gt]" => "2015-11-03T09:30:00Z"]]);
foreach ($customers as $customer) {
echo $customer->email . "\n";
}
// If I add a `before`, then it doesn't paginate automatically, rather it just does
// the first page. I think this makes some kind of sense since if you're using
// `before`, it suggests you're trying to control the pagination and are going
// beyond what the library supports. (You're essentially trying to use `before`
// as a filter, rather than relying on the library's built-in pagination.)
$customers = $gocardless->customers()->all(["params" => ["created_at[gt]" => "2015-11-03T09:30:00Z", "before" => "CU00071R7A40EG"]]);
foreach ($customers as $customer) {
echo $customer->email . "\n";
}
// If I specify an `after`, it goes a bit mad because `Paginator` resets this as
// part of its operation - see the `initial_response()` method.
$customers = $gocardless->customers()->all(["params" => ["created_at[gt]" => "2015-11-03T09:30:00Z", "after" => "CU00071R7A40EG"]]);
foreach ($customers as $customer) {
echo $customer->email . "\n";
}The conclusions I've come to are as follows - do let me know if anything is wrong or doesn't make sense:
You're right that using the |
|
In terms of improving the situation, I'd be in favour of making the library paginate in the opposite direction if you specify a |
|
Thanks for looking into this. I think we are beginning to understand each other. I have also made some stripped down tests: $events = $client->events()->list(['params' => ['limit' => 20]]);
echo "\nlist(limit=20)\n";
foreach ($events->records as $event)
{
echo $event->id . ' ' . $event->created_at . "\n";
}
echo "\nlist(limit=5, before=EV006F34D0YD8K)\n";
$events = $client->events()->list(['params' => ['limit' => 5, 'before' => 'EV006F34D0YD8K']]);
foreach ($events->records as $event)
{
echo $event->id . ' ' . $event->created_at . "\n";
}
echo "\nall(before=EV006F34D0YD8K) .. while hacking Paginator to grab pages of 5 record at a time\n";
$events = $client->events()->all(['params' => ['before' => 'EV006F34D0YD8K']]);
foreach ($events as $event)
{
echo $event->id . ' ' . $event->created_at . "\n";
which gives the following output: While I agree that the "list(limit=5, before=EV006F34D0YD8K)" is arguably correct (ie it's the 5 events just "before" the eventid specifiied -Sorry, I had that wrong in my last comment above)..it does show part of where the problem begins. When not specifying a filter ->list(limit=5) starts with the most globally recent event matching the (non-existent) criteria. Whereas list(limit=5, before=someid) shows the 5 events starting with event "5 up" from "someid". So when you try to de-paginate (either by calling ->all() or by iteratively calling ->list(limit=5, before=SOMEOTHERID) it doesn't work, because it's non-trivial which SOMEOTHERID to use, whether to use a "before" or "after" filter and whether to append or prepend the second set of records. For what it's worth, here is my de-pagination method which takes a $last_gc_event_id_processed, which we store each time we batch process events, (or null if we are starting from scratch) and returns all events chronologically "after" that id in "chronologically ascending" order (ie the order we must process). I am not super happy with the code, but it is partially a result of the bending over backwards I had to do, to get this to work. The key part is the IF statement in the middle of the de-pagination loop, which reverses the logic of whether to use $response->after or $response->before for the second "page" and whether to APPEND or PREPEND that second page to the first page. My code also uses array_reverse in both cases because I want chronologically ascending (for chronologically descending the APPEND/PREPEND logic would need to be reversed). The WHILE loop termination condition is the other interesting part, again requiring a switch on whether $last_gc_event_id_processed was passed or not. public static function listEvents(GoCardlessOrganisationPaymentMethod $opm, $last_gc_event_id_processed = null)
{
$client = self::getClient($opm);
$params = [];
$limit = 5; // slices of 5, but combined here, because complex and therefore easier for calling code
$params["limit"] = $limit;
if ($last_gc_event_id_processed !== null)
{
$params["before"] = $last_gc_event_id_processed;
}
$records = [];
do
{
$response = $client->events()->list(['params' => $params]);
if ($last_gc_event_id_processed === null)
{
// and use after condition when retrieving all (crazy!) ref https://github.com/gocardless/gocardless-pro-php/issues/34
$params["after"] = $response->after;
// prepend the reversed records (crazy!) ref https://github.com/gocardless/gocardless-pro-php/issues/34
$records = array_merge(array_reverse($response->records), $records); // merge in chronologically increasing order
}
else
{
// use before condition when last_id given (crazy!)
$params["before"] = $response->before;
// append the reversed records (crazy!)
$records = array_merge($records, array_reverse($response->records)); // merge in chronologically increasing order
}
}
while (count($response->records) == $limit && // if full set there might be more
(($last_gc_event_id_processed === null && $response->after !== null) || // use after condition when retrieving all (crazy!)
($last_gc_event_id_processed !== null && $response->before !== null))); // and use before condition when last_id given (crazy!)
return $records;
}Here is me calling that method: echo "\ncustom depagination listEvents(\$last_gc_event_id_processed = EV006F34D0YD8K) .. results in chrono asc order\n";
$events = GoCardlessPspTransaction::listEvents($opm, 'EV006F34D0YD8K');
foreach ($events as $event)
{
echo $event->id . ' ' . $event->created_at . "\n";
}
echo "\ncustom depagination listEvents(\$last_gc_event_id_processed = null) .. all results in chrono asc order\n";
$events = GoCardlessPspTransaction::listEvents($opm, null);
foreach ($events as $event)
{
echo $event->id . ' ' . $event->created_at . "\n";
}
and gives this result: |
Without query filter GoCardlessPro\Core\Paginator works fine
but with filter it does not
I ended up implementing my own "de-pagination" which is actually surprisingly non-trivial if a filter is used (to do with the reverse chronological order slice).
Did I miss sth? Or this is a straight bug / missing implementation?
The text was updated successfully, but these errors were encountered: