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

ob-stripe · 2017-11-30T15:38:05Z

r? @deontologician-stripe (dev-platform run, feel free to reassign)
cc @stripe/api-libraries @arcanedev-maroc

This fixes the issue described by @arcanedev-maroc here.

Basically, in #393 I added card to the list of nested updatable attributes so that users can update the expiry date on card sources:

$source = \Stripe\Source::create(array(
  "type" => "card",
  "token" => "tok_visa",
));
$source->card->exp_month = 12;
$source->card->exp_month = 2022;
$source->save();

This worked, but had the side-effect of turning all nested card hashes into \Stripe\AttachedObject. That's fine for sources since the card hash is just a hash, but not for token objects since the card hash nested in token objects is an actual card object, and so the instance should be a \Stripe\Card (even though I have trouble imagining a practical case where that would be an issue -- there's no reason I can think of to manipulate a card object via token rather than via a customer/recipient/account).

This PR changes the way the $nestedUpdatableAttributes works so that each class can define its own nested updatable attributes. StripeObject now uses late static bindings so that derived classes can override the getNestedUpdatableAttributes() static function (which replaces the $nestedUpdatableAttributes static variable) to define its own list of nested updatable attributes.

remi-stripe

While I understand the approach, I don't think this is the best direction. We're keeping a whitelist (which is bad in itself) but making it easier to mess up/mix.

I understand why this works but I also think the root cause is not worth this fix. No one can save a card object from a token. This is not a supported operation. It might work because the card id in the Token is the same as the one on the Charge though I don't think we guarantee this anywhere.

remi-stripe · 2017-11-30T15:44:47Z

lib/StripeObject.php

+                'metadata',
+                'owner',
+                'payout_schedule',
+                'personal_address',


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.

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.

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.

remi-stripe · 2017-11-30T15:45:38Z

lib/Source.php

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


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

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.

I understand but it feels wrong/confusing.

deontologician-stripe · 2017-12-01T22:03:42Z

I'm not familiar with this binding and my php is pretty rusty, is there someone better who could review this?

ob-stripe · 2017-12-07T18:43:23Z

@brandur-stripe Can you take a look at this and share your thoughts?

brandur-stripe

Nice work tackling this one OB!

This is probably a feeling that every one of us is getting, but it seems that these "updatable attribute" semantics in PHP are turning out to be a pretty abhorrent abstraction.

That said, I can't think of a better way to fix this without a major overhaul and this PR will end up making things slightly better if it gives us the ability to offload some attributes from StripeObject to Account, so +1!

brandur-stripe · 2017-12-07T19:03:31Z

lib/StripeObject.php

+        static $permanentAttributes = null;
+        if ($permanentAttributes === null) {
+            $permanentAttributes = new Util\Set(array('_opts', 'id'));
+        }


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 :/

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

brandur-stripe · 2017-12-07T19:06:00Z

lib/StripeObject.php

+                'metadata',
+                'owner',
+                'payout_schedule',
+                'personal_address',


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.

brandur-stripe · 2017-12-07T19:07:15Z

lib/Source.php

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


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.

… Source objects

ob-stripe · 2017-12-15T10:03:56Z

So I swore I posted an update on this issue, but it apparently got lost during last week's madness.

My approach does not work as well as I had hoped. What I said here is incorrect:

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.

This only works for top-level attributes. E.g. for Account, this would work for legal_entity but not for additional_owners and the 0, 1, etc. indexes. This is because the legal_entity hash will be cast as an AttachedObject, and its own nested attributes will then use AttachedObject's whitelist rather than Account's.

We could merge the PR as-is in order to fix the card issue, but I'm not too enthusiastic about it. I'm going to try rewriting the deserialization logic to see if we can get rid of the whitelist altogether.

ob-stripe · 2018-01-08T23:53:19Z

Closing this one in favor of #404.

stripe-ci assigned deontologician-stripe Nov 30, 2017

remi-stripe reviewed Nov 30, 2017

View reviewed changes

arcanedev-maroc mentioned this pull request Nov 30, 2017

Fix card source expiry date update #393

Merged

ob-stripe force-pushed the ob-fix-nested-card-object branch from 3cd874b to 9c981a0 Compare November 30, 2017 16:05

ob-stripe assigned brandur-stripe and unassigned deontologician-stripe Dec 7, 2017

brandur-stripe reviewed Dec 7, 2017

View reviewed changes

Ensure that card is only treated as a nested updatable attribute on…

3461f16

… Source objects

ob-stripe force-pushed the ob-fix-nested-card-object branch from 9c981a0 to 3461f16 Compare December 7, 2017 21:05

ob-stripe mentioned this pull request Dec 15, 2017

Always return Stripe\Card when calling Stripe\Token->card #402

Closed

ob-stripe mentioned this pull request Dec 20, 2017

Fix parameter serialization logic #404

Merged

ob-stripe closed this Jan 8, 2018

ob-stripe deleted the ob-fix-nested-card-object branch January 8, 2018 23:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ob-stripe commented Nov 30, 2017 •

edited

Loading

remi-stripe left a comment

remi-stripe Nov 30, 2017

ob-stripe Nov 30, 2017

brandur-stripe Dec 7, 2017

remi-stripe Nov 30, 2017

ob-stripe Nov 30, 2017

remi-stripe Nov 30, 2017

deontologician-stripe commented Dec 1, 2017

ob-stripe commented Dec 7, 2017

brandur-stripe left a comment

brandur-stripe Dec 7, 2017 •

edited

Loading

ob-stripe Dec 7, 2017

brandur-stripe Dec 7, 2017

brandur-stripe Dec 7, 2017

ob-stripe Dec 7, 2017

ob-stripe commented Dec 15, 2017

ob-stripe commented Jan 8, 2018

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

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

Conversation

ob-stripe commented Nov 30, 2017 • edited Loading

remi-stripe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deontologician-stripe commented Dec 1, 2017

ob-stripe commented Dec 7, 2017

brandur-stripe left a comment

Choose a reason for hiding this comment

brandur-stripe Dec 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ob-stripe commented Dec 15, 2017

ob-stripe commented Jan 8, 2018

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

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

ob-stripe commented Nov 30, 2017 •

edited

Loading

brandur-stripe Dec 7, 2017 •

edited

Loading