POC - Centralized payment method definitions #10217
Conversation
… with the new centralized payment definitions.
…payment method ID.
Test the buildOption 1. Jetpack Beta
Option 2. Jurassic Ninja - available for logged-in A12s🚀 Launch a JN site with this branch 🚀 ℹ️ Install this Tampermonkey script to get more options. Build info:
Note: the build is updated when a new commit is pushed to this PR. |
|
Size Change: +713 kB (+55%) 🆘 Total Size: 2 MB
ℹ️ View Unchanged
|
includes/payment-methods/Configs/Interfaces/BNPLPaymentMethodDefinition.php
Outdated
Show resolved
Hide resolved
includes/payment-methods/Configs/Traits/BNPL_Payment_Method.php
Outdated
Show resolved
Hide resolved
includes/payment-methods/Configs/Traits/BNPL_Payment_Method.php
Outdated
Show resolved
Hide resolved
includes/payment-methods/Configs/scripts/generate-payment-method-configs.php
Outdated
Show resolved
Hide resolved
includes/payment-methods/Configs/Traits/Base_Payment_Method.php
Outdated
Show resolved
Hide resolved
This reverts commit 70ec9bd.
I also got rid of the payment method class constants that were referring to another constant to get the ID. That required changes in a few other places to address errors. I added a new get_keywords() method to be able to implement these into the duplicate detection service.
…method "stripe_id"
…nd the build process
…y others we may need in the future.
…tion and move that functionality to a util class since it should largely be the same for all payment method definitions
…yment-method-definitions
|
@marcinbot I really appreciate you reviewing this! My apologies for it being on my back burner for a little while, but I've made several updates based on your feedback:
I still need to go through and add tests where appropriate for the new functionality (hence this is still a draft), but wanted to get your eyes on these architectural changes first to make sure I'm heading in the right direction. Could you please take another look when you have a chance? Thanks! |
| $this->limits_per_currency = WC_Payments_Utils::get_bnpl_limits_per_currency( self::PAYMENT_METHOD_STRIPE_ID ); | ||
| $this->countries = [ Country_Code::UNITED_STATES, Country_Code::CANADA ]; | ||
|
|
||
| $capabilities = AffirmDefinition::get_capabilities(); |
There was a problem hiding this comment.
With definitions in place now, we can have one UPE_Payment_Method class whose constructor could look like this:
__construct( $token_service, string $payment_method_definition_class ) {
$this->definition = $payment_method_definition_class;
$capabilities = $this->definition::get_capabilities();
// other initialization goes here
}
Then, whatever instantiates the above, could just instantiate multiple instances of UPE_Payment_Method but with different definitions passed in.
I think there's even a way to make Psalm enforce that the definition implements the interface like (note that this would require a phpcs rule to be disabled to allow the class-name as type):
/**
* @template T of PaymentMethodDefinitionInterface
*
* @param class-name<T> $payment_method_definition_class
*/
So we'd basically end up with one UPE_Payment_Method class and multiple definitions. This hinges on one of my questions below of whether we can move all the methods to this approach, but even if not, we can add some conditions here, like making payment_method_definition_class nullable and allowing inheriting classes to do their own thing for methods without a definition class.
There was a problem hiding this comment.
Yes, I had the thought that once all payment methods are converted to use definitions there was a potential for even more simplification. I can look at this a bit more to see what that would mean for payment methods that need additional logic for methods like get_countries(). We could put that in the definition, but I think we were trying to keep them pretty lean.
There was a problem hiding this comment.
We could put that in the definition, but I think we were trying to keep them pretty lean.
We are, but, depending on the case, we could either hardcode stuff as plain data or maybe bend the leanness a bit. I'm looking at the existing Klarna logic for countries and it doesn't seem too bad to keep in the definition if need be.
|
|
||
| export type PaymentMethodCapabilityType = typeof PaymentMethodCapability[ keyof typeof PaymentMethodCapability ]; | ||
|
|
||
| export const PaymentMethodDefinitions: PaymentMethodConfigurations = { |
There was a problem hiding this comment.
Since PHP enforces the values, and PaymentMethodDefinition interface enforces the shape of the objects, perhaps there's no need for this?
It would be really good to avoid duplicating this code and especially auto-generating stuff, which is a lesson learned the hard way in the V2 server.
There was a problem hiding this comment.
I think that exported type was accidentally left in the generation script. It's not being used anywhere so I'll remove it.
It would be really good to avoid duplicating this code and especially auto-generating stuff, which is a lesson learned the hard way in the V2 server.
Are you advocating for not automatically generating the client-side types based on the PHP code?
There was a problem hiding this comment.
Are you advocating for not automatically generating the client-side types based on the PHP code?
If there's a pre-existing mechanism to do that, then we can continue. But we should also keep it on a simpler side - there should be a way to feed any class to such a generator which would output a type.
I'm specifically not sure about declaring constants and complex types as constants (PaymentMethodDefinitions here) - it feels like there might be a more intuitive way of passing these from PHP to JS without resorting to "compiling" the code (maybe just serialize an object to JSON?). It would come at a cost of performance but could be easier to maintain.
| /** | ||
| * Generates payment method configurations for the frontend. | ||
| */ | ||
| class GeneratePaymentMethodConfigsCommand { |
There was a problem hiding this comment.
I think another argument against this is that we could end up accidentally shipping this.
It's not the end of the world if we do, and we can address it by moving this class to the cli folder, but still.
There was a problem hiding this comment.
I think another argument against this is that we could end up accidentally shipping this.
I'm happy to do whatever you think is best in this regard.
| /** | ||
| * Internal dependencies | ||
| */ | ||
| import type { PaymentMethodMapEntry } from '../payment-methods-map'; |
There was a problem hiding this comment.
I'm not exactly sure how this is compiling (maybe because it's just importing the TS type?), but there seem to be a circular dependency, here. client/payment-methods/generated-map.ts is importing client/payment-methods-map.tsx. But client/payment-methods-map.tsx is also importing client/payment-methods/generated-map.ts.
Maybe PaymentMethodMapEntry should be separated in a different file.
There was a problem hiding this comment.
Thanks, good catch! Resolved this in a16c8f3
There was a problem hiding this comment.
The approach you have here is really good and smart - I bet it wasn't easy to create the abstraction to support the use cases we have now.
I think you proved the viability pretty well by having Affirm/Afterpay/Klarna.
Tomorrow I'll take a closer look at better understanding the build steps and the complexity/extendability needs in case we need more use cases for the other "special" payment methods (like Alipay and the different supported currencies at checkout, which are based on the account's country).
Great job so far!
This reverts commit dccdc2d.
There was an issue with `require.context` not being available in Jest since it's a webpack thing. We're now mocking the CreatePaymentMethodIconComponent instead.
This PR introduces a centralized system for defining payment methods in WooCommerce Payments, significantly reducing the complexity and maintenance burden of implementing new payment methods. Previously, adding or modifying payment methods required changes across multiple files with duplicated logic. This new architecture provides a single source of truth for payment method definitions.
This PR was born out of a POC for centralizing payment method definitions to reduce the amount of changes that need to be made while implementing (or removing support for) a new payment method. It's resulting from #10098
Once I've added the appropriate unit tests, I'll mark the PR as ready but, given it's size I thought it'd be good to get an initial review.