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

[Core][Payment] Populate gateway choices based on payum configuration #5442

Conversation

Zales0123
Copy link
Member

Q A
Bug fix? no
New feature? no
BC breaks? yes
Related tickets
License MIT

*/
public function __construct(array $gateways)
public function __construct(RegistryInterface $gatewaysRegistry)
Copy link
Contributor

Choose a reason for hiding this comment

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

@makasim
Copy link
Contributor

makasim commented Jul 6, 2016

Pay attention to not all payum gateway could be used out of the box, some of them may require a special converter action.

@pjedrzejewski pjedrzejewski added Enhancement Minor issues and PRs improving the current solutions (optimizations, typo fixes, etc.). BC Break PRs introducing BC breaks (do not even try to merge). labels Jul 6, 2016
@@ -23,7 +23,8 @@
"php": "^5.6|^7.0",

"sylius/registry": "^0.19",
"sylius/resource": "^0.19"
"sylius/resource": "^0.19",
"payum/payum": "~1.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

payum/core would be enough

@makasim
Copy link
Contributor

makasim commented Jul 6, 2016

Not sure sylius payment component must have a dependency on payum/core. So maybe we can introduce a new form type, payum specific. one can use it if both sylius/payment and payum/core are installed. The sylius/sylius could use payum specific by default

It would be also better from BC point of view.

@pjedrzejewski ping

@pjedrzejewski
Copy link
Member

Definitely should not depend on Payum/Core. So I'd introduce a gateway choice type, that is replaced/populated with appropriate choices in PayumBundle. Ideally, entire glue between Sylius and Payum is in SyliusPayumBundle. And it should not be that hard, because only problem now is in the new Pay / After Pay actions on OrderController in Core. See #5415. Moving that to PayumBundle down the road should not be a problem.

@Zales0123 Zales0123 force-pushed the populate-gateway-choices-based-on-payum-configuration branch from f4dbd33 to 7295370 Compare July 6, 2016 12:24
@Zales0123
Copy link
Member Author

Ok, so finally we end up with something much simpler 😄 We remove gateways configuration from sylius_payment and prepend payum gateways with extension. Form type is left as it was, as well as parameter injected in it. @pjedrzejewski @makasim what say you? ;)

return;
}

$config = $container->getExtensionConfig('payum');
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not really accurate since some gateways could be added via container tag, or even stored in database.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure they can. However this is just a prepend, gateways configuration can be easily overwritten in yml file, or in extension, or in any other possible way.

@pjedrzejewski pjedrzejewski merged commit 0076f8c into Sylius:master Jul 7, 2016
@pjedrzejewski
Copy link
Member

Thanks Mateusz! 👍

@Zales0123 Zales0123 deleted the populate-gateway-choices-based-on-payum-configuration branch October 6, 2016 20:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BC Break PRs introducing BC breaks (do not even try to merge). Enhancement Minor issues and PRs improving the current solutions (optimizations, typo fixes, etc.).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants