Removing legacy payment method classes #4787
Conversation
wjrosa
changed the title
wjrosa
requested review from
a team,
daledupreez and
malithsen
and removed request for
a team
daledupreez
left a comment
daledupreez
left a comment
There was a problem hiding this comment.
I have some initial comments from inspection, and I will move to test this later this morning. That said, it feels like it might be worth splitting this PR up a bit so we can tackle more specific and focused testing.
| * @var string | ||
| */ | ||
| private $legacy_sepa_gateway_id = WC_Gateway_Stripe_Sepa::ID; | ||
| private $legacy_sepa_gateway_id = 'stripe_sepa'; |
There was a problem hiding this comment.
Might it be worth using a new constant for this rather than having 'stripe_sepa' everywhere?
It also feels like it could be worth pushing just that change into a dedicated PR, as that would be easy to inspect and review, and we could get it shipped with minimal risk. That would also reduce the size of this PR by quite a few files.
Co-authored-by: daledupreez <[email protected]>
Co-authored-by: daledupreez <[email protected]>
Co-authored-by: daledupreez <[email protected]>
Co-authored-by: daledupreez <[email protected]>
Co-authored-by: daledupreez <[email protected]>
Co-authored-by: daledupreez <[email protected]>
Co-authored-by: daledupreez <[email protected]>
Co-authored-by: daledupreez <[email protected]>
Co-authored-by: daledupreez <[email protected]>
@malithsen yes. I have some other branches to push related to code cleaning up. I will open other PRs in the next couple of weeks
@daledupreez I tried to make this PR focused on just removing the payment method classes, to avoid it getting too big or risky. Which parts do you think would be better to move to another PR? |
|
For splitting the PR, I was thinking it might make sense to change the references to |
|
Oh right. I created the other PR here: #4802 |
daledupreez
left a comment
daledupreez
left a comment
There was a problem hiding this comment.
While I would love to remove all of this code, I am really worried about unforeseen consequences of removing so much code without looking deeply into some of the nuances of what we're changing and/or removing.
| * | ||
| * @since 4.0.0 | ||
| */ | ||
| class WC_Gateway_Stripe_Alipay extends WC_Stripe_Payment_Gateway { |
There was a problem hiding this comment.
These classes were not marked as deprecated. Just to be good citizens, I think we should deprecate them before removing them, as merchants may be referencing them in custom code.
| protected function enable_upe( $settings ) { | ||
| $settings['upe_checkout_experience_accepted_payments'] = []; | ||
|
|
||
| $payment_gateways = WC_Stripe_Helper::get_legacy_payment_methods(); |
There was a problem hiding this comment.
Can we safely remove this logic? What will happen to merchants upgrading from older versions where UPE was not enabled yet?
| ]; | ||
| } | ||
|
|
||
| public function test_turning_on_upe_enables_the_correct_upe_methods_based_on_which_legacy_payment_methods_were_enabled() { |
There was a problem hiding this comment.
Don't we still need the underlying logic for upgrades?
wjrosa
added
the
status: blocked
wjrosa
changed the title
wjrosa
changed the title
wjrosa
removed
the
status: blocked
wjrosa
changed the base branch from
develop
to
dev/removing-deprecated-legacy-checkout-settings-methods
Changes proposed in this Pull Request:
In this simple cleanup PR, I am removing all the legacy checkout payment method classes, as we are no longer using them.
Testing instructions
Code review. Check if the tests are still passing. Perform some basic smoke testing and confirm no regression was introduced.
Changelog entry
Changelog Entry Comment
Comment
Post merge