-
Notifications
You must be signed in to change notification settings - Fork 850
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix card source expiry date update #393
Conversation
LGTM |
@@ -37,6 +37,7 @@ public static function init() | |||
'address', | |||
'address_kana', | |||
'address_kanji', | |||
'card', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this PR is a breaking change because it replace Stripe\Card
to Stripe\AttachedObject
object.
Skipped: https://github.com/stripe/stripe-php/blob/master/lib/Util/Util.php#L81
Try to var_dump($source->card)
to see the result.
cc: @ob-stripe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The card
hash under source objects does not have an "object" => "card"
entry (because it's not a card object), so convertToStripeObject()
would not convert those hashes to \Stripe\Card
objects but just to a generic \Stripe\Object
.
You're correct that this patch did cause the type to be changed from \Stripe\Object
to \Stripe\AttachedObject
, which might be seen as a breaking change, but I have a hard time thinking of a practical case where that would break anything. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try to add this test in TokenTest
class to see the issue:
/** @test */
public function it_can_create_card_token()
{
$token = self::createCardToken();
$this->assertInstanceOf(Token::class, $token);
$this->assertSame('card', $token->type);
$this->assertInstanceOf(Card::class, $token->card);
$this->assertSame('Visa', $token->card->brand);
}
/**
* Create a card Token for tests
*
* @return Token
*/
private static function createCardToken()
{
return Token::create([
'card' => [
'number' => '4242424242424242',
'exp_month' => date('n'),
'exp_year' => date('Y') + 1,
'cvc' => '314',
]
]);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, indeed, the card
hash nested in token objects is an actual card object, and this patch does cause the regression you described.
I'll try to push a fix shortly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would make your new test pass, but cause the test I added in this PR to fail (i.e. it would make it impossible to update expiry dates on card sources).
r? @dpetrovics-stripe (dev-platform run)
cc @stripe/api-libraries
Fixes an issue where the library failed to update the expiry date when updating a card source.