Skip to content

Conversation

@ebinnion
Copy link
Contributor

Fixes #7997

In #7997, @enejb reported an issue where his multisite was syncing an incorrect URL, the base URL for the site. This was caused by #5852 when we/I started syncing the raw URLs.

While I did test multisites, I apparently didn't test multisites with the constant set. Or, I would've noticed the wrong URL was being synced.

I was able to narrow this down to these lines in core WordPress:

// WP_HOME and WP_SITEURL should not have any effect in MS
remove_filter( 'option_siteurl', '_config_wp_siteurl' );
remove_filter( 'option_home',    '_config_wp_home'    );

The _config_wp_siteurl and _config_wp_home filters check if the WP_HOME or WP_SITEURL constants are set, and if so, uses those constants. BUT ... the filters that call those functions are removed for multisites.

In #5852, I was always returning the constant value, which is the incorrect behavior.

To test:

  • Connect a subsite on a network
  • Run a full sync and ensure values are synced correctly in network admin

@ebinnion ebinnion added [Package] Sync [Team] Poseidon [Type] Bug When a feature is broken and / or not performing as intended labels Oct 17, 2017
@ebinnion ebinnion added this to the 5.5 milestone Oct 17, 2017
@ebinnion ebinnion self-assigned this Oct 17, 2017
@ebinnion ebinnion requested a review from a team as a code owner October 17, 2017 16:18
@ebinnion ebinnion added the [Status] Needs Testing We need to add this change to the testing call for this month's release label Oct 17, 2017
@ebinnion ebinnion force-pushed the fix/raw-url-for-multisites branch from 484fa2e to 9fcd051 Compare October 17, 2017 16:21
@ebinnion ebinnion added the [Status] Needs Review This PR is ready for review. label Oct 17, 2017
@enejb
Copy link
Member

enejb commented Oct 17, 2017

Can we add a test here?
So that is a bit more clear what is going on.

@enejb enejb added the [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. label Oct 17, 2017
@enejb
Copy link
Member

enejb commented Oct 17, 2017

Also Can you explain what the process will be in fixing sites that are broken?
Do we have to ask the user to reconnect? Will they be fixed by when the plugin update happens?

@ebinnion
Copy link
Contributor Author

Existing test is now updated.

Also Can you explain what the process will be in fixing sites that are broken? Do we have to ask the user to reconnect? Will they be fixed by when the plugin update happens?

The sites should update automatically after plugin upgrade, once callables sync. If that fails, the user can initiate a full sync or cycle the connection.

@ebinnion ebinnion removed the [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. label Oct 17, 2017
Copy link
Contributor

@lezama lezama left a comment

Choose a reason for hiding this comment

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

:shipit:

@oskosk oskosk added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. [Status] Needs Testing We need to add this change to the testing call for this month's release labels Oct 19, 2017
Copy link
Contributor

@oskosk oskosk left a comment

Choose a reason for hiding this comment

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

:shipit:

@oskosk oskosk merged commit ca9c9c9 into master Oct 19, 2017
@oskosk oskosk deleted the fix/raw-url-for-multisites branch October 19, 2017 19:23
@oskosk oskosk removed the [Status] Ready to Merge Go ahead, you can push that green button! label Oct 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Package] Sync [Pri] BLOCKER [Type] Bug When a feature is broken and / or not performing as intended

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants