WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 2 years ago

Last modified 2 years ago

#20759 closed enhancement (fixed)

Implement set_url_scheme() where appropriate

Reported by: johnbillion Owned by: ryan
Milestone: 3.5 Priority: normal
Severity: normal Version: 3.4
Component: General Keywords: has-patch needs-testing
Focuses: Cc:

Description

#18017 introduced set_url_scheme(). We should now use this function in place of all the places where we do scheme logic.

Patch coming up.

Attachments (6)

20759.patch (10.0 KB) - added by johnbillion 3 years ago.
20759.diff (6.4 KB) - added by ryan 3 years ago.
Refresh of *_url() changes
20759.2.diff (806 bytes) - added by SergeyBiryukov 2 years ago.
20759.3.diff (919 bytes) - added by SergeyBiryukov 2 years ago.
20759.4.diff (921 bytes) - added by ryan 2 years ago.
Respect https in home
20759-ut.diff (1.1 KB) - added by ryan 2 years ago.
unit tests for https in home

Download all attachments as: .zip

Change History (23)

@johnbillion3 years ago

comment:1 @johnbillion3 years ago

  • Keywords has-patch needs-testing added

Patch which replaces instances in core where we're doing things like str_replace( 'http://', 'https://, $url ) with set_url_scheme().

There are several other places in core where we're building the URL out based on the $_SERVER superglobal and is_ssl(), but I've left these as-is and only replaced instances where we're actually replacing the scheme in a URL with another.

comment:2 @ryan3 years ago

  • Milestone changed from Awaiting Review to 3.5

comment:3 @ryan3 years ago

In [21664]:

Use set_url_scheme(). Props johnbillion, MarcusPope. see #19037 #20759

@ryan3 years ago

Refresh of *_url() changes

comment:11 @ryan2 years ago

  • Owner set to ryan
  • Resolution set to fixed
  • Status changed from new to closed

In [21734]:

Use set_url_scheme() in the *_url() functions to keep things DRY. Props johnbillion. fixes #20759

comment:12 @trepmal2 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

r21664 causes a fatal error on a fresh install.

Just checked out revision 21768, config'd, and went to the home page expecting to be automatically redirected to the wp-admin/install.php but was presented with

Fatal error: Call to undefined function set_url_scheme() in /path/to/trunk/wp-includes/functions.php on line 2987

If I bypass and go directly to wp-admin/install.php, setup the site details, and return to the homepage, no error.

@SergeyBiryukov2 years ago

comment:13 @SergeyBiryukov2 years ago

20759.2.diff fixes the fatal error in wp_guess_url() (with a comment).

comment:14 @ryan2 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In [21797]:

Don't use set_url_scheme() in wp_guess_url(). wp_guess_url() is used during install before set_url_scheme() is loaded. Props SergeyBiryukov. fixes #20759

comment:15 follow-up: @mindctrl2 years ago

  • Cc mindctrl added
  • Resolution fixed deleted
  • Status changed from closed to reopened
  • Type changed from enhancement to defect (bug)

Something in this set of patches broke the Visit Site links in the admin bar when the site is SSL only. It strips out the https and instead of linking https://domain.com it only links domain.com (no http* - only relative). I've tested this on three separate installs and found this to be true.

My test sites are all full-time SSL, with no option to access the sites via non-SSL means which results in the following error:

400 Bad Request

Your browser sent a request that this server could not understand.
Reason: You're speaking plain HTTP to an SSL-enabled server port.
Instead use the HTTPS scheme to access this URL, please.

Shouldn't this not be stripping off the protocol at the front of the link?

@SergeyBiryukov2 years ago

comment:16 in reply to: ↑ 15 ; follow-up: @SergeyBiryukov2 years ago

  • Type changed from defect (bug) to enhancement

The ticket itself is still an enhancement.

Replying to mindctrl:

It strips out the https and instead of linking https://domain.com it only links domain.com (no http* - only relative).

I could not reproduce stripping off the protocol, it's 'http' on my install. Before [21734], however, it used to be 'https' in this case.

In [21734], scheme calculation was removed from get_site_url(), but not from get_home_url(), which is used for "Visit Site" link in the toolbar.

20759.3.diff brings consistency and fixes the issue with "Visit Site" link in my testing.

comment:17 in reply to: ↑ 16 @mindctrl2 years ago

Replying to SergeyBiryukov:

The ticket itself is still an enhancement.

Sorry about that. Wasn't thinking when I changed that. Not enough sleep!

I could not reproduce stripping off the protocol, it's 'http' on my install. Before [21734], however, it used to be 'https' in this case.

20759.3.diff brings consistency and fixes the issue with "Visit Site" link in my testing.

You're right. It wasn't stripping the protocol. My browser was just hiding it and I didn't inspect the code.

This patch fixes the problem for me. Thanks!

comment:18 @ryan2 years ago

20759.3.diff Tests_URL::test_home_url_from_admin() and Tests_URL::test_network_home_url_from_admin().

Version 0, edited 2 years ago by ryan (next)

comment:19 @ryan2 years ago

The Visit Site link is 'http' in 3.4 as well. We don't currently support forcing https for frontend. AFAICT the code currently in trunk behaves the same as 3.4.

Last edited 2 years ago by ryan (previous) (diff)

comment:20 @ryan2 years ago

To get an https home link in 3.4 you would need to have https in the home option. Support for https in home is hit or miss. It looks like a fluke that get_home_url() in 3.4 allowed the raw home option through in this situation.

Last edited 2 years ago by ryan (previous) (diff)

comment:21 @nacin2 years ago

We *could* implicitly support front SSL if https is the protocol used for the home option. But that seems a bit tenuous.

@ryan2 years ago

Respect https in home

@ryan2 years ago

unit tests for https in home

comment:22 @ryan2 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In [21937]:

Honor https in home option.

fixes #20759

comment:24 @mindctrl2 years ago

Yes, on my sites I have the WordPress Address (URL) and Site Address (URL) configured with https and it has worked fine in 3.4.x.

I just updated to the latest trunk containing this latest patch and it works great.

@nacin, I'm not sure why WP wouldn't support SSL if https is in the home option.

Note: See TracTickets for help on using tickets.