Sync: IDC: Add get_raw_url method to Jetpack_Sync_Functions #5852

ebinnion · 2016-12-12T19:39:31Z

For sites that use domain mapping and multi language, it can be quite hard to sync uniform URLs back to WP.com, which can cause the site to be put in IDC.

To address this, I'd like to start syncing the unfiltered values for home and siteurl. This will introduced some jankiness where we sync the non-mapped domain for domain mapped sites. But, we can then address that by adding support for the top multi language and domain mapping plugins.

I've added support for Donncha's and WPMU Dev's domain mapping plugins. But, it's untested at the moment.

To test:

Create network and point domains to network
Checkout branch
Install and configure one of the domain mapping plugins
Ensure that the mapped domain gets sent to WPCOM
You can comment out this line to make sure that the non-mapped domain gets synced properly as well (this simulates the raw URL getting synced when a non-supported domain mapping plugin is used
Repeat steps for other domain mapping plugin
Ensure there are no errors

Note: it could take several minutes the URLs to be synced each time you change.

ebinnion · 2016-12-12T23:23:05Z

In the latest commit, I've added a first pass at support for Donncha's and WPMU Dev's domain mapping plugins. But, it's untested at the moment.

ebinnion · 2016-12-16T19:14:36Z

cc @simonwheatley @joshbetz for some input on how this would work for VIP sites that use domain mapping.

lezama · 2016-12-17T18:32:09Z

sync/class.jetpack-sync-functions.php

-			self::normalize_www_in_url( 'home', 'home_url' )
-		);
+		if (
+			Jetpack_Constants::is_defined( 'JETPACK_SYNC_USE_RAW_URL' ) &&


Any advantages if we flag this via Sync setting? so we can tweak it if needed via the sync settings endpoint?

That seems like a good change. 👍

lezama · 2016-12-17T18:39:03Z

sync/class.jetpack-sync-functions.php

+			$value = Jetpack_Sync_Options::get_option( $option_name );
+		}
+
+		if ( is_ssl() ) {


isn't checking for is_ssl going to cause issues? shouldn't we stick to what's on the option?

I don't think it would cause issues since we have the ssl normalization logic now. But, if we stick with this raw approach, then I think we can get rid of the ssl normalization. So, I think I agree with you that we could probably get rid of this. 😉

simonwheatley · 2016-12-19T12:53:26Z

…for some input on how this would work for VIP sites that use domain mapping.

Currently on VIP Go we change the domains for single sites by changing the home and siteurl option values in the DB, as you might expect. For multisite we do not currently support any domain mapping plugins, so we have clients change the values in the wp_blogs table for domain and path, and also change the home and siteurl option values.

Another domain mapping plugin to check out is https://github.com/humanmade/Mercator (note they're currently moving the UI and SSO into separate plugins: humanmade/Mercator#77 and humanmade/Mercator#79).

ebinnion · 2016-12-20T01:22:09Z

Thanks for the input. 🙇

Currently on VIP Go we change the domains for single sites by changing the home and siteurl option values in the DB, as you might expect.

This being the case, using this raw approach (and not applying filters) should work fine then. We should probably test on a test VIP GO site before 4.6 though, just to make sure.

Another domain mapping plugin to check out is https://github.com/humanmade/Mercator

@kraftbj had mentioned that one to me, and it was on my radar as an option. I think once we get this in, it should be fairly easy to roll in support for Mercator and perhaps one or two others that we decide to fully support.

ebinnion · 2017-01-30T21:36:54Z

I just rebased against master to clear out the merge conflict.

ebinnion · 2017-01-30T22:03:57Z

I pushed a commit so that we begin syncing the raw URL by default. Users should be able to opt-out of this by define( 'JETPACK_SYNC_USE_RAW_URL', false );

…raw url

ebinnion · 2017-07-22T21:16:59Z

This PR is testing a lot better now. I've tested the following scenarios:

On a site that is accessible via http and https, load up wp-admin, click around, the synced value should be the one in the DB
On a multisite, the domain that is synced should be the one from the db, unless the site is using one of the supported domain mapping plugins, then the domain synced should be the mapped domain.
On a site that is accessible via two separate domain names, the home/siteurl values that are stored in the DB should be synced.

We still need to test the case of cloning a site from A to B, production to localhost for example, and we should test against WPMU dev's plugin again since it's been several months since we last tested.

@lezama @roccotripaldi @dereksmart - can I get your help with this at the beginning of this next week? There are 2-3 open issues that should benefit from this?

lezama · 2017-07-22T23:38:25Z

sync/class.jetpack-sync-functions.php

-			'home_url',
-			self::normalize_www_in_url( 'home', 'home_url' )
-		);
+		$url = self::get_raw_or_filtered_url( 'home', 'home_url' );


