Task/piwoo 473 refactor payments api #969
Conversation
The SubscriptionModule was missing
| @@ -47,7 +50,7 @@ public function maybeDisableMealVoucherGateway(?array $gateways): array | |||
| } | |||
| $mealVoucherGatewayIndex = false; | |||
| foreach ($gateways as $key => $gateway) { | |||
| if (!($gateway instanceof MolliePaymentGateway)) { | |||
| if (!($gateway instanceof PaymentGateway)) { | |||
There was a problem hiding this comment.
I believe it would be wiser to match against a list of $gateway->id here.
The instanceof check might cause trouble if mollie is used among another gateway that uses our library.
| @@ -25,7 +25,7 @@ | |||
| use WC_Payment_Gateway; | |||
| use WP_Error; | |||
|
|
|||
| class MolliePaymentGateway extends WC_Payment_Gateway implements MolliePaymentGatewayI | |||
| class MolliePaymentGatewayHandler extends WC_Payment_Gateway implements MolliePaymentGatewayI | |||
There was a problem hiding this comment.
From what I gather, you no longer need to extend WC_Payment_Gateway here, right? This class is being reduced to "a bucket of utility functions" that are in the process of being refactored to a more fitting place...?
In other words, this is no longer acting as a payment gateway by itself..?
Just for my understanding :)
| $amount, | ||
| $reason | ||
| ); | ||
| return $this->refundProcessor->process_refund($order_id, $amount, $reason); |
There was a problem hiding this comment.
$this->refundProcessor is never set and RefundProcessorInterface does not have a process_refund method, but a refundOrderPayment.
It also seems to contradict my earlier question about whether this is now a utility class (if it actively carries out refunds instead of aiding with it I mean)
Is this dead code?
| use Psr\Container\ContainerInterface; | ||
| use Psr\Log\LoggerInterface as Logger; | ||
|
|
||
| class OldGatewayBuilder |
There was a problem hiding this comment.
Can you explain how this interacts with the payment gateway library?
It is used by the gateway.instances service (as opposed to the payment_gateways service).
If I am still correct in my assumption that MolliePaymentGatewayI is mostly defining a "common toolbox until everything is refactored", then it might make sense to avoid confusion in the service/classnames.
Perhaps a __deprecated.gateway_helpers => fn() => new DeprecatedGatewayHelper()` would work wonders giving clarity about the nature of things and the intent behind them.
In two years, it might be hard to dissect that this is a middle step during refactoring.
| @@ -26,7 +27,7 @@ | |||
| use Mollie\WooCommerce\PaymentMethods\Constants; | |||
| use WC_Order; | |||
|
|
|||
| class MollieSubscriptionGateway extends MolliePaymentGateway | |||
| class MollieSubscriptionGatewayHandler extends MolliePaymentGatewayHandler | |||
There was a problem hiding this comment.
This looks a little dangerous to me
This class implements WC_Payment_Gateway and sets up required hooks for $this->scheduled_subscription_payment() and others.
Some of these contain calls to the parent:: method, for example process_subscription_payment and process_payment
As a consequence, these methods do not go through the service locator and the API of the payment gateway library, but rather through the (assumed deprecated) old implementation. Am I seeing this correctly?
Maybe this is all dead code (the WC_Payment_Gateway methods I mean) that you haven't removed yet, though. Removing this in the next step would help me see clearer.
now uses callbacks to container objects namespaces were fixed dependencies fixed
Handles PIWOO-473
It also adds the payment-gateway library and refactors part of the code base into new classes and folders