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

Sync: warning on update #15209

Closed
jeherve opened this issue Mar 31, 2020 · 4 comments
Closed

Sync: warning on update #15209

jeherve opened this issue Mar 31, 2020 · 4 comments
Assignees
Labels
[Focus] Multisite [Package] Sync [Pri] Normal [Type] Bug When a feature is broken and / or not performing as intended

Comments

@jeherve
Copy link
Member

jeherve commented Mar 31, 2020

#15056 seems to trigger warnings in some scenarios, like switching branches in the Beta plugin:

Warning: call_user_func() expects parameter 1 to be a valid callback, class 'Automattic\Jetpack\Sync\Functions' does not have a method 'main_network_site_wpcom_id' in wp-content/plugins/jetpack-dev/vendor/automattic/jetpack-sync/src/modules/class-callables.php on line 213

@bisko Do you think you could take a look at it?

Thank you!

@jeherve jeherve added [Type] Bug When a feature is broken and / or not performing as intended [Focus] Multisite [Package] Sync [Pri] BLOCKER labels Mar 31, 2020
@jeherve jeherve added this to the 8.4 milestone Mar 31, 2020
@bisko
Copy link
Contributor

bisko commented Mar 31, 2020

I just tested with the Jetpack beta plugin, trying to switch to a random feature branch and it failed with a fatal:

[31-Mar-2020 11:28:24 UTC] PHP Fatal error:  Uncaught Error: Call to undefined method Automattic\Jetpack\Sync\Modules\Full_Sync_Immediately::get_sync_progress_percentage() in /var/www/html/wp-content/plugins/jetpack-dev/_inc/lib/debugger/debug-functions.php:115
Stack trace:
#0 /var/www/html/wp-includes/class-wp-hook.php(288): jetpack_debugger_enqueue_site_health_scripts()
#1 /var/www/html/wp-includes/class-wp-hook.php(312): WP_Hook->apply_filters()
#2 /var/www/html/wp-includes/plugin.php(478): WP_Hook->do_action()
#3 /var/www/html/wp-admin/admin-header.php(104): do_action()
#4 /var/www/html/wp-admin/admin.php(234): require_once('/var/www/html/w...')
#5 {main}
  thrown in /var/www/html/wp-content/plugins/jetpack-dev/_inc/lib/debugger/debug-functions.php on line 115

I think this issue is happening because there's a split moment when the "old" and "new" Jetpack code runs side by side and it's possible that it's failing because the Feature branch one is switching to might not be rebased to the latest master version and might be missing some features that trigger a fatal.

I don't think this is an issue of the main Jetpack plugin, but more related to how the Beta plugin switches around the code. I'm not going to close this issue for the moment and I'm going to dig into how the Jetpack Beta plugins work to confirm.

@bisko
Copy link
Contributor

bisko commented Mar 31, 2020

Looking at the code for the Jetpack Beta plugin and testing things out, this seems to be the case.

In the method admin_page_load we try to install the chosen branch and then the code flow continues until the end of the admin_page_load method where we perform wp_safe_redirect.

The problem occurs because we don't halt the "current" script execution immediately after the redirect and we execute all the handlers that come after the end of admin_page_load - other hooks, shutdown handlers, etc.

This (guessing due to the nature how code is loaded and executed) causes desync in the code where we load some code to be executed before the new plugin is activated. After the new code is put in place, we continue the execution flow and when we try to run the code that was loaded beforehand, it tries to load some of the functions to execute, but the files are not there or running an "older" version that doesn't have the code that needs to be executed and it causes either a Warning in call_user_func's case or a Fatal if we try direct function execution.

A solution might be to just add an exit(); after the wp_safe_redirect and it should prevent those errors from occurring. Going to test that out and send a PR for if it works.

@bisko
Copy link
Contributor

bisko commented Mar 31, 2020

Pushed a PR that should fix the issue - Automattic/jetpack-beta#98

@bisko bisko added the [Status] Needs Review To request a review from Crew. Label will be renamed soon. label Mar 31, 2020
@jeherve jeherve removed this from the 8.4 milestone Mar 31, 2020
@bisko
Copy link
Contributor

bisko commented Apr 1, 2020

Fixed in Jetpack Beta plugin v2.4.3

@bisko bisko closed this as completed Apr 1, 2020
@anomiex anomiex removed the [Status] Needs Review To request a review from Crew. Label will be renamed soon. label Oct 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Multisite [Package] Sync [Pri] Normal [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Development

No branches or pull requests

3 participants