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: +1.6 kB (0%) Total Size: 1.37 MB
ℹ️ View Unchanged
|
| use Base_Payment_Method; | ||
| use BNPL_Payment_Method; | ||
| use Payment_Method_Icons; |
There was a problem hiding this comment.
I'd try to avoid traits TBH, they tripped us up before. Although that's my personal preference.
With traits, it's not always clear where or even if a method is implemented or overridden.
If DRY is the goal, and there's some utility function that is needed in multiple places and is pure (i.e. no side effects or DB writes or HTTP calls) then perhaps it could be put in a static Util class rather than hidden in a trait?
And if the function is not pure, then we need to decide what this class does - we probably shouldn't mix a definition with hardcoded data and a service that manipulates said data.
| * | ||
| * @return array<string,array<string,array{min:int,max:int}>> | ||
| */ | ||
| public function get_limits_per_currency(): array; |
There was a problem hiding this comment.
I must admit that my knowledge of payment methods fails me here, but is this a feature of BNPL only?
What if we moved this to the base Payment Method definition, but allowed to define no limit? For example, a returned limit could be a nullable int, where null means that there's no limit?
| * @param string $country The country code. | ||
| * @return bool | ||
| */ | ||
| public function is_amount_within_limits( int $amount, string $currency, string $country ): bool { |
There was a problem hiding this comment.
Seems like this function is not being used, but I don't think the definition class should be responsible for calculating things.
The hypothetical calling code could do something like $amount > $definition->get_minimum_amount( ... ) && $amount < $definition->get_maximum_amount( ... ) and also check if the currency/country is even applicable before calling this.
| * | ||
| * @return array<string,array<string,array{min:int,max:int}>> Array of currency limits indexed by currency code and country code. | ||
| */ | ||
| abstract public function get_limits_per_currency(): array; |
There was a problem hiding this comment.
I wonder if this even needs to be exposed outside of the definition classes, in line with the above comments.
If we had a single interface, with get_min/max_limit methods, then we could let it to whoever works on future definitions to define the internal representation of the limits, if any.
| 'capabilities' => $definition->get_capabilities(), | ||
| 'currencies' => $definition->get_supported_currencies(), | ||
| 'countries' => $definition->get_supported_countries(), | ||
| 'icons' => $definition->get_relative_icon_paths(), |
There was a problem hiding this comment.
Another con for traits. The method get_all_payment_method_definitions returns interface instances, but the interface doesn't specify get_relative_icon_paths as a part of it. So we're already expecting that all definitions will use the icons trait. PHP will allow it, but hopefully you can imagine how quickly things will get out of hand if there's even one method that breaks this approach.
In short, we should decide that a method "shape" is defined by one interface and stick to that. What happens in the implementations of that interface is up to those implementations, not shared traits.
| /** | ||
| * Class implementing the Affirm payment method definition. | ||
| */ | ||
| class Affirm implements BNPLPaymentMethodDefinition { |
There was a problem hiding this comment.
Similar to above, AffirmDefinition will be a bit clearer.
| $this->accept_only_domestic_payment = true; | ||
| $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 ]; | ||
| $this->definition = new AffirmDefinition(); |
There was a problem hiding this comment.
We could make all the definition methods static and we wouldn't have to do the awkward initialization here. Although this will work too.
| $this->is_reusable = in_array( Payment_Method_Capability::TOKENIZATION, $capabilities, true ); | ||
| $this->is_bnpl = in_array( Payment_Method_Capability::BUY_NOW_PAY_LATER, $capabilities, true ); |
There was a problem hiding this comment.
Seems like these is_ methods could be a part of the definition interface.
| $this->icon_url = $icons['default']['path']; | ||
| $this->dark_icon_url = $icons['dark']['path']; |
There was a problem hiding this comment.
Rather than returning all icons in get_icons and forcing the underlying implementation to adhere to a poorly defined array shape, we could make these into direct getters too.
| public function get_stripe_id(): string { | ||
| return $this->get_id() . '_payments'; | ||
| } |
There was a problem hiding this comment.
We could make this method and get_id static and get rid of the Payment_Method constants. The methods could return hardcoded strings.
No need to force constants if we make sure that the methods are used consistently instead.
This is 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