Get rid of ExternalAccount #417
Conversation
| $msg = "Bank accounts cannot be accessed without a customer ID or account ID."; | ||
| throw new Error\InvalidRequest($msg, null); | ||
| } | ||
| $parentExtn = urlencode(Util\Util::utf8($parent)); |
There was a problem hiding this comment.
It feels weird to encode so much knowledge about the URL structure in this object, but this is similar to, stripe-ruby, for example:
https://github.com/stripe/stripe-ruby/blob/master/lib/stripe/bank_account.rb
On the one hand, this leads to duplicated logic; on the other hand, it simplifies this object's dependencies and leads to more explicit tests.
|
Great implementation (with nice test additions!). I think others would have a better idea as to whether this is a desirable change or not, but the reduction in special casing seems like a good idea to me in concept at least. +1. |
lib/Account.php
Outdated
| @@ -120,7 +120,7 @@ public function deauthorize($clientId = null, $opts = null) | |||
| * @param array|null $params | |||
| * @param array|string|null $opts | |||
| * | |||
| * @return ExternalAccount | |||
| * @return ApiResource | |||
| */ | |||
| public static function createExternalAccount($id, $params = null, $opts = null) | |||
There was a problem hiding this comment.
Should we remove those methods explicitly in favor of $account->external_accounts->create() instead? This avoids the confusion and we would automatically return the right class based on the type of external accounts
There was a problem hiding this comment.
No, these methods let users manipulate external accounts without having to retrieve the account first (like other nested resource methods). Also, to clarify, these methods will return the correct type -- though of course we cannot know what this type will be in the PHPDoc.
There was a problem hiding this comment.
Actually, I've updated the PHPDoc to use BankAccount|Card instead of ApiResource.
lib/AlipayAccount.php
Outdated
| @@ -7,7 +7,60 @@ | |||
| * | |||
| * @package Stripe | |||
| */ | |||
| class AlipayAccount extends ExternalAccount | |||
| class AlipayAccount extends ApiResource | |||
There was a problem hiding this comment.
Where are we on the idea that we should remove those classes entirely since we don't support them anymore and have not for over a year? Since PHP is dynamic and would return an ApiResource anyway when you get one of those on a Charge for example, is it time to get rid of it at the same time?
There was a problem hiding this comment.
I think we can keep the classes around -- the definitions do still exist in the OpenAPI spec, and in the case of Bitcoin some users do still send requests to the top-level endpoint /v1/bitcoin/receivers.
I've removed the refund() method from BitcoinReceiver though, and updated the PHPDoc to include links to the sources API documentation.
lib/Card.php
Outdated
| public static function retrieve($_id, $_opts = null) | ||
| { | ||
| $msg = "Cards cannot be accessed without a customer, recipient or account ID. " . | ||
| "Retrieve a bank account using \$customer->sources->retrieve('card_id'), " . |
ob-stripe
force-pushed
the
ob-drop-externalaccount
branch
2 times, most recently
from
76ad07d
to
c6f5857
Compare
ob-stripe
force-pushed
the
ob-drop-externalaccount
branch
from
c6f5857
to
21381d6
Compare
|
Okay, since the consensus seems to be in favor, I'm going to pull this in the integration branch for v6. |
r? @brandur-stripe
cc @stripe/api-libraries
Okay, I'm not 100% sure we actually want this change, but since we're going to release a new major version I thought I'd open the PR to gather feedback.
This PR removes the
ExternalAccountclass entirely. The existing implementation has a number of flaws:instanceUrl()method that assumes every external account can be attached to a customer, an account or a recipientverify()method that assumes every external account can be verifiedBankAccounthas its ownverify()methodSourcedoes not derive fromExternalAccountThis change makes the implementation closer to what we have in stripe-ruby and stripe-python: all "external accounts" (namely: cards, bank accounts, sources, Alipay accounts and Bitcoin receivers) derive directly from
APIResourceand have their owninstanceUrl()method that knows which type of owner each resource can have (e.g. bank accounts cannot be attached to recipients, etc.), plus customretrieve()andupdate()methods that return an helpful error message.