General: Connection: Only force register on build_connect_url max one time#7745
General: Connection: Only force register on build_connect_url max one time#7745
Conversation
gravityrail
left a comment
There was a problem hiding this comment.
For reasons of expediency, I think it's ok to merge this - it will work. But I think the code could use a cleanup (as per my comment) which will make it much more readable and eliminate recursion entirely, as well as possibly ditching one of the function arguments.
| sprintf( '/sites/%d', $site_id ) .'?force=wpcom', | ||
| '1.1' | ||
| ); | ||
| // Let's check the existing blog token to see if we need to re-register. We only check once per minute |
There was a problem hiding this comment.
I believe all this logic should be encapsulated into a function, as part of the condition for entering the first part of the if-statement. That way no recursion is necessary:
if ( $register || ! $token || ! $site_id || ! $this->check_tokens_are_valid() ) {
// render register URL
} else {
// render authorize URL
}
In fact, if this works out you may be able to drop the $register arg completely from the function.
There was a problem hiding this comment.
Thanks for the review. I agree that we should probably refactor a bit. I'm not sure that we can get rid of the raw option which ensure we only call out once, but it could at least make things a bit more readable. 👍
|
Oh, I totally missed that one! Thanks for fixing, this fixes #7694 . |
Today, while testing Jetpack Onboarding, with latest
masterfor Jetpack, I cleared out the connection on my site with:wp option delete jetpack_optionswp option delete jetpack_private_optionsThen, when I tried to reconnect, I got an
ERR_TOO_MANY_REDIRECTSerror in Chrome. Upon debugging, it looks likebuild_connect_urlgot into some loop where it kept trying to register. Here's some debug.Note how the calls are back-to-back and
build_connect_urlis calling itself. This is actually Jetpack redirecting back to itself. I was able to isolate the issue back to #6964.How to reproduce
It's not foolproof, but I was often able to reproduce by connecting, and then running the WP CLI calls to delete local Jetpack options.
How to test
Get your site in a state where you get the redirect, then apply this patch.