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

ach_credit_transfer property #603

Merged
merged 1 commit into from
Feb 26, 2019
Merged

ach_credit_transfer property #603

merged 1 commit into from
Feb 26, 2019

Conversation

nickdnk
Copy link
Contributor

@nickdnk nickdnk commented Feb 26, 2019

Hello

On my pull request from yesterday I noticed I had erroneously included the ach_credit_transfer property on the Source class as it was the example used in the docs, however, this particular property is dynamic and only present if the type of the object is ach_credit_transfer.

If we are to include ach_credit_transfer, we should include all the possible enumerations of type. I can do that as well. What do you think?

I apologise for overlooking this.

https://stripe.com/docs/api/sources/object

@nickdnk
Copy link
Contributor Author

nickdnk commented Feb 26, 2019

Perhaps we could do something like this:

 * existing properties...
 * @property string $usage
 *
 * As the `type` property determines which object is associated with a source, one of the following
 * properties exist on the object if (and only if) the `type` is equal to any of these property names.
 * For example; if $source->type === 'card', then $source->card will be available as a \Stripe\Card.
 *
 * @property mixed $ach_credit_transfer
 * @property mixed $ach_debit
 * @property mixed $alipay
 * @property mixed $bancontact
 * @property Card $card
 * @property mixed $card_present
 * @property mixed $card_present_single_message
 * @property mixed $eps
 * @property mixed $giropay
 * @property mixed $ideal
 * @property mixed $multibanco
 * @property mixed $p24
 * @property mixed $paper_check
 * @property mixed $sepa_credit_transfer
 * @property mixed $sepa_debit
 * @property mixed $sofort
 * @property ThreeDSecure $three_d_secure
 * @property mixed $wechat
 *
 * @package Stripe

@remi-stripe ? :)

I've seen a bit of confusion in the IRC channel regarding this particular object structure, so perhaps PHPDocs could help make this clearer.

@remi-stripe
Copy link
Contributor

@nickdnk I think it's fine to leave as it. There are a lot of properties/hashes that are not always set/present.

I'm not sure what the alternative gives us since in the IDE it would still show each one separately right? I'm fine adding each one if you're up for it, but without the ones that are not public yet:

 * @property mixed $ach_credit_transfer
 * @property mixed $ach_debit
 * @property mixed $alipay
 * @property mixed $bancontact
 * @property mixed $card
 * @property mixed $card_present
 * @property mixed $eps
 * @property mixed $giropay
 * @property mixed $ideal
 * @property mixed $multibanco
 * @property mixed $p24
 * @property mixed $sepa_debit
 * @property mixed $sofort
 * @property mixed $three_d_secure
 * @property mixed $wechat

Also the thee_d_secure one is not an instance of TheeDSecure and card is not an instance of Card. Those are completely separate objects.

@nickdnk
Copy link
Contributor Author

nickdnk commented Feb 26, 2019

I only added the explanation for clarity, since it's atypical for your objects to have a property that defines another existing property directly.

How is a card hash not an instance of Card? Can you explain? I was under the impression that the SDK always constructs the actual objects based on their object property (if it knows it and has a matching entity) exactly for this reason.

I'm well aware that the auto-complete in IDEs will always show all properties, but I think that's better than them saying "this property does not exist" because all of them are missing by design as of now.

Edit: To clarify, we already do this with the discount property on the Invoice class, for instance.

@remi-stripe
Copy link
Contributor

I only added the explanation for clarity, since it's atypical for your objets to have a property that defines another existing property directly.

Sure but that would not be visible to any dev that doesn't go and read the header of the class right?

How is a card hash not an instance of Card? Can you explain? I was under the impression that the SDK always constructs the actual objects based on their object property (if it knows it and has a matching entity) property exactly for this reason.

It does, but you're misunderstanding how this resource works. Cards are the historical object and look like this:

{
  id: "card_123",
  object: "card",
  last4: "4242",
  brand: "Visa",
  fingerprint: "RanDoM"
}

