Skip to content

Fix "View Full Site" and "View Mobile Site" for WordPress Subdirectory Installations#12414

Merged
kraftbj merged 6 commits intoAutomattic:masterfrom
dstein64:master
May 21, 2019
Merged

Fix "View Full Site" and "View Mobile Site" for WordPress Subdirectory Installations#12414
kraftbj merged 6 commits intoAutomattic:masterfrom
dstein64:master

Conversation

@dstein64
Copy link
Copy Markdown
Contributor

@dstein64 dstein64 commented May 19, 2019 •
edited by jeherve
Loading

When WordPress is installed to a subdirectory, the "View Full Site" and "View Mobile Site" links do not work properly. Incidentally, the problem is masked when on a blog post page, but present on the home page.

For example, suppose a blog is hosted at https://somesite.com/blog, with this URL set for both WordPress Address and Site Address in the Wordpress settings (i.e., the blog is intended to be accessed at the subdirectory). Under this scenario, the "View Full Site" link expands to https://somesite.com/blog/blog/?ak_action=reject_mobile, which has an extra blog/. It should expand to https://somesite.com/blog/?ak_action=reject_mobile. The double occurrence of blog is due to the omitted $url argument to add_query_arg combined with the call to home_url. The call to add_query_arg returns /blog/?ak_action=reject_mobile and that is passed to home_url which returns https://somesite.com/blog/blog/?ak_action=reject_mobile.

The same thing happens for the "View Mobile Site" link.

The problem occurs on the blog's homepage, and results in a page not found. The same issue occurs on blog posts, where blog/ is added to the URL twice, but it appears that Wordpress is robust to such malformed URLs.

Changes proposed in this Pull Request:

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

  • This is a bug fix, not a new/modified feature.

Testing instructions:

  • To replicate the issue, navigate on a phone to a WordPress blog installed to a subdirectory, and click "View Full Page". The page will not be found (although further navigations will load the full site, not the mobile one).
  • With the changes in this PR, the problem is not present.

Proposed changelog entry for your changes:

  • Fix "View Full Site" and "View Mobile Site" for WordPress subdirectory installations

@dstein64 dstein64 requested a review from a team May 19, 2019 16:37
@jetpackbot
Copy link
Copy Markdown
Collaborator

jetpackbot commented May 19, 2019 •
edited
Loading

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: June 4, 2019.
Scheduled code freeze: May 28, 2019

Generated by 🚫 dangerJS against a10b3c2

@dstein64 dstein64 mentioned this pull request May 19, 2019
Merged
@jeherve jeherve added [Pri] Normal [Status] Needs Review This PR is ready for review. Bug When a feature is broken and / or not performing as intended [Feature] Mobile Theme aka minileven labels May 20, 2019
@jeherve jeherve added this to the 7.4 milestone May 20, 2019
@jeherve jeherve changed the title Fix "View Full Site" and "View Mobile Site" for Wordpress Subdirectory Installations Fix "View Full Site" and "View Mobile Site" for WordPress Subdirectory Installations May 20, 2019
@jeherve
Copy link
Copy Markdown
Member

jeherve commented May 20, 2019

@BoleynSu Do you want to give this a try, see if it works for you?

@BoleynSu
Copy link
Copy Markdown
Contributor

BoleynSu commented May 20, 2019 via email

jeherve
jeherve requested changes May 20, 2019
View reviewed changes
Copy link
Copy Markdown
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.

Thank you for the contribution.

This works well. I would only recommend escaping as late possible, so you could move esc_url and use it right as the anchor is being built / the link being outputted. This is a small detail right now, but could be important in the future if other things are added to the functions.

@jeherve jeherve added [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. and removed [Status] Needs Review This PR is ready for review. labels May 20, 2019
@dstein64
Copy link
Copy Markdown
Contributor Author

dstein64 commented May 20, 2019

@jeherve, a10b3c2 adds the requested modification. I also split one of the modified lines as it grew beyond 100 characters. However, I'm not sure if there is a suggested limit, as I saw further down in the file a line that is almost 300 characters.

Thanks for reviewing.

jeherve
jeherve approved these changes May 21, 2019
View reviewed changes
Copy link
Copy Markdown
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 looks good to me. 👍

@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels May 21, 2019
@kraftbj kraftbj merged commit 67c2f94 into Automattic:master May 21, 2019
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels May 21, 2019
jeherve added a commit that referenced this pull request May 23, 2019
@jeherve
Changelog: add #12414
9a8a4a0
jeherve added a commit that referenced this pull request May 27, 2019
@kraftbj @jeherve
7.4 Changelog (#12304)
0fb4436 
* Kick off the changelog

* Add 7.3.1

* Update date and post link

* changelog: add #12219

* changelog: add #12170

* changelog: add #12184

* Changelog: add #12268

* Changelog: add #12081

* Changelog: add #12323

* Changelog: add #12204

* Changelog: add #12269

* Changelog: add #12332

* changelog: add #12339

* changelog: add #12209

* Changelog: add #12319

* Changelog: add #12357

* Changelog: add #12124

* Changelog: add #12373

* Changelog: add #12252

* Changelog: add #12383

* Changelog: add #12372

* changelog: add #12337

* Changelog: add #12290

* Changelog: add #12301

* Changelog: add #12061

* Testing list: add instructions for #12061

* Changelog: add #12393

* Update minimum supported version

See #12287

* Changelog: add #12406

* Testing list: add #12406

* Changelog: add #12277

* Changelog: add #12412

* Changelog: add #11318

* Changelog: add #12328

* Changelog: add #12425

* Changelog: add #12380

* Changelog: add #12428

* Changelog: add #12414

* Changelog: add #12395

* Changelog & Testing list: add #12416, #12417, #12418, and #12348

* changelog: add #12379

* Changelog: add #12341

* changelog: add #12444

* Changelog: add #12434

* Changelog: add #12454

* Changelog: add #12460

* Changelog: add #12463

* Changelog: add #12457

* Changelog / testing list: add #10333

* Changelog: add #12467


Co-authored-by: Jeremy Herve <jeremy@jeremy.hu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jeherve jeherve jeherve approved these changes

Assignees

No one assigned

Labels

Bug When a feature is broken and / or not performing as intended [Feature] Mobile Theme aka minileven [Pri] Normal

Projects

None yet

Milestone

7.4

Development

Successfully merging this pull request may close these issues.

6 participants

@dstein64 @jetpackbot @jeherve @BoleynSu @kraftbj @matticbot