-
Notifications
You must be signed in to change notification settings - Fork 847
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 parameter serialization logic #404
Conversation
Sorry for the delay on this one OB. I've done a partial review, and will get to the rest of it today (I think). |
Since this is a breaking change, I've updated the base branch to be |
23d3b88
to
d6d1645
Compare
I rebased the branch on |
d6d1645
to
b97f04d
Compare
0b320c6
to
9f10791
Compare
b97f04d
to
a8265ad
Compare
9f10791
to
f1d8593
Compare
a8265ad
to
ba0d1d3
Compare
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.
Incredible Olivier!! Another great example of an improvement that never would have been done without you.
I left a couple comments on the code, but it's all pretty minor. 🚢
lib/ApiResource.php
Outdated
private static $HEADERS_TO_PERSIST = array('Stripe-Account' => true, 'Stripe-Version' => true); | ||
/** | ||
* @return Stripe\Util\Set A list of fields that can be their own type of | ||
* API resource (say a nested card under an account fore xample), and if |
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.
Minor typo: "fore xample".
@@ -173,4 +185,45 @@ public static function createLoginLink($id, $params = null, $opts = null) | |||
{ | |||
return self::_createNestedResource($id, static::PATH_LOGIN_LINKS, $params, $opts); | |||
} | |||
|
|||
public function serializeParameters($force = false) |
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.
Just OOC, is this mostly copied from the Ruby implementation? The test suite looks like it's pretty good in terms of vetting the new code, but just wondering how much we should expect to have to worry about it.
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.
Yep, it's basically a straight port from the Ruby implementation.
* individually on their own endpoints, but there are certain cases, | ||
* replacing a customer's source for example, where this is allowed. | ||
*/ | ||
public $saveWithParent = false; |
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.
Somewhat too bad this is getting copied across — it felt a little janky as it was coming into Ruby and I was secretly hoping that there was a better way that I hadn't thought of :) It's okay though. At least similar implementations will make easier to reason about it if you're familiar with the other.
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.
Yeah... I might have been cargo culting a little bit too much in this PR but I couldn't come up with anything better and I like that the implementations are consistent across languages for the reason you exposed.
lib/ApiResource.php
Outdated
* @param array|null|mixed $params The list of parameters to validate | ||
* | ||
* @throws Stripe\Error\Api if $params exists and is not an array | ||
*/ |
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.
Thanks for filling in this documentation!
lib/ApiResource.php
Outdated
@@ -206,7 +301,7 @@ protected function _delete($params = null, $options = null) | |||
* @param array|null $params | |||
* @param array|string|null $options | |||
* | |||
* @return StripeObject | |||
* @return Stripe\StripeObject |
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.
Nice fixes too ...
lib/StripeObject.php
Outdated
// | ||
// Another example is that on save API calls it's sometimes desirable to | ||
// update a customer's default source by setting a new card (or other) | ||
// object with `#source=` and then saving the customer. The |
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 #
designation for instance variables is fairly Ruby specific. Is there a more conventional way for PHP? (Maybe just .
.)
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 used ->source =
. I'm not sure how conventional it is but I think it's fairly explicit.
lib/StripeObject.php
Outdated
// Another example is that on save API calls it's sometimes desirable to | ||
// update a customer's default source by setting a new card (or other) | ||
// object with `#source=` and then saving the customer. The | ||
// `#save_with_parent` flag to override the default behavior allows us to |
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.
save_with_parent
-> saveWithParent
@@ -179,4 +179,92 @@ public function testCanCreateLoginLink() | |||
$resource = Account::createLoginLink(self::TEST_RESOURCE_ID); | |||
$this->assertInstanceOf("Stripe\\LoginLink", $resource); | |||
} | |||
|
|||
public function testSerializeNewAdditionalOwners() |
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.
Thanks for adding all these tests. Account's serialization is problematic enough that you really can't have enough test coverage of it.
tests/Stripe/StripeObjectTest.php
Outdated
$this->assertEquals($values["map"]["1"], $copyValues["map"]["1"]); | ||
$this->assertEquals($values["map"]["2"], $copyValues["map"]["2"]); | ||
} | ||
public function testDeepCopyMaintainClass() |
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.
Maybe space out these function definitions a bit (should have a newline in there).
tests/Stripe/StripeObjectTest.php
Outdated
$optsReflector->getValue($copyValues["map"]["0"]) | ||
); | ||
$this->assertEquals($values["map"]["1"], $copyValues["map"]["1"]); | ||
$this->assertEquals($values["map"]["2"], $copyValues["map"]["2"]); |
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.
Nit, but the whole section above is pretty dense. I like that we have good test coverage, but it could probably use some more whitespace for readability.
7ab25b9
to
c77b125
Compare
c77b125
to
42f1b44
Compare
LGTM. |
@brandur-stripe I've squashed the existing commits and added a new one. As part of the changes to the serialization logic, I initially duplicated the Can you take a look at the latest commit? Thanks :) |
@@ -36,10 +36,11 @@ | |||
require(dirname(__FILE__) . '/lib/Error/OAuth/UnsupportedResponseType.php'); | |||
|
|||
// API operations | |||
echo "bouh"; |
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.
Leftover from manual debugging 😨
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.
Oops. Totally missed that apparently, haha.
@@ -40,6 +40,10 @@ class Account extends ApiResource | |||
use ApiOperations\All; | |||
use ApiOperations\Create; | |||
use ApiOperations\Delete; | |||
use ApiOperations\NestedResource; | |||
use ApiOperations\Retrieve { | |||
retrieve as protected _retrieve; |
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.
Resources that have their own custom implementation of API methods but need to call the standard implementation (usually because they have some additional parameter checking) can use this syntax to "inherit" the trait methods under a different name.
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's pretty cool. Thanks for the ntoe!
Makes sense. Thanks! Alright, I had one more look over, and I think it's pretty solid. LGTM. |
r? @brandur-stripe
cc @stripe/api-libraries
This is a pretty beefy PR, sorry about that! I can probably split it up in a few smaller PRs.
Basically, this replaces the entire parameter serialization logic to make it as similar as possible to stripe-ruby's. This lets us get rid of the nested parameters whitelist (with a minor exception for
metadata
), so this PR fixes #402 and replaces #396.While I expect most users will be able to upgrade without issues, this PR does introduce some breaking changes:
AttachedObject
no longer exists. All nested objects that are not a specific API resource are nowStripeObject
s (as is the case in stripe-ruby and stripe-python).Collection
now derives fromStripeObject
instead ofApiResource
. This is also the case in stripe-ruby, and is necessary to solve a serialization quirk when replacing a list object (e.g. when replacingitems
on a subscription object)Tagged as WIP to avoid merging for now. Since this will require a major version bump, we might want to bundle some other breaking changes with it.
Let me know if you have any questions or if you want me to split the PR into smaller ones.