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

Bring back the rest-user-exists-controller file #6303

Merged
merged 6 commits into from
May 15, 2023
Merged

Conversation

malithsen
Copy link
Member

@malithsen malithsen commented May 15, 2023

This fixes an error that happens during the plugin upgrade process.

class-wc-rest-user-exists-controller.php and its references were removed in #6174

During the plugin upgrade process, init_rest_api of the current WCPay version is invoked. This results in printing an error during the upgrade. This PR adds the file back to silence the error until we find a more robust solution.

Changes proposed in this Pull Request

Testing instructions

  • Install WCPay 5.8.1.
  • Go through onboarding.
  • Update to 5.9.0.
  • With base branch, expect a fatal error below. With PR branch, expect no error.
Failed opening '/srv/users/user217f0884/apps/user217f0884/public/wp-content/plugins/woocommerce-payments/includes/admin/class-wc-rest-user-exists-controller.php' for inclusion (include_path='.:/opt/sp/php7.4/lib/php') in /srv/users/user217f0884/apps/user217f0884/public/wp-content/plugins/woocommerce-payments/includes/class-wc-payments.php on line 954

Fatal error: Uncaught Error: Class 'WC_REST_User_Exists_Controller' not found in /srv/users/user217f0884/apps/user217f0884/public/wp-content/plugins/woocommerce-payments/includes/class-wc-payments.php:955 Stack trace: #0 /srv/users/user217f0884/apps/user217f0884/public/wp-includes/class-wp-hook.php(308): WC_Payments::init_rest_api(Object(WP_REST_Server)) #1 /srv/users/user217f0884/apps/user217f0884/public/wp-includes/class-wp-hook.php(332): WP_Hook->apply_filters(NULL, Array) #2 /srv/users/user217f0884/apps/user217f0884/public/wp-includes/plugin.php(517): WP_Hook->do_action(Array) #3 /srv/users/user217f0884/apps/user217f0884/public/wp-includes/rest-api.php(577): do_action('rest_api_init', Object(WP_REST_Server)) #4 /srv/users/user217f0884/apps/user217f0884/public/wp-includes/rest-api.php(535): rest_get_server() #5 /srv/users/user217f0884/apps/user217f0884/public/wp-includes/rest-api.php(2889): rest_do_request(Object(WP_REST_Request)) #6 [internal function]: rest_preload_api_request(Array, '/wc-analytics/r...') #7 /srv/use in /srv/users/user217f0884/apps/user217f0884/public/wp-content/plugins/woocommerce-payments/includes/class-wc-payments.php on line 955

  • Run npm run changelog to add a changelog file, choose patch to leave it empty if the change is not significant. You can add multiple changelog files in one PR by running this command a few times.
  • Covered with tests (or have a good reason not to test in description ☝️)
  • Tested on mobile (or does not apply)

Post merge

malithsen added 2 commits May 15, 2023 07:48
This fixes an error that happens during the plugin upgrade process.
We'll need to keep this file until a more robust solution is found.
@botwoo
Copy link
Collaborator

botwoo commented May 15, 2023

Test the build

Option 1. Jetpack Beta

  • Install and activate Jetpack Beta.
  • Use this build by searching for PR number 6303 or branch name fix/plugin-upgrade in your-test.site/wp-admin/admin.php?page=jetpack-beta&plugin=woocommerce-payments

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:

  • Latest commit: e039b80
  • Build time: 2023-05-15 16:06:57 UTC

Note: the build is updated when a new commit is pushed to this PR.

@malithsen malithsen marked this pull request as ready for review May 15, 2023 15:06
@github-actions
Copy link
Contributor

github-actions bot commented May 15, 2023

Size Change: 0 B

Total Size: 1.1 MB

