ach_credit_transfer property

[nickdnk]

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]

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]

@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]

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]

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]

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]

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]

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]

That should do it.

[remi-stripe]

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]

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

[nickdnk]

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.