-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
[Payum] Add Be2bill gateway #669
Conversation
@@ -2376,12 +2376,12 @@ | |||
"version": "v1.0.4", | |||
"source": { | |||
"type": "git", | |||
"url": "https://github.com/omnipay/omnipay.git", | |||
"url": "https://github.com/adrianmacneil/omnipay.git", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is invalid, the repo was renamed.
IMO it's not good idea to have everything in main repository, it's matter of time that it will become messy. btw. @makasim any real reason why all those services are separated packages? (I mean: |
|
||
public function __construct(ContainerInterface $container) | ||
{ | ||
$this->container = $container; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could avoid container useage. since we are on symfony 2.3 we can inject request via setter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
I thought it is a good idea. Also omnipay split its repo into separate packages. To be concrete:
|
Good question. We have to maintain them if we still want to say that sylius supports payments (here the list of payment) out of the box. And it is not that hard because sylius provide only integration block and should take care of that block not the rest payment logic. |
{ | ||
foreach (array('CARDCODE', 'CARDCVV', 'CARDFULLNAME', 'CARDVALIDITYDATE') as $idx) { | ||
if (isset($paymentDetails[$idx])) { | ||
unset($paymentDetails[$idx]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need to check that a variable is set before you unset it, just unset it, it's safe.
Rebased for #636 and fixed feedbacks. I think it's a good thing to have everything in main repo for same reason. It enables us to really say that we do support this and this payment method. @makasim There is a lot of duplication between different payment method Action. Could it be possible to reduce it? |
Looks good to me
FYI: These thihgs are what I want to do to reduce code duplication:
@winzou any other suggestions? |
@makasim that's a good start, we'll see then if there is still something we can do. |
[Payum] Add Be2bill gateway
This Geometry thing is crazy. :S Thank you Alexandre, I really like to see how easy it is to add new gateway. @makasim 👍 |
Totally, that's a really good point! |
This is a new payment gateway: Be2bill.
@makasim please review =)