Skip to content
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

Fatal error when upgrading to a version with different constructor arguments #4438

Closed
shendy-a8c opened this issue Jul 14, 2022 · 7 comments
Closed
Labels
category: core WC Payments core related issues, where it’s obvious. category: engineering For product engineering, architecture work, tech debt and so on. component: disputes Issues related to Disputes focus: disputes type: bug The issue is a confirmed bug.

Comments

@shendy-a8c
Copy link
Contributor

Describe the bug

This bug is discovered when we are requiring new argument for WC_REST_Payments_Disputes_Controller.

When upgrading to a version that requires 2nd argument, it throws a fatal error as if the 2nd parameter isn't passed.

The fatal error is not too harmless. It's only showing and might alert merchants.

To Reproduce

  1. Install and activate WCPay.
  2. Onboard so store is connected to a Stripe account. I see that if store isn't onboarded yet, I don't see the fatal error.
  3. Modify WC_REST_Payments_Disputes_Controller's constructor to accept a new argument and pass that argument from class-wc-payments.php.
  4. Run npm run build on that modified code and install + activate the generated woocommerce-payments.zip.

Actual behavior

A fatal error of too few argument will be thrown.

Screenshots

Expected behavior

No fatal error.

Additional context

I laid out a more lengthy explanation here. tl;dr; looks like the older version of class-wc-payments.php is executing while class-wc-rest-payments-disputes-controller.php is updated with newer version within the same runtime.

I think this reported problem is caused by this same problem due to this change.

@shendy-a8c shendy-a8c added type: bug The issue is a confirmed bug. needs triage Manually put issue into triage process. labels Jul 14, 2022
@htdat htdat added needs prioritisation Triage finished and issues are ready for the following processing. category: engineering For product engineering, architecture work, tech debt and so on. category: core WC Payments core related issues, where it’s obvious. and removed needs triage Manually put issue into triage process. labels Jul 18, 2022
@htdat
Copy link
Member

htdat commented Jul 18, 2022

Sounds like this paJDYF-43G-p2 is relevant.

@shendy-a8c
Copy link
Contributor Author

Thank you for pointing that out, @htdat. That link is relevant in a way that when a breaking change (like a new constructor parameter) is introduced we have to consider other parties who might depend on it because they might not be aware and the change will break their code.

What surprised me with this issue is that WCPay itself will break on upgrade. The solution might be to avoid breaking changes, ie by making the new constructor argument optional, even though that is not ideal and a bit unclean.

@leonardola
Copy link
Contributor

Seems related to #7464 and #7446

@haszari haszari added the focus: misc or unknown Issues that need to be added to a focus area (aka "needs focus"). label Mar 11, 2024
@vbelolapotkov vbelolapotkov added component: disputes Issues related to Disputes focus: disputes and removed focus: misc or unknown Issues that need to be added to a focus area (aka "needs focus"). labels Mar 13, 2024
@vbelolapotkov
Copy link
Collaborator

@haszari sending this one to your team since the problem is coming from Disputes.

@vbelolapotkov vbelolapotkov removed the needs prioritisation Triage finished and issues are ready for the following processing. label Mar 13, 2024
@haszari
Copy link
Contributor

haszari commented Mar 13, 2024

Thanks @vbelolapotkov – added to Helix's board for prioritisation.

@jessy-p
Copy link
Contributor

jessy-p commented Mar 14, 2024

Not specific to disputes, same as #7464
More information here. paJDYF-aZa-p2

@haszari
Copy link
Contributor

haszari commented Mar 25, 2024

Thanks @jessy-p.

More information here. paJDYF-aZa-p2

Summarising this for clarity. Reminder – P2 links aren't readable and we need to get the relevant context on the issue.

  • During update, WooPayments code is running
  • The code is being updated as it runs
  • This leads to errors when constructor args change, etc

Closing as a duplicate of #7464

If anyone has more info here, and there's something for Helix to action, please comment & reopen or log a new issue.

@haszari haszari closed this as not planned Won't fix, can't repro, duplicate, stale Mar 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: core WC Payments core related issues, where it’s obvious. category: engineering For product engineering, architecture work, tech debt and so on. component: disputes Issues related to Disputes focus: disputes type: bug The issue is a confirmed bug.
Projects
None yet
Development

No branches or pull requests

7 participants