ℹ️ View Unchanged
Filename Size
release/woocommerce-payments/assets/css/admin.css 1.01 kB
release/woocommerce-payments/assets/css/success.css 401 B
release/woocommerce-payments/dist/blocks-checkout.css 1.39 kB
release/woocommerce-payments/dist/blocks-checkout.js 39.8 kB
release/woocommerce-payments/dist/checkout.css 444 B
release/woocommerce-payments/dist/checkout.js 27.9 kB
release/woocommerce-payments/dist/index.css 39.6 kB
release/woocommerce-payments/dist/index.js 235 kB
release/woocommerce-payments/dist/multi-currency-analytics.js 1.05 kB
release/woocommerce-payments/dist/multi-currency-switcher-block.js 58.2 kB
release/woocommerce-payments/dist/multi-currency.css 2.99 kB
release/woocommerce-payments/dist/multi-currency.js 52.7 kB
release/woocommerce-payments/dist/order.css 248 B
release/woocommerce-payments/dist/order.js 15 kB
release/woocommerce-payments/dist/payment-gateways.css 704 B
release/woocommerce-payments/dist/payment-gateways.js 31.3 kB
release/woocommerce-payments/dist/payment-request.js 12 kB
release/woocommerce-payments/dist/settings.css 8.7 kB
release/woocommerce-payments/dist/settings.js 160 kB
release/woocommerce-payments/dist/subscription-edit-page.js 668 B
release/woocommerce-payments/dist/subscription-product-onboarding-modal.css 527 B
release/woocommerce-payments/dist/subscription-product-onboarding-modal.js 13 kB
release/woocommerce-payments/dist/subscription-product-onboarding-toast.js 703 B
release/woocommerce-payments/dist/subscriptions-empty-state.css 298 B
release/woocommerce-payments/dist/subscriptions-empty-state.js 11.9 kB
release/woocommerce-payments/dist/tos.css 236 B
release/woocommerce-payments/dist/tos.js 14.5 kB
release/woocommerce-payments/dist/upe_checkout.css 444 B
release/woocommerce-payments/dist/upe_checkout.js 33.1 kB
release/woocommerce-payments/dist/upe_split_checkout.css 444 B
release/woocommerce-payments/dist/upe_split_checkout.js 33.5 kB
release/woocommerce-payments/dist/upe_with_deferred_intent_creation_checkout.js 31.4 kB
release/woocommerce-payments/dist/upe-blocks-checkout.css 1.39 kB
release/woocommerce-payments/dist/upe-blocks-checkout.js 39.1 kB
release/woocommerce-payments/dist/upe-split-blocks-checkout.css 1.39 kB
release/woocommerce-payments/dist/upe-split-blocks-checkout.js 39.7 kB
release/woocommerce-payments/dist/woopay-express-button.js 16.6 kB
release/woocommerce-payments/dist/woopay.css 4.03 kB
release/woocommerce-payments/dist/woopay.js 70.4 kB
release/woocommerce-payments/includes/subscriptions/assets/css/plugin-page.css 633 B
release/woocommerce-payments/includes/subscriptions/assets/js/plugin-page.js 720 B
release/woocommerce-payments/vendor/automattic/jetpack-assets/build/i18n-loader.js 2.43 kB
release/woocommerce-payments/vendor/automattic/jetpack-assets/src/js/i18n-loader.js 1.01 kB
release/woocommerce-payments/vendor/automattic/jetpack-connection/dist/tracks-ajax.js 522 B
release/woocommerce-payments/vendor/automattic/jetpack-connection/dist/tracks-callables.js 581 B
release/woocommerce-payments/vendor/automattic/jetpack-identity-crisis/babel.config.js 160 B
release/woocommerce-payments/vendor/automattic/jetpack-identity-crisis/build/index.css 2.32 kB
release/woocommerce-payments/vendor/automattic/jetpack-identity-crisis/build/index.js 13.8 kB
release/woocommerce-payments/vendor/automattic/jetpack-identity-crisis/build/index.rtl.css 2.32 kB
release/woocommerce-payments/vendor/woocommerce/subscriptions-core/assets/css/about.css 1.2 kB
release/woocommerce-payments/vendor/woocommerce/subscriptions-core/assets/css/admin-order-statuses.css 403 B
release/woocommerce-payments/vendor/woocommerce/subscriptions-core/assets/css/admin.css 3.56 kB
release/woocommerce-payments/vendor/woocommerce/subscriptions-core/assets/css/checkout.css 299 B
release/woocommerce-payments/vendor/woocommerce/subscriptions-core/assets/css/modal.css 742 B
release/woocommerce-payments/vendor/woocommerce/subscriptions-core/assets/css/view-subscription.css 572 B
release/woocommerce-payments/vendor/woocommerce/subscriptions-core/assets/css/wcs-upgrade.css 411 B
release/woocommerce-payments/vendor/woocommerce/subscriptions-core/assets/js/admin/admin-pointers.js 544 B
release/woocommerce-payments/vendor/woocommerce/subscriptions-core/assets/js/admin/admin.js 9.07 kB
release/woocommerce-payments/vendor/woocommerce/subscriptions-core/assets/js/admin/jstz.js 6.8 kB
release/woocommerce-payments/vendor/woocommerce/subscriptions-core/assets/js/admin/jstz.min.js 3.83 kB
release/woocommerce-payments/vendor/woocommerce/subscriptions-core/assets/js/admin/meta-boxes-coupon.js 544 B
release/woocommerce-payments/vendor/woocommerce/subscriptions-core/assets/js/admin/meta-boxes-subscription.js 2.38 kB
release/woocommerce-payments/vendor/woocommerce/subscriptions-core/assets/js/admin/moment.js 22.1 kB
release/woocommerce-payments/vendor/woocommerce/subscriptions-core/assets/js/admin/moment.min.js 11.6 kB
release/woocommerce-payments/vendor/woocommerce/subscriptions-core/assets/js/admin/payment-method-restrictions.js 1.29 kB
release/woocommerce-payments/vendor/woocommerce/subscriptions-core/assets/js/admin/wcs-meta-boxes-order.js 502 B
release/woocommerce-payments/vendor/woocommerce/subscriptions-core/assets/js/frontend/payment-methods.js 355 B
release/woocommerce-payments/vendor/woocommerce/subscriptions-core/assets/js/frontend/single-product.js 429 B
release/woocommerce-payments/vendor/woocommerce/subscriptions-core/assets/js/frontend/view-subscription.js 1.38 kB
release/woocommerce-payments/vendor/woocommerce/subscriptions-core/assets/js/frontend/wcs-cart.js 387 B
release/woocommerce-payments/vendor/woocommerce/subscriptions-core/assets/js/modal.js 1.1 kB
release/woocommerce-payments/vendor/woocommerce/subscriptions-core/assets/js/wcs-upgrade.js 1.27 kB
release/woocommerce-payments/vendor/woocommerce/subscriptions-core/build/index.css 392 B
release/woocommerce-payments/vendor/woocommerce/subscriptions-core/build/index.js 3.06 kB

