Skip to content

[3.0]: Abstract Customer object #80

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 4 commits into from
Feb 13, 2016
Merged

[3.0]: Abstract Customer object #80

merged 4 commits into from
Feb 13, 2016

Conversation

greydnls
Copy link
Contributor

@greydnls greydnls commented Feb 9, 2016

Two main things here:

  1. Abstract Parameter manipulation. This is duplicated all over the place and it bugs me. We can make use of traits now, so we should.
  2. Customer object. This is something I've wanted to do for a long time. This will allow us to start implementing Bank Account features in 3.0 as well as making the Credit Card object much cleaner. This is my first stab at it. I'm happy to hear feedback on any of this.

@greydnls greydnls changed the title Enhancement: Abstract Customer object [3.0]: Abstract Customer object Feb 9, 2016
@greydnls greydnls added this to the V3 milestone Feb 9, 2016

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@judgej
Copy link
Member

judgej commented Feb 9, 2016

It is good to see this split up. Just a few thoughts:

  • Does the customer and address need to be bound together still? For example, does every address have to have a first name and last name (e.g. a company wound't)? Does every customer have to have an address (I suspect not, when paying by card token). Not sure the best thing to do here, but the fact that you cannot setName() without a space in it, because it automatically gets split into first and last name, can be an issue at times.
  • The CreditCard is just one example of a more generic payment type. For example, Stripe and Sage Pay Integration map a credit card onto a temporary token, and it is the credit card token that is used to initiate the payment. Similarly, longer-lasting saved card tokens could also be used to pay. These all feel like specific instances of a more general payment type (probably a better word) that should probably share a common interface.

@barryvdh
Copy link
Member

I think a Customer + CreditCard is good. A Customer object doesn't need to have all parameters set, but it's probably good to keep them together.

CreditCard is indeed pretty generic for the 'old style' CreditCard gateways, but don't think it's bad. The tokens from Stripe etc are all different right? So not sure if a Interface is needed, or just agree to use the same parameter key for that (eg. token?)

So can we still use the array definition in the Request for Customer?

$request->purchase([
 'customer' => [
     'name'= 'Barry',
  ]
]);

@@ -29,6 +29,7 @@
"require": {
"php": ">=5.6",
"alcohol/iso4217": "^3.1",
"beberlei/assert": "^2.4",
Copy link
Member

Choose a reason for hiding this comment

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

Are you actually using this library? Don't see it used in this commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, no, I need to remove that. Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

Updated!

@greydnls
Copy link
Contributor Author

Would it be worth it to break an Address value object out of the customer? I could do that.

The idea behind abstracting out the credit card was to allow for bank account transfer and/or other payment methods. I could see the usefulness of possibly having a PaymentMethodInterface for them to adhere to, but I'd need to look at it more closely.

@barryvdh
Copy link
Member

I guess it would probably make sense to have an Address object, instead of duplicate parameters for billing/shipping.

@judgej
Copy link
Member

judgej commented Feb 10, 2016

Some gateways have more addresses than just billing and shipping, even. Obviously that would be gateway specific, but to have an address object to throw in would be immensely useful. Again - and I keep wittering on about this - interfaces are so key to flexibility down the line. Whatever we think needs to be in an address now, another gateway will want something additional, and so a user of OmniPay may need to use their own address class, which may be a simple extension of the OmniPay Address class (so long as everything isn't marked as final) or one of their own that meets the minimum contract.

* Set Card Title.
*
* @param string $value Parameter value
* @return CreditCard provides a fluent interface.

Choose a reason for hiding this comment

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

Wrong phpdoc, probably a copy/paste issue (in many places)

Copy link
Member

Choose a reason for hiding this comment

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

Should be fixed, replaced them with $this

barryvdh added a commit that referenced this pull request Feb 13, 2016
@barryvdh barryvdh merged commit da2addb into 3.0 Feb 13, 2016
@barryvdh barryvdh deleted the customerObject branch February 13, 2016 14:37
@barryvdh
Copy link
Member

Does the Customer object still belong in the card, or should we just make it a standalone option, and nothing in the CreditCard?

@judgej
Copy link
Member

judgej commented Feb 14, 2016

The CreditCard class has always felt like it has too much in it. The addresses should be taken out to separate classes for a start, because there is just too much duplication there. It would also allow for more flexibility in what goes into the address object. For example, some gateways may want an ISO country code - some 2 digit and some 3 digit - and others want the full country name. They could be separate fields and the gateway would take what it wants. As it is at the moment (v2.0) I have a system that uses multiple gateways and I need to feed different details into the address depending on what gateway is being used, which makes it a little less abstract than it could be.

Each PostalAddress could also have a Person - their name details - or perhaps a more generic "Entity" which could be a person or an organisation. The person or entity should also be given to the CreditCard as a part of its authentication details.

@judgej
Copy link
Member

judgej commented Feb 14, 2016

Or maybe a person has a postal address, rather than the other way around. But there are instances where an API could want any one of a person, postal address or credit card detail, without any of the others.

@barryvdh
Copy link
Member

Yeah I agree, but we shouldn't make it to difficult for people. Right now, it's easy to just fill in 1 array with all the users details + creditcard. I agree that Customer should be seperate from the Creditard. The CC is a very specific set. We have te be aware of certain duplications, for example, the Name. Is it the creditcard owner name the same as the Billing name? Should it be inserted twice, or should it be inherited from the customer? An Address also has a name etc, so do you have to set it on the Customer or the BillingAddress etc?

It's important to note that we primarily just provide a means to map data to the gateway. And some gateways just want the name to show in the logs, others want 2 address to provide shipping or something. We shouldn't make it too hard to understand..

This was referenced May 18, 2018
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants