Reworked pager is a breaking change in the API

[TimHas]

With the following change most of the error handling has to be reworked and rethought. Not cool.
#195

[drewish]

Would you care to say more?

[TimHas]

Sure, sorry didn't include it before.
An example:
the following code worked fine before and caught the error correctly:

try {
    $billing_info = Recurly_BillingInfo::get(123);
} catch (Recurly_NotFoundError $e) {
    return false;
}

Now we need to do something like

try {
    $billing_info = Recurly_BillingInfo::get(123);
    $billing_info->rewind(); // otherwise the actualy request will not be done
} catch (Recurly_NotFoundError $e) {
    return false;
}

[drewish]

Yeah that is strange because I wouldn't think billing info would be returning a pager.

[drewish]

I'm really confused on how that would be working when I tried running the second sample using the code in master I get:

Fatal error: Call to undefined method Recurly_BillingInfo::rewind() in /Users/andrew/projects/recurly-client-php/mytest.php on line 18

On the other hand running:

try {
    $billing_info = Recurly_BillingInfo::get('test2');
    var_dump($billing_info->first_name);
    var_dump($billing_info->last_name);
} catch (Recurly_NotFoundError $e) {
    return false;
}

gives me:

string(4) "asdf"
string(5) "234we"

Could you make sure you're on the latest release and then try running the following to see what that call is actually returning?

$billing_info = Recurly_BillingInfo::get('test2');
var_dump(get_class($billing_info));

[nerrad]

There is an issue with the pager (but I'm seeing this with the Recurly_SubscriptionList class):

Here's an example code snippet I have:

try {
                $subscriptions = Recurly_SubscriptionList::getForAccount(
                    $account_id,
                    $args
                );
                if ( $subscriptions instanceof Recurly_SubscriptionList ) {
                    $this->_subscription_user_cache[$user_id][$state] = $subscriptions;
                }
            } catch ( Recurly_NotFoundError $e ) {
                //let's verify if the account_code is correct, and if not then we reset the account
                try {
                    $account = Recurly_Account::get( $account_id );
                } catch ( Recurly_NotFoundError $e ) {
                    //account_code is incorrect so let's remove the cached link
                    self::remove_remote_account_id_for_user( $user_id );
                }
                $subscriptions = array();
            }

earlier code grabs the account_id that is linked to the local user. Previously, if there was an issue with the account_id, the Recurly_NotFoundError exception would be triggered, and I'd verify that it was the account_id.

However the client code calling this snippet is getting a valid Recurly_SubscriptionList object returned and then when it does this:

if ( $subscriptions instanceof Recurly_SubscriptionList ) {
                        $subscriptions->rewind();

that produces:

"PHP message: PHP Fatal error:  Uncaught exception 'Recurly_NotFoundError' with message 'Couldn't find Account with account_code = 56cf3e2868ebf' in /home/doodah/www/eelive/wp-content/plugins/pue-product-management/libraries/recurly/recurly/response.php:55
Stack trace:
#0 /home/doodah/www/live/wp-content/plugins/pue-product-management/libraries/recurly/recurly/pager.php(93): Recurly_ClientResponse->assertValidResponse()
#1 /home/doodah/www/live/wp-content/plugins/pue-product-management/libraries/recurly/recurly/pager.php(37): Recurly_Pager->_loadFrom('/accounts/56cf3...')
#2 /home/doodah/www/live/wp-content/plugins/pue-product-management/main/business/recurly/PUE_PM_Recurly_Adapter.php(449): Recurly_Pager->rewind()

The only thing I can think of, is this account_code is attached to a transaction that was declined, and does not exist as a full account in recurly (a transition account?). For some reason this valid account_code check is being delayed until the pager is run.

As the original poster pointed out, this requires a different method of error checking by client code. Is it expected that this exception would be triggered by the pager?

[nerrad]

It looks like the change is, that the requested Resource object will get returned regardless of whether the given account id is correct or not and the account id is only checked when the paging is triggered. Previously the request using the account_id was done immediately on instantiation which would have thrown the error.

[nerrad]

yah anything that is a child of Recurly_Pager is just instantiating without making an initial request (the request is delayed until the first rewind())

[TimHas]

@nerrad thanks for better overview of the problem. This is exactly what I meant :)

[nerrad]

fwiw (to the library devs), it "looks" like this might be expected behaviour with the changes introduced in #195 (i.e. delay the request until the first rewind()). However, one possible fix is to do an initial rewind in the Recurly_Pager::__construct()? That would at least preserve previous behaviour.

Another possible option is for any resources involving the account_id, verify that account_id is valid before returning the resource.

[drewish]

@nerrad ah, okay yeah that flow makes more sense. i'll spend some more time looking at this today.

[drewish]

Hey apologies I haven't been able to get into this and I'm out of the office for the rest of the week. It'll be at the top of my list on Monday.

[nerrad]

so it appears (as noted in #212) that the request for the resource is now done in the rewind() or count() iterator methods instead of the specific methods related to retrieving the resource (eg Recurly_SubscriptionList::getForAccount ) which is a significant change in behaviour. One cannot rely on the specific getters (for resources extending Recurly_Pager) having done the initial request any longer. IF this is intended it should probably be (if it isn't already...if so I missed it) documented fairly clearly.

[drewish]

I'm going to close this since it wasn't necessarily resolved as much as it seems everyone has moved past it.