WordPress.org

Make WordPress Core

Opened 23 months ago

Closed 19 months ago

Last modified 19 months 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 23 months ago.
20759.diff (6.4 KB) - added by ryan 20 months ago.
Refresh of *_url() changes
20759.2.diff (806 bytes) - added by SergeyBiryukov 20 months ago.
20759.3.diff (919 bytes) - added by SergeyBiryukov 19 months ago.
20759.4.diff (921 bytes) - added by ryan 19 months ago.
Respect https in home
20759-ut.diff (1.1 KB) - added by ryan 19 months ago.
unit tests for https in home

Download all attachments as: .zip

Change History (23)

johnbillion23 months ago

comment:1 johnbillion23 months 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 ryan20 months ago

  • Milestone changed from Awaiting Review to 3.5

comment:3 ryan20 months ago

In [21664]:

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

ryan20 months ago

Refresh of *_url() changes

comment:11 ryan20 months 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 trepmal20 months 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.

SergeyBiryukov20 months ago

comment:13 SergeyBiryukov20 months ago

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

comment:14 ryan19 months 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: mindctrl19 months 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?

SergeyBiryukov19 months ago

comment:16 in reply to: ↑ 15 ; follow-up: SergeyBiryukov19 months 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 mindctrl19 months 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 ryan19 months ago

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

Last edited 19 months ago by ryan (previous) (diff)

comment:19 ryan19 months 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 19 months ago by ryan (previous) (diff)

comment:20 ryan19 months ago

To get an https home link in 3.4 you would need to have https in the home option. Support 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.

Version 0, edited 19 months ago by ryan (next)

comment:21 nacin19 months ago

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

ryan19 months ago

Respect https in home

ryan19 months ago

unit tests for https in home

comment:22 ryan19 months ago

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

In [21937]:

Honor https in home option.

fixes #20759

comment:24 mindctrl19 months 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.