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

REST API: Use home URL as unmapped URL #13818

Merged
merged 1 commit into from
Oct 25, 2019

Conversation

mmtr
Copy link
Member

@mmtr mmtr commented Oct 23, 2019

Fixes Automattic/wp-calypso#36837

Changes proposed in this Pull Request:

Uses the site URL used as the value of the unmapped_url option returned when requesting a site via REST API.

WordPress.com sites stores in the siteurl option the *.wordpress.com domain of a site, but this does not apply for Jetpack sites where that option could be the default value (same mapped domain as the home URL) or a custom URL defined by the user (i.e. subdirectory installs) that is not intended to be used as unmapped URL.

Given that Jetpack sites don't have an unmapped URL (they all have a mapped domain and can be accessed only through that mapped domain), this PR ensures its value fallbacks to the home URL (which is actually the mapped domain).

This was resulting in issues like Automattic/wp-calypso#36837, caused by Calypso building a wrong preview URL due to the unmapped URL pointing to the subdirectory where the WordPress files live.

Quick note to understand the differences between site and home URLs, since it can be very easy to confuse them:

  • Site URL: it is where the admin pages are, along with all the other parts of WordPress, such as the folders /wp-content/ and /wp-include. The value of this setting is displayed in the "Settings > General" screen as "WordPress Address (URL)". It is defined by the WP_SITEURL constant and stored in the siteurl site option.
  • Home URL: it is the public-facing part of a site. The value of this setting is displayed in the "Settings > General" screen as "Site Address (URL)". It is defined by the WP_HOME constant and stored in the home site option.

Is this a new feature or does it add/remove features to an existing part of Jetpack?

Fix an existing bug in the REST API shared with WP.com

Testing instructions:

  • Set up a Jetpack site with WordPress installed in a subdirectory:
    • Method 1: official and long way.
    • Method 2: hacky and quick way. We'll fake the site URL to make WordPress believe it is in a subdirectory. Since it is actually not in a subdirectory, this will result in the admin pages being unusable, but it'll be enough for testing the front-end and the API.
add_filter( 'site_url', function ( $site_url ) { return $site_url . '/wp'; } );
  • Apply D34373-code and sandbox public-api.wordpress.com.
  • Make a GET request to https://public-api.wordpress.com/rest/v1.1/sites/<YOUR_JETPACK_SITE>?fields=ID,URL,options&options=unmapped_url.
  • Verify the unmapped_url option points to the home URL of your site.
  • Go to https://wordpress.com/posts/<YOUR_JETPACK_SITE>.
  • Preview a post using the contextual menu on the right:

Screen Shot 2019-10-23 at 13 15 32

  • Make sure the post is previewed successfully and you don't get a 404 error.
  • Repeat steps with a Simple site and confirm posts can be previewed too.

Further testing

  • Check there are no regressions in other places where the unmapped URL is used, for both Jetpack and Simple sites:
    • Site redirect (Simple sites only). The unmapped URL is used in the "thank you" screen: Screen Shot 2019-10-23 at 13 32 04
    • Customizer. It should load the *.wordpress.com URL in an iframe for Simple sites, and redirect to the WP Admin customizer for Jetpack sites.
    • Preview a site. It should successfully preview a site with no 404 errors.
    • Preview a post with the classic editor. It should successfully preview a post with no 404 errors.
    • Stats spark line (Simple sites only). The image displayed points to wp-includes and it is built using the unmapped URL:
      image.

Proposed changelog entry for your changes:

REST API: Use home URL as unmapped URL

@mmtr mmtr requested a review from a team October 23, 2019 12:00
@mmtr mmtr self-assigned this Oct 23, 2019
@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello mmtr! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D34373-code before merging this PR. Thank you!

@jetpackbot
Copy link

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: November 5, 2019.
Scheduled code freeze: October 29, 2019

Generated by 🚫 dangerJS against 12e95f1

@mmtr mmtr added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] In Progress labels Oct 23, 2019
@mmtr mmtr requested a review from a team October 23, 2019 13:21
@gwwar gwwar added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Oct 23, 2019
@gwwar
Copy link
Contributor

gwwar commented Oct 23, 2019

Noting that D34373 has green tests. Not sure how to re-trigger the PR check here. We might need to rebase?

@kwight
Copy link
Contributor

kwight commented Oct 23, 2019

One way to test this is with a JN site and the home_url filter in a plugin:

  1. Create a JN site with the JP Beta plugin also installed, and this branch selected.
  2. Connect the site to your WordPress.com account.
  3. Activate this branch in the Beta plugin.
  4. Activate the Hello Dolly plugin, and add this to it with the Plugin Editor:
add_filter( 'home_url', function () { return 'testing'; } );

With D34373-code on a sandbox and the API sandboxed, we should be able to make the API calls above and get the correct unmapped URL.

@kwight
Copy link
Contributor

kwight commented Oct 23, 2019

🎉

Screen Shot 2019-10-23 at 2 35 42 PM

@kwight
Copy link
Contributor

kwight commented Oct 23, 2019

