[Payment] Adding PaymentFactory #4214
Conversation
lchrusciel
commented
Feb 18, 2016
lchrusciel
commented
| Q | A |
|---|---|
| Bug fix? | yes |
| New feature? | no |
| BC breaks? | yes |
| Deprecations? | no |
| License | MIT |
michalmarcinkowski
added
Awaiting Review
Enhancement
|
@lchrusciel Can we also remove the dispatcher from the listener? It's never used. |
|
Please take a look at the failing build, otherwise looks good to me. In a separate PR we should definitely figure out a better name than PaymentProcessor, it's more like OrderPaymentsUpdater, but still far from perfect. :/ |
| $paymentFactoryDefinition = $container->getDefinition('sylius.factory.payment'); | ||
| $paymentFactoryClass = $paymentFactoryDefinition->getClass(); | ||
| $decoratedPaymentFactoryDefinition = new Definition($paymentFactoryClass); | ||
| $decoratedPaymentFactoryDefinition->setArguments([$factoryDefinition]); |
There was a problem hiding this comment.
How about squeezing this to one line:
$decoratedPaymentFactoryDefinition = new Definition($paymentFactoryClass, [$factoryDefinition]);?
lchrusciel
force-pushed
the
introduce-payment-factory
branch
3 times, most recently
from
dfebfc4
to
c1a3592
Compare
lchrusciel
force-pushed
the
introduce-payment-factory
branch
from
b59e0b2
to
4903d01
Compare
[Payment] Adding PaymentFactory
|
Thank you Łukasz! 👍 |
| @@ -43,15 +38,11 @@ public function __construct(FactoryInterface $paymentFactory) | |||
| /** | |||
| * {@inheritdoc} | |||
| */ | |||
| public function createPayment(OrderInterface $order) | |||
| public function processOrderPayments(OrderInterface $order) | |||
There was a problem hiding this comment.
Hi, I know this was merged a while ago, but I'm just working through an update...
Is this class (or this method) actually doing any processing?
It seems a bit pointless and could go into the Factory (or a Core extension/decoration of the Factory), because the naming is confusing now. There's no processing of order payments going on here that I can see - just creating them.
There was a problem hiding this comment.
also the name implies multiple payments but this method only creates a single new payment for the total of an order. Could be something like PaymentFactory::createForOrderTotal() ?
|
@peteward Yeah, the naming is unfortunate, but we want to have it separated from payment factory because we want to allow custom implementations, which may create multiple payments or do whatever they want. That's why we have this extra service. That being said, the naming and this as a whole must be improved, I agree. |
|
Understood, perhaps |
[Payment] Adding PaymentFactory
