Make WordPress Core

Opened 12 years ago

Closed 12 years ago

Last modified 12 years ago

#20759 closed enhancement (fixed)

Implement set_url_scheme() where appropriate

Reported by: johnbillion's profile johnbillion Owned by: ryan's profile 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 12 years ago.
20759.diff (6.4 KB) - added by ryan 12 years ago.
Refresh of *_url() changes
20759.2.diff (806 bytes) - added by SergeyBiryukov 12 years ago.
20759.3.diff (919 bytes) - added by SergeyBiryukov 12 years ago.
20759.4.diff (921 bytes) - added by ryan 12 years ago.
Respect https in home
20759-ut.diff (1.1 KB) - added by ryan 12 years ago.
unit tests for https in home

Download all attachments as: .zip

Change History (23)

@johnbillion
12 years ago

#1 @johnbillion
12 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.

#2 @ryan
12 years ago

  • Milestone changed from Awaiting Review to 3.5

#3 @ryan
12 years ago

In [21664]:

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

@ryan
12 years ago

Refresh of *_url() changes

#11 @ryan
12 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

#12 @trepmal
12 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.

#13 @SergeyBiryukov
12 years ago

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

#14 @ryan
12 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

#15 follow-up: @mindctrl
12 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?

#16 in reply to: ↑ 15 ; follow-up: @SergeyBiryukov
12 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.

#17 in reply to: ↑ 16 @mindctrl
12 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!

#18 @ryan
12 years ago

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

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

#19 @ryan
12 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 12 years ago by ryan (previous) (diff)

#20 @ryan
12 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 12 years ago by ryan (previous) (diff)

#21 @nacin
12 years ago

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

@ryan
12 years ago

Respect https in home

@ryan
12 years ago

unit tests for https in home

#22 @ryan
12 years ago

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

In [21937]:

Honor https in home option.

fixes #20759

#24 @mindctrl
12 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.