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?
src/Gateway/OldGatewayBuilder.php
Outdated
| 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
# Conflicts: # src/Payment/MollieObject.php # tests/php/Functional/Payment/OrderRequestStrategyTest.php
Fix file names as per PSR-4
…-api' into task/PIWOO-473-refactor-payments-api
this is a middle step before removing the class
| $className = 'Mollie\\WooCommerce\\PaymentMethods\\PaymentFieldsStrategies\\' . ucfirst($method->getProperty('id')) . 'FieldsStrategy'; | ||
| $paymentFieldsStrategy = class_exists($className) ? new $className( | ||
| $deprecatedGateway, | ||
| $gateway->get_description(), | ||
| $dataService | ||
| ) : new DefaultFieldsStrategy($deprecatedGateway, $gateway->get_description(), $dataService); | ||
| $issuers = $paymentFieldsStrategy->getFieldMarkup($deprecatedGateway, $dataService); |
There was a problem hiding this comment.
I can't find piece with this dynamic classname generation. It is not very flexible because it limits our naming options and forces all possible classes to come from a single namespace - and as a result, fom one single folder in our plugin.
If we would maintain a classmap in an external service like this:
$fieldStrategy = [
'billie' => BillieFieldsStrategy::class,
'riverty' => RivertyFieldsStrategy::class,
// more...
];then we could just check if an entry exists in the array and we would no longer be tied to a single PSR-4 location: Strategies could be supplied by other modules. At the same time, we'd no longer be forced to follow a single naming convention.
src/Gateway/inc/services.php
Outdated
| $className = 'Mollie\\WooCommerce\\PaymentMethods\\PaymentFieldsStrategies\\' . ucfirst($paymentMethod->getProperty('id')) . 'FieldsStrategy'; | ||
| return class_exists($className) ? new $className($deprecatedGatewayHelper, $gatewayDescription, $dataHelper) : new DefaultFieldsStrategy($deprecatedGatewayHelper, $gatewayDescription, $dataHelper); | ||
| } |
There was a problem hiding this comment.
Hm, we're even duplicating code here. A factory working off the aforementioned classmap/dictionary could be reused here.
| $middlewareClass = 'Mollie\\WooCommerce\\Payment\\Request\\Middleware' . $field; | ||
| if (class_exists($middlewareClass)) { | ||
| $middleware = $this->container->get($middlewareClass); |
There was a problem hiding this comment.
Is this doing anything? Looks it can only match MiddlewareHandler, but that does not feel intentional. Maybe I just don't understand what this is doing and how it works
| return array_reduce( | ||
| array_reverse($middlewares), | ||
| static function ($next, $middleware) { | ||
| return static function ($requestData, $order, $context) use ($middleware, $next) { | ||
| return $middleware($requestData, $order, $context, $next); | ||
| }; | ||
| }, | ||
| static function ($requestData) { | ||
| return $requestData; | ||
| } | ||
| ); |
There was a problem hiding this comment.
Does psalm accept this? I feel this could really use some more typehints
# Conflicts: # composer.lock # src/Assets/AssetsModule.php # src/Gateway/GatewayModule.php # src/Gateway/MolliePaymentGatewayHandler.php # src/Payment/MollieObject.php
we need new tests for the generic gateway
# Conflicts: # mollie-payments-for-woocommerce.php # src/Settings/Page/Section/PaymentMethods.php
Handles PIWOO-473
It also adds the payment-gateway library and refactors part of the code base into new classes and folders