Skip to content
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.

Sign up for GitHub

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

[WIP] Ensure that card is only treated as a nested updatable attribute on Source objects #396

Closed
wants to merge 1 commit into from
Closed

[WIP] Ensure that card is only treated as a nested updatable attribute on Source objects #396

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions lib/Source.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,20 @@
*/
class Source extends ApiResource
{
/**
* @return Util\Set Attributes that are nested but still updatable from
* the parent class's URL (e.g. metadata).
*/
public static function getNestedUpdatableAttributes()
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we add a comment somewhere in here that gives a breakdown similar to the one in this PR's description? I feel like this is a pretty difficult-to-understand complexity of the PHP library and I worry that the reasons we have card in here will be forgotten pretty quickly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

{
static $nestedUpdatableAttributes = null;
if ($nestedUpdatableAttributes === null) {
$nestedUpdatableAttributes = StripeObject::getNestedUpdatableAttributes();
$nestedUpdatableAttributes->add('card');
Copy link
Contributor

Choose a reason for hiding this comment

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

I worry that this will make it even harder to not miss edge-case. We already "break" the API often by forgetting to add those new API params to the whitelist. Now we need to add those but to potentially multiple objects now instead of just one canonical (though hidden) place in the library

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What this PR does is give the option to override the global whitelist in each class. If we're afraid that splitting up the whitelist will make it harder to maintain, we can simply keep the PR in its current form, i.e. keep the global whitelist on StripeObject and keep the one special case of card under Source.

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand but it feels wrong/confusing.

}
return $nestedUpdatableAttributes;
}

/**
* @param array|string $id The ID of the source to retrieve, or an options
* array containing an `id` key.
Expand Down
88 changes: 47 additions & 41 deletions lib/StripeObject.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,45 +13,53 @@
class StripeObject implements ArrayAccess, JsonSerializable
{
/**
* @var Util\Set Attributes that should not be sent to the API because
* @return Util\Set Attributes that should not be sent to the API because
* they're not updatable (e.g. API key, ID).
*/
public static $permanentAttributes;
public static function getPermanentAttributes()
{
static $permanentAttributes = null;
if ($permanentAttributes === null) {
$permanentAttributes = new Util\Set(array('_opts', 'id'));
}
Copy link
Contributor

@brandur-stripe brandur-stripe Dec 7, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Oh man. I was going to say that it would be nice to just move this up to a statically initialized expression so we don't have to check the null everytime, but just reading through the manual it seems like this only became supported in 5.6 for any non-trivial (i.e. not an integer or string assignment) expression :/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep. I amended the commit so you can't see it, but my first submission used static initializations 😭

return $permanentAttributes;
}

/**
* @var Util\Set Attributes that are nested but still updatable from
* @return Util\Set Attributes that are nested but still updatable from
* the parent class's URL (e.g. metadata).
*/
public static $nestedUpdatableAttributes;

public static function init()
public static function getNestedUpdatableAttributes()
{
self::$permanentAttributes = new Util\Set(array('_opts', 'id'));
self::$nestedUpdatableAttributes = new Util\Set(array(
// Numbers are in place for indexes in an `additional_owners` array.
//
// There's a maximum allowed additional owners of 3, but leave the
// 4th so errors work properly.
0, 1, 2, 3, 4,

'additional_owners',
'address',
'address_kana',
'address_kanji',
'card',
'dob',
'inventory',
'legal_entity',
'metadata',
'owner',
'payout_schedule',
'personal_address',
'personal_address_kana',
'personal_address_kanji',
'shipping',
'tos_acceptance',
'transfer_schedule',
'verification',
));
static $nestedUpdatableAttributes = null;
if ($nestedUpdatableAttributes === null) {
$nestedUpdatableAttributes = new Util\Set(array(
// Numbers are in place for indexes in an `additional_owners` array.
//
// There's a maximum allowed additional owners of 3, but leave the
// 4th so errors work properly.
0, 1, 2, 3, 4,

'additional_owners',
'address',
'address_kana',
'address_kanji',
'dob',
'inventory',
'legal_entity',
'metadata',
'owner',
'payout_schedule',
'personal_address',
Copy link
Contributor

Choose a reason for hiding this comment

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

This lives under legal_entity on the Account object and legal_entity is not a real class/object in the SDK (nor the API). This means you can't define a custom getNestedUpdatableAttributes and make it work for some of those params. Would the plan be to "leave" it on StripeObject directly? If so, won't most people pushing PR just default to putting it here anyway? Seems hard to realize there are edge-cases when you do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The attributes defined here don't have to be top-level attributes in a given object, they can themselves be nested under other attributes (else the current code would not work either). So my intent was to move all of the identity verification attributes under Account, but as said above we can also keep the attributes in StripeObject by default and only override the global whitelist when needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd be bullish in moving these under something like Account now that class-specific rules are supported. It's always seemed pretty janky to me that things like 0, 1, 2, 3, 4 and additional_owners sit at the top level.

'personal_address_kana',
'personal_address_kanji',
'shipping',
'tos_acceptance',
'transfer_schedule',
'verification',
));
}
return $nestedUpdatableAttributes;
}

/**
Expand Down Expand Up @@ -112,14 +120,14 @@ public function __set($k, $v)
);
}

if (self::$nestedUpdatableAttributes->includes($k)
if (static::getNestedUpdatableAttributes()->includes($k)
&& isset($this->$k) && $this->$k instanceof AttachedObject && is_array($v)) {
$this->$k->replaceWith($v);
} else {
// TODO: may want to clear from $_transientValues (Won't be user-visible).
$this->_values[$k] = $v;
}
if (!self::$permanentAttributes->includes($k)) {
if (!static::getPermanentAttributes()->includes($k)) {
$this->_unsavedValues->add($k);
}
}
Expand Down Expand Up @@ -223,19 +231,19 @@ public function refreshFrom($values, $opts, $partial = false)
}

foreach ($removed as $k) {
if (self::$permanentAttributes->includes($k)) {
if (static::getPermanentAttributes()->includes($k)) {
continue;
}

unset($this->$k);
}

foreach ($values as $k => $v) {
if (self::$permanentAttributes->includes($k) && isset($this[$k])) {
if (static::getPermanentAttributes()->includes($k) && isset($this[$k])) {
continue;
}

if (self::$nestedUpdatableAttributes->includes($k) && is_array($v)) {
if (static::getNestedUpdatableAttributes()->includes($k) && is_array($v)) {
$this->_values[$k] = AttachedObject::constructFrom($v, $opts);
} else {
$this->_values[$k] = Util\Util::convertToStripeObject($v, $opts);
Expand Down Expand Up @@ -265,7 +273,7 @@ public function serializeParameters()
}

// Get nested updates.
foreach (self::$nestedUpdatableAttributes->toArray() as $property) {
foreach (static::getNestedUpdatableAttributes()->toArray() as $property) {
if (isset($this->$property)) {
if ($this->$property instanceof StripeObject) {
$serialized = $this->$property->serializeParameters();
Expand Down Expand Up @@ -308,5 +316,3 @@ public function __toArray($recursive = false)
}
}
}

StripeObject::init();
1 change: 0 additions & 1 deletion tests/StripeObjectTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,6 @@ public function testJsonEncode()

public function testReplaceNewNestedUpdatable()
{
StripeObject::init(); // Populate the $nestedUpdatableAttributes Set
$s = new StripeObject();

$s->metadata = array('bar');
Expand Down
17 changes: 17 additions & 0 deletions tests/TokenTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,4 +10,21 @@ public function testUrls()
$token = new Token('abcd/efgh');
$this->assertSame($token->instanceUrl(), '/v1/tokens/abcd%2Fefgh');
}

public function testNestedCardObject()
{
$token = Token::constructFrom(
array(
'id' => 'tok_foo',
'object' => 'token',
'card' => array(
'id' => 'card_foo',
'object' => 'card',
),
),
new Util\RequestOptions()
);
$this->assertSame("Stripe\\Token", get_class($token));
$this->assertSame("Stripe\\Card", get_class($token->card));
}
}