Add charge parameter to subscription termination functions

[aaron-junot]

Fixes #313

Playground script:

<?php
// Assumes subscription "47c96143efc6c189bbf5bb47969166b4" exists on the site
include './scripts/php/stub.php';

try {
  $subscription = Recurly_Subscription::get('47c96143efc6c189bbf5bb47969166b4');
  $subscription->terminateWithoutRefund(false);

  print "Subscription: $subscription" . "\n";
} catch (Recurly_NotFoundError $e) {
  print "Subscription Not Found: $e";
} catch (Recurly_Error $e) {
  print "Subscription already terminated: $e";
}

[drewish]

Not a block, from me but something to consider.

[drewish]

I'm wondering if there's other ways we could pass in this parameter... having unnamed Boolean parameters doesn't make for very readable code. Having to pass in an array with the options is more verbose but we'd be able to have a understandable bit of code:

$subscription->terminateAndRefund(array('charge_for_unbilled_usage' => false))

vs

  $subscription->terminateWithoutRefund(false);

[aaron-junot]

I did some research, and named parameters have apparently been a subject of debate in the PHP world. Since they don't exist at the moment, we basically have two options:

1. Do your suggestion of passing in an options array. This has the benefit of making it more explicit, but you lose the ability to specify the type of the option, so you have to do some validation within the function. At that point, would we throw an exception if charge_for_unbilled_usage was not a boolean? Would we even check for the type, or would we just check for its presence and if it's not falsey, assume that it's meant to be true? It's hard because the default by the API is true and not false, so I'm not a huge fan of this solution even though it's better than what I have now.

2. We could create class constants that evaluate to true or false so that people can pass in something that makes it more clear. So the API would look like

$subscription->terminateWithoutRefund(Recurly_Subscription::CHARGE_FOR_UNBILLED_USAGE);

and

$subscription->terminateWithoutRefund(Recurly_Subscription::NO_CHARGE_FOR_UNBILLED_USAGE);

I'm open to suggestions on the naming of these class constants should we go this route.

The nice thing about option number 2 is that it leaves it up to the programmer as to whether they want to use our class constants or not, but the validation of what's being passed in is still super simple.

Oh, and there's always the secret option number 3, which is just forget about the 3 existing terminateAnd... methods, and make the terminate() method public, allowing people to pass in what they like as the refundType and charge variables. But this feels like moving backwards.

[drewish]

Your point about secret option 3 actually hints at another option, we could just make that part of the function signatures: terminateAndPartialRefundIgnoreUnbilledUsage... that could get too crazy but is an option.

[aaron-junot]

Well, the benefit of doing that would be that we wouldn't break anything for anyone, no one would have mysterious unreadable booleans being passed into the method, and people have to explicitly choose to go against the default...

I don't think including those would be any crazier than option number 2, but I like that it keeps with the pattern we already had.

Shall I go ahead and change over to that pattern?

[bhelx]

I'd prefer keeping it simple and leaving it as a simple boolean parameter. The documentation should suffice for the small percentage of people who do not want to use the default of true. If we believe we may expand beyond this one parameter then we might want to consider another option.

[bhelx]

👍