We could pass the constant name here and simplify get_raw_url

roccotripaldi

Haven't tested yet, but the code looks great.

roccotripaldi · 2017-07-24T16:10:33Z

3rd-party/domain-mapping.php

+			$hooked = call_user_func( array( $this, self::$test_methods[ $i ] ) );
+		}
+	}
+


ebinnion · 2017-07-24T18:59:07Z

Latest commit makes a couple of changes to the IDC logic uses the same URLs that are sent in sync.

lezama · 2017-07-24T21:39:05Z

sync/class.jetpack-sync-module-callables.php

@@ -145,7 +145,7 @@ public function maybe_sync_callables() {
 			return;
 		}

-		$callable_checksums = (array) get_option( self::CALLABLES_CHECKSUM_OPTION_NAME, array() );
+		$callable_checksums = (array) Jetpack_Options::get_raw_option( self::CALLABLES_CHECKSUM_OPTION_NAME, array() );


was this causing some misbehaviours?

From memory, I noticed that the callables weren't syncing as often as I expected them to. Once I made this change, it cleared up.

lezama · 2017-07-24T21:59:33Z

I think it's safe enough to merge, let's keep an eye on all the IDC/Sync things while in beta 😱

ebinnion added IDC [Package] Sync [Status] In Progress labels Dec 12, 2016

ebinnion added this to the 4.6 milestone Dec 12, 2016

ebinnion self-assigned this Dec 12, 2016

ebinnion force-pushed the update/sync-home-siteurl-from-db branch 2 times, most recently from db62c72 to 3890883 Compare December 13, 2016 17:39

ebinnion mentioned this pull request Dec 13, 2016

IDC / WPML: need to exclude WPML sites from current IDC process #5793

Closed

ebinnion added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] In Progress labels Dec 15, 2016

ebinnion requested review from roccotripaldi, dereksmart and kraftbj December 15, 2016 02:13

lezama reviewed Dec 17, 2016

View reviewed changes

jeherve added the [Pri] High label Jan 26, 2017

ebinnion force-pushed the update/sync-home-siteurl-from-db branch from 46386fc to 52c02a6 Compare January 30, 2017 21:36

ebinnion force-pushed the update/sync-home-siteurl-from-db branch from 3e30cce to 64d93f9 Compare January 30, 2017 22:07

jeherve modified the milestones: 4.5.1, 2/17 - February Jan 30, 2017

ebinnion force-pushed the update/sync-home-siteurl-from-db branch from 2c0a78e to e54dbeb Compare July 19, 2017 00:28

ebinnion added 2 commits July 18, 2017 21:49

Sync: Stop using removed Jetpack_Sync_Options class

f6e22f5

Sync: IDC: Get home/siteurl with sync functions method

e244dde

ebinnion force-pushed the update/sync-home-siteurl-from-db branch from 8a19915 to af86a56 Compare July 22, 2017 00:20

ebinnion added 2 commits July 22, 2017 12:32

IDC: Remove broken test that is no longer applicable

bfd6b46

