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.

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

Added more constants, fixed more docs, added missing objects #481

Merged
merged 1 commit into from
Jun 13, 2018
Merged

Added more constants, fixed more docs, added missing objects #481

merged 1 commit into from
Jun 13, 2018

Conversation

nickdnk
Copy link
Contributor

@nickdnk nickdnk commented May 30, 2018

Hello

  1. Added Discount, DisputeEvidence and OrderItem objects - none of these extend ApiResource since they don't have an id property which I assume is necessary for that. Anywhere these objects are referenced in phpdocs I've update those from the previously used mixed type. DisputeEvidence isn't really a class since it doesn't have an OBJECT_NAME. If you think it's silly I'll remove it from the PR. I only added it because it exists as a standalone documented object: https://stripe.com/docs/api#dispute_evidence_object. And: Are Discount and OrderItem supposed to be in the Util map?

  2. Added charge decline code constants to Charge - following the logic elsewhere in the library. I was not sure if this should be on Card or on Charge, but I think Charge, no? Or perhaps we should move them to the \Stripe\Error\Card object?

  3. Added payout failure code constants to Payout - following the logic elsewhere in the library.

  4. Indented Subscription status constants like elsewhere - minor edit.

Some things I noticed:

The Topup object is missing from the documentation, even though it links to it itself: https://stripe.com/docs/api#topup_object - it doesn't exist on the page. For this reason I can't check if the phpdocs match.

The SourceTransaction object - what is that? Why do we have it? I can't find it anywhere in the documentation. Isn't this just a normal Charge object used as a source for transactions? See https://stripe.com/docs/connect/charges-transfers#transfer-availability

And; is there somehow we can follow changes made to the API? It doesn't seem to be documented anywhere when changes to properties take place, which makes it quite tedious to go through all of the objects again and again to keep them updated. I assume this is the reason the phpdocs were outdated in the first place.

Cheers

@ob-stripe
Copy link
Contributor

Hey Nick. Awesome work as usual.

Added Discount, DisputeEvidence and OrderItem objects - none of these extend ApiResource since they don't have an id property which I assume is necessary for that. Anywhere these objects are referenced in phpdocs I've update those from the previously used mixed type. DisputeEvidence isn't really a class since it doesn't have an OBJECT_NAME. If you think it's silly I'll remove it from the PR. I only added it because it exists as a standalone documented object: https://stripe.com/docs/api#dispute_evidence_object. And: Are Discount and OrderItem supposed to be in the Util map?

So, yes, you do need to add those new objects to the Util map so the deserializer knows about them. If you don't add them, then the objects will be deserialized to default StripeObjects instances.

This also means that you should remove the DisputeEvidence class: because it doesn't have an object property, the deserializer won't know about it. (We could write custom deserialization login to always cast instances to the correct class, like we do in stripe-java, but I would really rather avoid introducing this kind of complexity in the PHP library.)

Also, you're correct that your new classes should not extend ApiResource, but they should extend StripeObject for backwards compatibility (so users can still access attributes via both attribute (->foo) and array (['foo']) syntax, etc.).

Overall though, I have some concerns about adding those new classes purely for the sake of describing the attributes with phpdoc. @brandur-stripe wdyt?

Added charge decline code constants to Charge - following the logic elsewhere in the library. I was not sure if this should be on Card or on Charge, but I think Charge, no? Or perhaps we should move them to the \Stripe\Error\Card object?

No, I think Charge is the right place for these. Sadly, non-card payments can be declined too :)

The Topup object is missing from the documentation, even though it links to it itself: https://stripe.com/docs/api#topup_object - it doesn't exist on the page. For this reason I can't check if the phpdocs match.

Topups are a private beta feature. The link is valid, but you need to be logged into an account that has been invited into the beta to be able to view it.

The SourceTransaction object - what is that? Why do we have it? I can't find it anywhere in the documentation. Isn't this just a normal Charge object used as a source for transactions? See https://stripe.com/docs/connect/charges-transfers#transfer-availability

No, SourceTransaction describes a transaction on a receiver-type source like ACH Credit Transfer: https://stripe.com/docs/sources/ach-credit-transfer#source-transactions. The object is not yet described in the API reference, but you can view an example in the link above. It might be difficult to fully describe it with phpdoc because, like Source objects, it is polymorphic depending on the source's type.

And; is there somehow we can follow changes made to the API? It doesn't seem to be documented anywhere when changes to properties take place, which makes it quite tedious to go through all of the objects again and again to keep them updated. I assume this is the reason the phpdocs were outdated in the first place.

At this time, your best bet is to look at the diffs in the OpenAPI spec: https://github.com/stripe/openapi. Ultimately we'll be able to autogenerate the phpdocs directly from the spec, but we're not quite there yet.

@nickdnk
Copy link
Contributor Author

nickdnk commented May 31, 2018

@ob-stripe

Hey

I removed the DisputeEvidence class and made the other changes you mentioned. I'll leave it up to you guys to decide if you want OrderItem and Discount as classes in the library, since they don't "do" anything. I think it's fair to have them because of the OBJECT_NAME constant - for consistency and to support phpdocs. They now extend StripeObject.

I added those classes to Util and init.php.

I'll rebase to one commit once we've agreed on everything.

@nickdnk
Copy link
Contributor Author

nickdnk commented May 31, 2018

Does this mean that all variables currently defined as mixed could/should be StripeObject ?

@nickdnk
Copy link
Contributor Author

nickdnk commented Jun 2, 2018

I've just added the invoice_pdf and hosted_invoice_url properties to Invoice - these are new :)

@nickdnk
Copy link
Contributor Author

nickdnk commented Jun 11, 2018

Added the active property to Plan

@brandur-stripe
Copy link
Contributor

Thanks for keeping this one alive and updated @nickdnk!

If you get a chance, could you squash your commits? Otherwise we'll do it for you when we bring it in (and the commit message might not be as good).

Overall though, I have some concerns about adding those new classes purely for the sake of describing the attributes with phpdoc. @brandur-stripe wdyt?

Sorry @ob-stripe, I totally missed this @ to me last time. I guess I could see it either way, but IMO since these are considered first class API objects (i.e. they have their own sections in the API reference), it's okay to have them as separate classes, so I'd +1 this as currently implemented. I'll defer to you though.

I did a quick pass on the changes otherwise too, and LGTM.

ptal @ob-stripe

@nickdnk
Copy link
Contributor Author

nickdnk commented Jun 11, 2018

@brandur-stripe Squashed as requested :)
Edit: I just noticed I forgot to change the discount property of Subscription from mixed to Discount. I just did that and rebased again.

Added charge decline code constants to Charge
Added payout failure code constants to Payout
Indented subscription constants like elsewhere
Corrected alphabetical order of constants in Payout
Updated phpdocs across the board
Copy link
Contributor

@ob-stripe ob-stripe left a comment

Choose a reason for hiding this comment

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

LGTM!

@ob-stripe ob-stripe merged commit 964c228 into stripe:master Jun 13, 2018
@ob-stripe
Copy link
Contributor

Released as 6.8.0. Thanks again @nickdnk :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants