Make WordPress Core

Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#28487 closed enhancement (maybelater)

Introduce a function to determine if the scheme of a URL is https

Reported by: johnbillion's profile johnbillion Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: Security Keywords: has-patch
Focuses: Cc:

Description

[28674] introduces some logic which determines if the home URL's scheme is https. We should abstract this logic into a helper function which can be reused. I'll be using it in a patch on #28427 at least.

Adding to the Security component as this forms part of our work on improving SSL in 4.0.

Attachments (3)

28487.diff (619 bytes) - added by johnbillion 10 years ago.
28487.is_ssl.diff (1.1 KB) - added by georgestephanis 10 years ago.
28487.2.diff (1.4 KB) - added by johnbillion 10 years ago.

Download all attachments as: .zip

Change History (17)

@johnbillion
10 years ago

#1 @johnbillion
10 years ago

  • Keywords has-patch added

Patch

#2 follow-up: @SergeyBiryukov
10 years ago

To avoid confusion with is_ssl(), I'd suggest something like is_https_url() as a name.

#3 @nacin
10 years ago

Hmm. Not sure this is necessary. Probably would cause more confusion than not.

Still trying to see how things shake out and whether we need functions to wrap the https check for home/siteurl, versus a generic helper.

#4 in reply to: ↑ 2 @johnbillion
10 years ago

My patch on #28427 demonstrates the use case for this.

Replying to SergeyBiryukov:

To avoid confusion with is_ssl(), I'd suggest something like is_https_url() as a name.

Agreed.

#5 @georgestephanis
10 years ago

Just added 28487.is_ssl.diff -- which integrates this functionality into the existing is_ssl function as a possible alternative placement.

The existing code and new code both check things via rather discrete methods, so I'm unsure if it's best to logically clump them in the same function, but just a possibility to chew on.

#6 @georgestephanis
10 years ago

Also, I got something confused in my head when I wrote the patch, thinking parse_url gets cranky if passes a non-URL string, whereas it just returns false. The @ prefixing it is unnecessary and should be dropped. My bad.

@johnbillion
10 years ago

#7 @johnbillion
10 years ago

In 28894:

Introduce is_https_url() for testing whether the scheme for a given URL is https. See #28487.

#8 @johnbillion
10 years ago

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

I've gone ahead and dropped this function in in order to move some other SSL tickets along. If we decide to go with specific functions for home and siteurl rather than this generic helper, that's fine and we can re-open this.

#9 @nacin
10 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

I'm concerned about limited utility and the ambiguity this introduces with it existing alongside is_ssl(). I suggest we remove it for now and return to a parse_url() check, since that's more clear anyway.

#10 @ocean90
10 years ago

In 29309:

Revert [28894] as it's currently unused in core.

see #28487.

#11 @ocean90
10 years ago

  • Milestone 4.0 deleted
  • Resolution set to maybelater
  • Status changed from reopened to closed

#12 @ocean90
10 years ago

In 29310:

Remove unit test for is_https_url(), see [29309].

see #28487.

#13 @ocean90
10 years ago

In 29311:

Replace is_https_url() with 'https' === parse_url( $url, PHP_URL_SCHEME ).

see #28427, #28487.

#14 @ocean90
10 years ago

My main reason for the revert was the naming of the function. I agree to nacin with the proximity to is_ssl(). For that 28487.is_ssl.diff looks much better. Otherwise I think has_url_scheme( $url, $scheme ) or get_url_scheme( $url ) would reduce the ambiguity with is_ssl() and aligns more with set_url_scheme().

Note: See TracTickets for help on using tickets.