WordPress.org

Make WordPress Core

Opened 9 years ago

Closed 8 years ago

Last modified 8 years ago

#9008 closed task (blessed) (fixed)

add a home_url() function to enforce http/https scheme

Reported by: Denis-de-Bernardy Owned by:
Milestone: 3.0 Priority: high
Severity: major Version: 2.7
Component: Template Keywords: has-patch
Focuses: Cc:

Description

bloginfo() seems to ignore the site's requested scheme for things like feed url, pingback url, edit uri, wlwmanifest.

Attachments (9)

9008.diff (20.9 KB) - added by Denis-de-Bernardy 9 years ago.
9008.2.diff (21.6 KB) - added by ryan 9 years ago.
9008.3.diff (19.8 KB) - added by hakre 8 years ago.
9008-deprecate-get_option-bloginfo.diff (1.6 KB) - added by nacin 8 years ago.
Rough cut for deprecating bloginfo('url') and get_option('home')
9008.5.diff (1.7 KB) - added by nacin 8 years ago.
Deprecate bloginfo('url').
home_url_fixes.patch (6.9 KB) - added by junsuijin 8 years ago.
for use in addition to nacin's 9008.5.diff
9008.4.diff (3.0 KB) - added by PeteMall 8 years ago.
9008.6.diff (730 bytes) - added by PeteMall 8 years ago.
Revert parse_url()
9008_script-loader.patch (345 bytes) - added by ocean90 8 years ago.

Download all attachments as: .zip

Change History (54)

#1 @Denis-de-Bernardy
9 years ago

that being, if you browse https://yoursite.com, the relevant links will still point to http://yoursite.com

#2 @DD32
9 years ago

Because your site url is set to http://yoursite.com in your options..?

#3 @Denis-de-Bernardy
9 years ago

indeed, but it's still inconsistent:

https://dev.semiologic.com/

If you look at the source, you'll notice that the permalinks to posts go to the https version, and that most of the includes (scripts, css) use https. (Those that don't are from my own plugins, which I'm in the process of fixing to use site_url().)

I might be getting this wrong, but it seems to me that on the https version of the site, some kind of site_home() function should be used in a manner that is identical to the site_url() function for the bloginfo links and like.

#4 @DD32
9 years ago

Ah.. I see what you mean now.

I generally use site_url('') or site_url('/') over bloginfo now..

#5 @Denis-de-Bernardy
9 years ago

  • Keywords dev-feedback added
  • Summary changed from bloginfo() ignores scheme to inconsistent http/https scheme

Right. The complicated one to change is get_feed_url(), because there is not equivalent (best I know anyway) to site_url() for the home url.

A separate candidate "fix" would be to enforce http for sites that are declared to use it. In other words, in site_url():

$url = str_replace( 'http://', "{$scheme}://", get_option('siteurl') );

Should really be something like:

$url = str_replace( array('http://', 'https://'), "{$scheme}://", get_option('siteurl') );

And then, when a url is browsed as https:// where http:// should be used, it would redirect to the http:// versions of that url.

#6 @Denis-de-Bernardy
9 years ago

Maybe we should add some kind of FORCE_SCHEME define? :-)

#7 @ryan
9 years ago

  • Milestone changed from 2.7.1 to 2.8

We could add home_url() and FORCE_SSL_HOME.

#8 @sivel
9 years ago

  • Cc matt@… added

#9 @Denis-de-Bernardy
9 years ago

moving this to 2.9, because it's a potentially huge patch.

#10 @Denis-de-Bernardy
9 years ago

  • Keywords needs-patch early added; dev-feedback removed
  • Milestone changed from 2.8 to 2.9
  • Priority changed from normal to high
  • Summary changed from inconsistent http/https scheme to add a home_url() function to enforce http/https scheme

#11 @Denis-de-Bernardy
9 years ago

  • Keywords has-patch added; needs-patch removed

attached patch sticks to defining the function, enforcing the ssl setting of the page if relevant (no define forces the SSL), and replaces occurrences of get_option(home) with the new function.

#12 @Denis-de-Bernardy
9 years ago

I suggest we worry about introducing a FORCE_SSL define once this gets checked in.

#13 @Denis-de-Bernardy
9 years ago