The Source object is an abstraction other multiple payment types. And for a Card Source it looks like this:

{
  id: "src_123",
  object: "source",
  type: "card",
  card: {
    last4: "4242",
    brand: "Visa",
    fingerprint: "RanDoM"
  }
}

The nested card hash is not an API resource. It's just a raw hash, nothing else. It doesn't have the object field. The resource is the Source.

I'm well aware that the auto-complete in IDEs will always show all properties, but I think that's better than them saying "this property does not exist" because all of them are missing by design as of now.

You were worried that adding ach_credit_transfer was incorrect because it was sometimes null and would be confusing. That's all I was addressing. I'm fine adding all of them!

@nickdnk
Copy link
Contributor Author

nickdnk commented Feb 26, 2019

I only added the explanation for clarity, since it's atypical for your objets to have a property that defines another existing property directly.

Sure but that would not be visible to any dev that doesn't go and read the header of the class right?

Of course, but I think it makes sense to make this slightly odd logic available in as many places as possible to make it easier on devs.

How is a card hash not an instance of Card? Can you explain? I was under the impression that the SDK always constructs the actual objects based on their object property (if it knows it and has a matching entity) property exactly for this reason.

It does, but you're misunderstanding how this resource works. Cards are the historical object and look like this:

{
  id: "card_123",
  object: "card",
  last4: "4242",
  brand: "Visa",
  fingerprint: "RanDoM"
}

The Source object is an abstraction other multiple payment types. And for a Card Source it looks like this:

{
  id: "src_123",
  object: "source",
  type: "card",
  card: {
    last4: "4242",
    brand: "Visa",
    fingerprint: "RanDoM"
  }
}

The nested card hash is not an API resource. It's just a raw hash, nothing else. It doesn't have the object field. The resource is the Source.

Cool. mixed it is then :)

I'm well aware that the auto-complete in IDEs will always show all properties, but I think that's better than them saying "this property does not exist" because all of them are missing by design as of now.

You were worried that adding ach_credit_transfer was incorrect because it was sometimes null and would be confusing. That's all I was addressing. I'm fine adding all of them!

I was only worried about adding ach_credit_transfer if that's the only one of the varying properties we add, since that's just inconsistent.

I'll add all of them as mixed and update my PR:

Shall I include them at the bottom after the proposed explanation, or do you want them alphabetically mixed with all the others?

@remi-stripe
Copy link
Contributor

I would recommend keeping it alphabetized for now, I think it's cleaner and we can just update this if we hear feedback from devs that it's confusing.

There's a world in the future where stripe-php will be auto-generated and we can easily add comments for all the properties!

@nickdnk
Copy link
Contributor Author

nickdnk commented Feb 26, 2019

I would recommend keeping it alphabetized for now, I think it's cleaner and we can just update this if we hear feedback from devs that it's confusing.

Ok!

There's a world in the future where stripe-php will be auto-generated and we can easily add comments for all the properties!

And here I was hoping for a job as the official phpdoc maintainer 😄

@nickdnk
Copy link
Contributor Author

nickdnk commented Feb 26, 2019

That should do it.

@remi-stripe
Copy link
Contributor

And here I was hoping for a job as the official phpdoc maintainer 😄

Don't worry that future is far away I thiiiiiink. Also you can always start helping us maintain stripe-go and stripe-dotnet :p

@remi-stripe remi-stripe merged commit 480a5db into stripe:master Feb 26, 2019
@remi-stripe
Copy link
Contributor

Thanks for all the hardwork as usual @nickdnk! Released as 6.30.3

@nickdnk
Copy link
Contributor Author

nickdnk commented Feb 26, 2019

You're welcome.

I struggled a bit with trying to get the proper exceptions included in phpdocs for all objects, but it seems to be an impossible feat because of the way the library is built with traits. I have a lot of silenced Exception warnings because of this, unfortunately. As you can imagine I use phpdocs extensively in my project.

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.

3 participants