compressed-size-action

Copy link
Contributor

@shendy-a8c shendy-a8c left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code diff looks good. Will test. I left a comment about leaving changelog empty.
Also, the file can be an empty class and that will be fine I think.
The goal is to avoid the fatal error.

Significance: patch
Type: fix

Missing file during plugin upgrade
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest to not add any changelog for this PR as the problem this PR is trying to fix did not exist in previous version.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed in 1dd9f40

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I meant not to remove the whole changelog file because then GH check for changelog will fail.

You can:

  • Run npm run changelog.
  • Choose patch.
  • Leave empty when prompted with "Changelog entry. May be left empty if this change is particularly insignificant".
  • Leave a comment why you put an empty changelog.

I know that npm run changelog isn't too clear.

I talked about it several times, eg p1683308423223529/1683298938.613009-slack-CGGCLBN58

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I misunderstood. Added an empty changelog with a comment

@shendy-a8c
Copy link
Contributor

I updated PR description with better reproduce steps and error.

@malithsen
Copy link
Member Author

Also, the file can be an empty class and that will be fine I think.

Looks like the register_routes method within the class is also called during the upgrade. We can experiment with keeping the definition of that method and removing everything else, but opted to keep the whole file out of caution.

Copy link
Contributor

@shendy-a8c shendy-a8c left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides changelog concern, this PR should be good to merge. Approving.

@malithsen malithsen enabled auto-merge May 15, 2023 16:03
@malithsen malithsen added this pull request to the merge queue May 15, 2023
Merged via the queue into develop with commit d45c765 May 15, 2023
@malithsen malithsen deleted the fix/plugin-upgrade branch May 15, 2023 16:23
shendy-a8c added a commit that referenced this pull request May 16, 2023
@vladolaru
Copy link
Contributor

@malithsen @shendy-a8c Is this file still needed after half a year? As I understand, this is code that is not being used, but it is left for upgrade purposes.

@malithsen
Copy link
Member Author

malithsen commented Nov 15, 2023

As I understand, this is code that is not being used, but it is left for upgrade purposes.

@vladolaru This is correct. If we remove it, there's the risk of users upgrading from 5.8.x to a later version running into the error, but at this point % of versions below, that seems to be less than 1.5% 469f8fdffca011514a0024294f6dca43-logstash. Even for those users, it will not prevent the upgrade. So, I don't have any strong objections to removing this file.

For some additional context, this issue is being discussed in paJDYF-aZa-p2

@timur27 timur27 mentioned this pull request Dec 20, 2023
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants