-
Notifications
You must be signed in to change notification settings - Fork 216
Removing legacy payment method classes #4787
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Are we planning to do a similar cleanup for the frontend code (to remove is_upe_enabled distinction) in a separate PR?
daledupreez
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
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