adding to this, the links in wp-admin are kept verbatim, in case ADMIN_SSL is turned on.

#14 @Denis-de-Bernardy
9 years ago

  • Keywords needs-review added

#16 @ryan
9 years ago

  • Milestone changed from 2.9 to 3.0

#17 @mattrude
9 years ago

  • Cc m@… added

#18 @westi
9 years ago

  • Type changed from defect (bug) to enhancement

Marking this as an enhancement as we are talking about adding functionality by adding a home_url() function rather than fixing a bug.

@ryan
9 years ago

#19 @ryan
9 years ago

Refreshed patch. I changed it so if you're in the admin and is_ssl() the links are not forced to be https since most people wouldn't want that. We can add defines for those who want that.

@hakre
8 years ago

#20 @hakre
8 years ago

Reviewed, updated ryans patch a bit to follow code guidelines better for the new function.

#22 @Denis-de-Bernardy
8 years ago

  • Keywords 2nd-opinion added; has-patch needs-review early removed

I suggest we leave this one open pending extra discussion and maybe an extra patch. I had originally opened this one because I was testing the following two use-cases:

  1. browsing https when it really should be http
  1. browsing http when it really should be https

2nd opinion welcome on how we ought to better manage those two use cases. Maybe we should 301 redirect. Maybe we should introduce a FORCE_SSL that forces the SSL pref on the front end. All options are available.

#23 @Denis-de-Bernardy
8 years ago

possibly of interest: the force_ssl_content() in ms-functions.php

#24 follow-up: @nacin
8 years ago

We need to go through and update bloginfo('url') references to home_url(), no? (29 instances in core.)

#25 in reply to: ↑ 24 @nacin
8 years ago

Replying to nacin:

We need to go through and update bloginfo('url') references to home_url(), no? (29 instances in core.)

Unless I'm missing something -- I think we should do that, then mark bloginfo('url') as deprecated (see #11652) and change bloginfo() to use home_url() instead of get_option('home').

I wonder if we can also mark get_option('home') as deprecated in some way as well. (We would have to prevent the deprecated warning inside home_url()).

#26 follow-up: @Denis-de-Bernardy
8 years ago

for backwards compat, I think we should stick to making get_bloginfo('url') return home_url(). I'm sure a lot of themes and plugins use that function.

#27 in reply to: ↑ 26 @nacin
8 years ago

Replying to Denis-de-Bernardy:

for backwards compat, I think we should stick to making get_bloginfo('url') return home_url(). I'm sure a lot of themes and plugins use that function.

Right but at the same time we should suggest that they switch over from get_bloginfo('url') as well. home_url() is much cleaner.

#28 @Denis-de-Bernardy
8 years ago

Very true. I'd +1 the switch, personally.

#29 @nacin
8 years ago

Not perfect, but I have attached a quick patch to deprecate get_option('home') and *bloginfo*('url'). We'd still need to replace all bloginfo('url') references in core -- there's 41 instances when you include *bloginfo_rss().

