Skip to content

Commit

Permalink
Fix pinning for stripeclient
Browse files Browse the repository at this point in the history
  • Loading branch information
richardm-stripe committed Aug 18, 2023
1 parent 8860217 commit f845b4c
Show file tree
Hide file tree
Showing 2 changed files with 19 additions and 2 deletions.
3 changes: 1 addition & 2 deletions lib/BaseStripeClient.php
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,7 @@ class BaseStripeClient implements StripeClientInterface, StripeStreamingClientIn
'api_key' => null,
'client_id' => null,
'stripe_account' => null,
// Note, even if null, ApiRequestor will default this to Stripe::$apiVersion
'stripe_version' => null,
'stripe_version' => \Stripe\Util\ApiVersion::CURRENT,
'api_base' => self::DEFAULT_API_BASE,
'connect_base' => self::DEFAULT_CONNECT_BASE,
'files_base' => self::DEFAULT_FILES_BASE,
Expand Down
18 changes: 18 additions & 0 deletions tests/Stripe/BaseStripeClientTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
*/
final class BaseStripeClientTest extends \Stripe\TestCase
{
use TestHelper;
/** @var \ReflectionProperty */
private $optsReflector;

Expand Down Expand Up @@ -202,4 +203,21 @@ public function testRequestWithOptsInParamsWarns()
static::assertNotNull($charge);
static::assertSame('acct_456', $this->optsReflector->getValue($charge)->headers['Stripe-Account']);
}

public function testRequestWithNoVersionDefaultsToPinnedVersion()
{
$client = new BaseStripeClient([
'api_key' => 'sk_test_client',
'api_base' => MOCK_URL,
]);
$this->expectsRequest("get", "/v1/charges/ch_123", null, [
"Stripe-Version: " . \Stripe\Util\ApiVersion::CURRENT,
]);
$charge = $client->request(
'get',
'/v1/charges/ch_123',
[],
[]
);
}
}

2 comments on commit f845b4c

@ankurk91
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bug fix could go to v11.0.1, there was no need to tag v12

@remi-stripe
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ankurk91 It's kind of a big breaking change to do this, even if it's what we originally intended. And there were already a lot of integrations relying on this new major so doing another major was safer and more in line with semver rules

Please sign in to comment.