Correct URL used for refunding application fees

[brandur]

This uses the new endpoint instead of the deprecated one.

Related to stripe/stripe-php#208.

/cc @russelldavies It seems like you have a fair bit of context on this one.
Would you mind taking a look? Thanks!

[russelldavis]

lg! This is a breaking change, so we should do a major version bump if we're going w/ semantic versioning. (Works well since the other changes since the last release are also breaking.)

[brandur]

@russelldavis Thanks! And +1 on the major bump.

[bkrausz]

This seems different than how we usually do things: we don't usually provide a convenience method to do sub-object creation, instead deferring to the ability to call .refunds.create.

For example, Charge#refund still calls the deprecated refund endpoint.

[kyleconroy]

Yeah, going to agree with @bkrausz here. If we want to do anything, we should just remove the refund method entirely.

[russelldavis]

We definitely shouldn't keep the call to the deprecated endpoint. I think removing the refund method entirely or keeping this as a convenience method (with the changes I mention at #354) are both options to consider. It seems like charge.refund would be a nice/intuitive convenience method for charge.refunds.create (though I don't feel strongly). Any particular reasons against the convenience method?

Regardless of whether a convenience method exists, should we update the charge (like I mention in #354) when calling charge.refunds.create?

[bkrausz]

I think it would be much clearer to remove the method, since changing how it works would be more likely to break upgrades in a subtle way.

Does this mean we should do a major-version pass to remove/cleanup all of our deprecated singular endpoints (Charge#refund, Transfer#cancel, ApplicationFee#refund, etc)?

[russelldavis]

I think it would be much clearer to remove the method, since changing how it works would be more likely to break upgrades in a subtle way.

True. On the other hand, if we kept the method around, it would actually turn this into a non-breaking change for any integrations that aren't using the return value of the method (if we do what I propose in #354; for example, charge.refund; puts "refunded #{charge.amount_refunded}" would continue to work). I suspect a large number of integrations would be in that camp. That alone might make it worth keeping this around.

Does this mean we should do a major-version pass to remove/cleanup all of our deprecated singular endpoints (Charge#refund, Transfer#cancel, ApplicationFee#refund, etc)?

Agreed, either way we should make a major-version change consistently across all those endpoints.

[brandur]

Thanks for the great discussion here!

Summary: agreed that using refunds.create is a better path here, but I disagree (or agree with Russell ;) that we should axe the old interface and bump the major version when producing a backward compatible implementation helper is so trivial do. I've updated the patch in #354 with this new approach. Let me know what you think.

Deprecations are annoying for users, but maybe even worse, act as a discouragement to upgrading which is something that we should try to avoid here. Of course let's make sure to be open to deprecation when necessary, but it doesn't seem worth it for something so comparatively minor (I'm still endeavoring to have #313 fixed for the next one). Also, I didn't add it, but am open to the idea of possibly marking that helper as obsolete so that we can consider removing it for the next major version bump (if anyone feels particularly strongly about that).

Does this mean we should do a major-version pass to remove/cleanup all of our deprecated singular endpoints (Charge#refund, Transfer#cancel, ApplicationFee#refund, etc)?

+1 making the rounds through these!

/cc @remistr

[kyleconroy]

A little history around charge.refund. It was never a convenience method; at one point it was the only way to refund a charge. The /v1/charges/ch_xxx/refunds endpoint didn't exist, so charge.refund made sense. Once the refunds endpoint existed, we changed all the documentation to point to charge.refunds.create as the correct way create refunds and deprecated calling .refund.

Deprecations are annoying for users, but maybe even worse, act as a discouragement to upgrading which is something that we should try to avoid here.

This is still going to be a breaking change, and one that may be more subtle that just removing the methods. We're also going to now have two official ways for refunding a charge, canceling a transfer, etc.

[brandur]

A little history around charge.refund. It was never a convenience method; at one point it was the only way to refund a charge. The /v1/charges/ch_xxx/refunds endpoint didn't exist, so charge.refund made sense. Once the refunds endpoint existed, we changed all the documentation to point to charge.refunds.create as the correct way create refunds and deprecated calling .refund.

Oh, yes, Thank-you! I think someone filled me in off-thread.

This is still going to be a breaking change, and one that may be more subtle that just removing the methods. We're also going to now have two official ways for refunding a charge, canceling a transfer, etc.

Can you elaborate on why you think it would be a breaking change?

[kyleconroy]

For some methods, changing the endpoint will change the behavior, mainly around returned value. In this case, /refund used to return the application_fee, not the refund. For charges, we'd then have to expand the charge on the returned refund.

[brandur]

For some methods, changing the endpoint will change the behavior, mainly around returned value. In this case, /refund used to return the application_fee, not the refund. For charges, we'd then have to expand the charge on the returned refund.

Oh shoot, I should have been more clear on this, but can you check out the new diff in #354? Essentially the proposal here is to use the new path, but to rebuild the current resource after the call is issued (application fee in this case) so that our API stays compatible. We'll then update our documentation to use .refunds.create, leaving .refund in a state of "supported, but obsolete".

[kyleconroy]

My FUD has been misplaced. I missed the new diff, which does the right thing.

Does this mean we should do a major-version pass to remove/cleanup all of our deprecated singular endpoints (Charge#refund, Transfer#cancel, ApplicationFee#refund, etc)?

We still need to be careful about upgrading these methods to use the new endpoints, as it is still possible that behavior differs between the new and old endpoint. However, since I don't have a concrete example, feel free to ignore.

[brandur]

We still need to be careful about upgrading these methods to use the new endpoints, as it is still possible that behavior differs between the new and old endpoint. However, since I don't have a concrete example, feel free to ignore.

+1! If/when tackling them, let's make sure to pull in people who would be aware of any subtle inconsistencies of that sort. Thanks!