There are ways we can do this to avoid the backtrace, but it's only running when WP_DEBUG is on so it isn't a big footprint. (This also means that it won't trigger the deprecated_argument_run hook, which would normally be triggered even if WP_DEBUG is off.)

And I guess instead of did_action('init') we could just replace the get_option('home') instance in wp-settings.php with home_url(). Ideas welcome.

@nacin
8 years ago

Rough cut for deprecating bloginfo('url') and get_option('home')

#30 @Denis-de-Bernardy
8 years ago

@nacin: is the additional logic in get_option() really needed? that function is called so many times all over the place that we ought to keep it as simple as possible.

#31 @westi
8 years ago

We can't put that logic into get_option().

We could put something on a pre_option filter hook to do the check there so it only runs for the home option.

I wonder if this is one place where putting the message in the face of the developer is a step too far due to the cost outweighing the benifit and we should just hilight it in the phpDoc.

#32 @dd32
8 years ago

I wonder if this is one place where putting the message in the face of the developer is a step too far due to the cost outweighing the benifit and we should just hilight it in the phpDoc.

Thats my thoughts too.

Personally, I think we should just make it known that if a plugin wants to retrieve a correct url (http/https) that the api should be used instead of relying on the underlying options.

#33 @nacin
8 years ago

ryan and I discussed this briefly on IRC today.

We can put logic into get_bloginfo(), for sure. (We would need to change a few dozen references.) If we are to do it for get_option(), the best idea so far I think is this:

  • Add a simple do_action() to the top of the *_url() family (or maybe come up with a filter that is actually useful).
  • On WP_DEBUG, attach a filter to pre_option_$option. In the callback, call _deprecated_argument(), but first check $wp_current_filter to make sure we're not coming from a *_url() function.

That avoids the backtrace and I feel the benefit would outweigh what I think would be minimal cost.

@nacin
8 years ago

Deprecate bloginfo('url').

#34 @nacin
8 years ago

We should deprecate bloginfo('url') for home_url(), and bloginfo('wpurl') for site_url(), we'll just need to cleanup the bloginfo calls first so there aren't lots of deprecated warnings in trunk.

@junsuijin
8 years ago

for use in addition to nacin's 9008.5.diff

#35 @junsuijin
8 years ago

  • Cc junsuijin@… added
  • Keywords has-patches added

home_url() works well but I don't follow the logic on trunk wp-includes/link-template.php, li 1773

[...] && !is_admin() [...]

If I don't want to force SSL logins/admin (due to self-signed certificate or whatever other reason), but I want to use them personally for security purposes, any link that takes me back to the frontend returns me to the non-SSL scheme, which means any links from the frontend to the backend (at least those using the home_url api), don't work. This seems like a good enough reason to ensure that the current scheme is preserved on admin page links by default, and a define (as ryan suggested), could then be added to alter that behavior if desired. Basically, I think it's safe to assume that the user will want to maintain whatever scheme they've used to login with, since their cookies won't be valid for the other scheme.

Additionally, I think nacin's patch to deprecate bloginfo('url') is good and have submitted an additional patch to use home_url() appropriately on admin pages. I've also changed the offending line in link-template.php just to make it clear what I mean, and added comments where there is not yet an api function to handle the task. Thanks to nacin for his help finding the other locations that need updating :)

#36 @nacin
8 years ago

  • Keywords needs-patch added; 2nd-opinion has-patches removed

(In [13474]) get_bloginfo('wpurl') should use site_url(), not get_option('siteurl'). see #9008


Because of the filters in get_bloginfo(), get_bloginfo_rss(), and bloginfo_rss(), and their wide use, I think it would be okay to leave as-is and simply transition to home_url() and site_url() inside get_bloginfo().

We could still use a solution to route direct calls from get_option() to home_url() and site_url().

What's left: There are still calls to get_option('siteurl') in core. They need to become site_url(). Many of them then have a path then appended, those can be changed to use the first parameter of site_url().

#37 @nacin
8 years ago

  • Type changed from enhancement to task (blessed)

#38 @mikeschinkel
8 years ago

  • Cc mikeschinkel@… added

@PeteMall
8 years ago

#39 follow-up: @ryan
8 years ago

Is that dropped parse_url() intentional?

#40 in reply to: ↑ 39 @nacin
8 years ago

Replying to ryan:

Is that dropped parse_url() intentional?

Nope. I mentioned it to Pete, he didn't get around to refreshing the patch. It's good to go otherwise.

@PeteMall
8 years ago

Revert parse_url()

#41 @nacin
8 years ago

(In [14430]) Revert accidental removal of parse_url(). see #9008, props PeteMall.

#42 @ocean90
8 years ago

  • Keywords has-patch added; needs-patch removed

9008_script-loader.patch: Added home_url() to ignore

Prevents a fatal error:

Fatal error:  Call to undefined function home_url() in \wp\wp-includes\script-loader.phpon line 399.

#43 @nacin
8 years ago

(In [14442]) Define home_url() in load-scripts.php. see #9008, props ocean90.

#44 @nacin
8 years ago

(In [14455]) Revert part of r14429 as it broke HTTP in setup-config. Revisit when we decide to fully deprecate get_bloginfo(url). see #9008.

#45 @ryan
8 years ago

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

Let's close this as done against 3.0. Open new tickets for further work.

Note: See TracTickets for help on using tickets.