With a standalone, subdirectory install on Linode that appears to be working properly, the site URL looks like <domain>/index.php, so calls to /posts on WordPress.com produce a preview View link of https://<domain>/index.php/2019/10/23/hello-world/ (this happens even on JP stable, so it's not directly connected to this PR).

@mmtr
Copy link
Member Author

mmtr commented Oct 24, 2019

One way to test this is with a JN site and the home_url filter in a plugin:

Subdirectory installs typically have a home URL pointing to the root address (i.e. example.com) and a site URL pointing to the directory (i.e. example.com/wp), so we would need to use the site_url filter in this case

With a standalone, subdirectory install on Linode that appears to be working properly, the site URL looks like <domain>/index.php, so calls to /posts on WordPress.com produce a preview View link of https://<domain>/index.php/2019/10/23/hello-world/ (this happens even on JP stable, so it's not directly connected to this PR).

Seems to be normal, given the site has a custom permalink:

image

Does Calypso refuse from previewing those links?

@mmtr
Copy link
Member Author

mmtr commented Oct 24, 2019

I think there is an issue with the Linode site and probably it is not correctly configured. Trying to load a post (i.e. https://<domain>/index.php/2019/10/23/hello-world/) results in a Document root with no WP. WP is installed in /wordpress. error.

I'm trying now setting up a local env with Chassis, which seems to install WordPress in a subdirectory out-of-the-box.

@mmtr
Copy link
Member Author

mmtr commented Oct 24, 2019

I'm trying now setting up a local env with Chassis, which seems to install WordPress in a subdirectory out-of-the-box.

No luck with this since the site is not publicly available (so Jetpack cannot connect to it) and ngrok doesn't help given that it only exposes the site under http (making the site unpreviewable in WordPress.com).

@mmtr
Copy link
Member Author

mmtr commented Oct 24, 2019

Updated the testing instructions with the fake method suggested by @kwight. I tried that and it seems to work (despite being impossible to load the admin pages).

Copy link
Contributor

@gwwar gwwar left a comment

Choose a reason for hiding this comment

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

Mob tested with @mmtr @kwight and @gwwar. For a subdirectory install, verified a 404 preview before and a working preview after with this branch and diff applied.

Kudos to @griffbrad for providing the testing site! ✨

Screen Shot 2019-10-24 at 9 26 17 AM

Copy link
Contributor

@kwight kwight left a comment

Choose a reason for hiding this comment

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

🚢

@jeherve jeherve added [Feature] WPCOM API [Type] Bug When a feature is broken and / or not performing as intended labels Oct 25, 2019
@jeherve jeherve added this to the 7.9 milestone Oct 25, 2019
@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Oct 25, 2019
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

This seems to work well in my tests. Merging.

@jeherve jeherve merged commit 1b07673 into master Oct 25, 2019
@jeherve jeherve deleted the update/rest-api-unmapped-url-home-url branch October 25, 2019 21:20
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Oct 25, 2019
jeherve added a commit that referenced this pull request Oct 28, 2019
jeherve added a commit that referenced this pull request Oct 29, 2019
* 7.9: Changelog

* Update version number

* Update stable tag and tested up to

* Changelog: add #13530

* changelog: add #13578

* Changelog: add #13598

* Changelog: add entry for numerous block preview changes

* Changelog: add #13599

* changelog: add #13541

* Changelog: add #13542

* Changelog: add #13331

* Changelog: add #13558

* Changelog: add #13409

* Changelog: add #13582

* Changelog: add #13600

* Changelog: add #13601

* Changelog: add #13595

* Changelog: add #12695

* Changelog: add #13009

* Changelog: add #13649

* Changelog: add #13450

* Changelog: add #13507

* Changelog: add #13658

* Changelog: add #13687

* changelog: add #13683

* Changelog: add #9323

* Changelog: add #13681

* Fix typos in readme

* Add link to WordPress Beta Tester plugin

* Changelog: add #13630

* Changelog: add #13695

* Changelog: add #13659

* Changelog: add #13716

* Changelog: add #13664

* Changelog: add #13682

* Changelog: add #13362

* Changelog: add #13563

* Add testing list for #13563

* Changelog: add #13735

* Changelog: add #13752

* Changelog: add #13624

* Changelog: add #13756

* Changelog: add #13745

* Changelog: add #13728

* Changelog: add #13779

* Changelog: add #13699

* Changelog: add #13804

* Changelog: add #13761

* Changelog: add #13637

* Changelog: add #13517

* Changelog: add #13521

* Changelog: add #13729

* Testing list: add testing instructions for #13729

* Changelog: add sync changes

* Changelog: add #13807

* Changelog: add #13654

* Changelog: add #13795

* Changelog: add #13801

* Changelog: add #13818

* Changelog: add #13725

* Changelog: add #13831

* Changelog: add #13516

* Testing list: add Twenty Twenty instructions

* Changelog: add #13799

* Changelog: add #13805

* Changelog: add #13688

* Changelog: add #13830
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] WPCOM API Touches WP.com Files [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.

Previews: Some Jetpack site modal previews may fail with 404s [2]
6 participants