Sync: Factor out logic to get raw URL; Do not set scheme

1824c78

ebinnion force-pushed the update/sync-home-siteurl-from-db branch from a2dff44 to 1824c78 Compare July 22, 2017 17:33

Sync: Update broken test after last commit to not modify protocol on …

0404b78

…raw url

ebinnion added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] In Progress labels Jul 22, 2017

lezama reviewed Jul 22, 2017

View reviewed changes

roccotripaldi reviewed Jul 24, 2017

View reviewed changes

3rd-party/domain-mapping.php

$hooked = call_user_func( array( $this, self::$test_methods[ $i ] ) );

}

}

Copy link

Member

roccotripaldi Jul 24, 2017

Choose a reason for hiding this comment

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

Very cool!

Sync: IDC: Switch IDC methods to conditonally use raw URL as well

15f0555

ebinnion requested a review from a team as a code owner July 24, 2017 17:41

ebinnion added 2 commits July 24, 2017 14:52

Sync: IDC: Update docblocks to 5.2 for raw URL logic

c448404

Sync: IDC: Simplify some logic around getting URLs

1e4a204

ebinnion force-pushed the update/sync-home-siteurl-from-db branch from 1e4a204 to f798850 Compare July 24, 2017 20:29

lezama reviewed Jul 24, 2017

View reviewed changes

ebinnion force-pushed the update/sync-home-siteurl-from-db branch from 3166c36 to 1e4a204 Compare July 24, 2017 21:56

lezama approved these changes Jul 24, 2017

View reviewed changes

ebinnion added this to the 5.2 milestone Jul 24, 2017

ebinnion merged commit 3ba94b2 into master Jul 24, 2017

ebinnion deleted the update/sync-home-siteurl-from-db branch July 24, 2017 22:18

ebinnion removed the [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. label Jul 24, 2017

ebinnion mentioned this pull request Jul 24, 2017

Debugger: Add some sync IDC URLs to the debugger #7515

Merged

This was referenced Oct 12, 2017

Sync: Add protocol to raw urls #7984

Merged

Sync: Fix issue with syncing raw URLs for multisites #8006

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sync: IDC: Add get_raw_url method to Jetpack_Sync_Functions #5852

Sync: IDC: Add get_raw_url method to Jetpack_Sync_Functions #5852

ebinnion commented Dec 12, 2016 •

edited

Loading

ebinnion commented Dec 12, 2016

ebinnion commented Dec 16, 2016

lezama Dec 17, 2016

ebinnion Dec 19, 2016

lezama Dec 17, 2016

ebinnion Dec 19, 2016

simonwheatley commented Dec 19, 2016

ebinnion commented Dec 20, 2016 •

edited

Loading

ebinnion commented Jan 30, 2017

ebinnion commented Jan 30, 2017

ebinnion commented Jul 22, 2017

lezama Jul 22, 2017

roccotripaldi left a comment

roccotripaldi Jul 24, 2017

ebinnion commented Jul 24, 2017

lezama Jul 24, 2017

ebinnion Jul 24, 2017

lezama commented Jul 24, 2017

Sync: IDC: Add get_raw_url method to Jetpack_Sync_Functions #5852

Sync: IDC: Add get_raw_url method to Jetpack_Sync_Functions #5852

Conversation

ebinnion commented Dec 12, 2016 • edited Loading

ebinnion commented Dec 12, 2016

ebinnion commented Dec 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonwheatley commented Dec 19, 2016

ebinnion commented Dec 20, 2016 • edited Loading

ebinnion commented Jan 30, 2017

ebinnion commented Jan 30, 2017

ebinnion commented Jul 22, 2017

Choose a reason for hiding this comment

roccotripaldi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebinnion commented Jul 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lezama commented Jul 24, 2017

ebinnion commented Dec 12, 2016 •

edited

Loading

ebinnion commented Dec 20, 2016 •

edited

Loading