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

Prevent plugin initialization if Jetpack 8.1 or earlier is installed #760

Merged
merged 2 commits into from
Sep 1, 2020

Conversation

DanReyLop
Copy link
Contributor

@DanReyLop DanReyLop commented Jul 1, 2020

Due to changes in the autoloader, if Jetpack 8.1 or earlier is installed alongside WCPay, the whole site would crash. The crash would happen in the \Jetpack\Config initialization code.

I added a quick & dirty check to show an admin notice if Jetpack 8.1 or earlier is installed, and prevent the rest of the plugin from initializing. I didn't want to spend a lot of effort in this so it's not as smart as the main dependency checker (which shows a link to update the dependency, tells you which version of the dependency you have, etc). I just wanted something temporary that's better than the site crashing.

Jetpack 8.2 (which is already compatible with WCPay) was released in February 2020. I think we can remove this piece of code in a few months, maybe when that version is 1 year old? Anyway, I left a TODO comment, if someone stumbles upon it in the distant future it can be removed.

To test:

  • Install Jetpack 8.1 without using this WCPay branch. Note that the site crashes.
  • Install Jetpack 8.2. Everything works fine.
  • Install Jetpack 8.1 again, now using this WCPay branch. You'll see a notice telling you to upgrade, and you won't see the [$] Payments menu at all.
  • Install Jetpack 8.2, still using this WCPay branch. Everything should work fine.
  • Disable Jetpack. Everything should still work fine.

Fixes #577.

Due to changes in the autoloader, if Jetpack 8.1 or earlier is installed alongside WCPay, the whole site would crash. The crash would happen in the `\Jetpack\Config` initialization code.

I added a quick & dirty check to show an admin notice if Jetpack 8.1 or earlier is installed, and prevent the rest of the plugin from initializing. I didn't want to spend a lot of effort in this so it's not as smart as the main dependency checker (which shows a link to update the dependency, tells you which version of the dependency you have, etc). I just wanted something temporary that's better than the site crashing.

Jetpack 8.2 (which is already compatible with WCPay) was released in February 2020. I think we can remove this piece of code in a few months, maybe when that version is 1 year old? Anyway, I left a TODO comment, if someone stumbles upon it in the distant future it can be removed.

To test:
- Install Jetpack 8.1 without using this WCPay branch. Note that the site crashes.
- Install Jetpack 8.2. Everything works fine.
- Install Jetpack 8.1 again, now using this WCPay branch. You'll see a notice telling you to upgrade, and you won't see the `[$] Payments` menu at all.
- Install Jetpack 8.2, still using this WCPay branch. Everything should work fine.
- Disable Jetpack. Everything should still work fine.
@kbrown9
Copy link
Member

kbrown9 commented Jul 2, 2020

Hi @DanReyLop! I tested this to see what's going on.

Here’s what I tested:

  • The latest stable release of Woocommerce Payments, v1.1.0.
  • Jetpack v8.1.1.

I see this error:

Fatal error: Uncaught Error: Call to undefined method Automattic\Jetpack\Connection\Manager::configure() in /srv/users/user1ceceb88/apps/user1ceceb88/public/wp-content/plugins/woocommerce-payments/vendor/automattic/jetpack-config/src/class-config.php:211

Does that match what you’ve seen? Also, Woocommerce Payments v1.1.0 uses jetpack-autoloader v1.7.0, correct? So the new v2.0 Jetpack autoloader isn't involved in this error.

The above error is caused by a limitation in the v1.x Jetpack Autoloader. That autoloader doesn’t know where the latest version of a package is until all of the plugins have loaded.

In Jetpack v8.1.1, it appears that Jetpack used the Connection package before all of the plugins had loaded, most importantly before Woocommerce Payments had loaded. Therefore, the autoloader used Jetpack’s version of the Connection package, not the later version installed by Woocommerce Payments.

I think your approach preventing plugin initialization is the only way to resolve this. Even when WooCommerce Payments starts using the v2.0 autoloader package, Jetpack v8.1.1 will still load first and load its connection package before WooCommerce Payments is loaded.

I hope that makes sense; let me know if anything's unclear!

@DanReyLop
Copy link
Contributor Author

I see this error ... Does that match what you’ve seen?

Correct, that's what I'm seeing.

Also, Woocommerce Payments v1.1.0 uses jetpack-autoloader v1.7.0, correct? So the new v2.0 Jetpack autoloader isn't involved in this error.

Correct. We can't update jetpack-autoloader to 2.0 until WooCommerce does it. Even though we only use the woocommerce/woocommerce dependency for testing, composer install wouldn't be able to run and it would be quite annoying.

I think your approach preventing plugin initialization is the only way to resolve this. Even when WooCommerce Payments starts using the v2.0 autoloader package, Jetpack v8.1.1 will still load first and load its connection package before WooCommerce Payments is loaded.

That makes sense, thanks for clarifying! I wasn't completely clear about why this was happening so I just "treated the symptom", which is the only thing we can do :)

?>
<div class="notice wcpay-notice notice-error">
<p><b><?php echo esc_html( __( 'WooCommerce Payments', 'woocommerce-payments' ) ); ?></b></p>
<p><?php echo esc_html( __( 'The version of Jetpack installed is too old to be used with WooCommerce Payments. Please deactivate or update Jetpack.', 'woocommerce-payments' ) ); ?></p>
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it should probably explicitly mention WooCommerce Payments is disabled too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added in 8aa0aa5

@@ -53,3 +56,32 @@ function wcpay_init() {

// Make sure this is run *after* WooCommerce has a chance to initialize its packages (wc-admin, etc). That is run with priority 10.
add_action( 'plugins_loaded', 'wcpay_init', 11 );
Copy link
Contributor

@allendav allendav Jul 15, 2020

Choose a reason for hiding this comment

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

Please add a comment saying that if they change the priority here they need to do so in wcpay_check_old_jetpack_version too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added in 8aa0aa5

Copy link
Contributor

@dwainm dwainm left a comment

Choose a reason for hiding this comment

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

🚢

@dwainm
Copy link
Contributor

dwainm commented Aug 24, 2020

Tested and works as advertised. Also re-ran the build and it passed all non-e2e tests: https://travis-ci.com/github/Automattic/woocommerce-payments/builds/176175825 ( e2e tests were still running when I submitted this comment)

@dwainm
Copy link
Contributor

dwainm commented Aug 24, 2020

It seems like e2e tests failed but the reasons seem unrelated.

@DanReyLop DanReyLop merged commit 4320aa8 into master Sep 1, 2020
@DanReyLop DanReyLop deleted the add/check-if-old-jetpack-version branch September 1, 2020 11:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enforce Jetpack 5.3 minimum
4 participants