Make WordPress Core

Opened 10 years ago

Closed 5 years ago

Last modified 4 years ago

#16867 closed enhancement (maybelater)

Where is it appropriate to use filter_var

Reported by: aaroncampbell Owned by:
Milestone: Priority: normal
Severity: normal Version: 3.2
Component: General Keywords: westi-likes
Focuses: Cc:


Now that we require PHP 5.2 we have access to a whole plethora of new functions. One of those is filter_var. It seems like this could be useful for things like validating and filtering E-Mail addresses, URLs, IPs, etc. (there are plenty of [php.net/manual/en/filter.filters.php filters]).

This was brought up in #15379 where nacin said:

Those can be buggy. We could potentially leverage it internally but we need to watch out for vulnerabilities and what not across PHP versions.

I mostly wanted this to be the place where we decide what could benefit from it and what shouldn't.

Change History (15)

#1 follow-up: @scribu
10 years ago

I guess it boils down to this: do we want to handle our own bugs or PHPs? :)

#2 in reply to: ↑ 1 @aaroncampbell
10 years ago

Replying to scribu:

I'm not sure that's a fair assessment. We obviously use a TON of PHP functions without much worry as to dealing with PHPs bugs, and when PHP bugs have plagued us we've dealt with them. If we can say that this particular function (or some of it's specific filters) are considerably more prone to bugs, or even that there are several versions of PHP that we will be supporting with known issues, then maybe we could justify ignoring them. I just want to make sure we've considered their possible advantages such as red-lining the whole is_email function.

#3 @demetris
10 years ago

  • Cc dkikizas@… added

#4 @nacin
9 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to maybelater
  • Status changed from new to closed
  • Type changed from defect (bug) to enhancement

Think its safe to close this as maybelater.

#5 @westi
9 years ago

  • Keywords westi-likes needs-patch added; 2nd-opinion removed
  • Milestone set to Future Release
  • Resolution maybelater deleted
  • Status changed from closed to reopened
  • Version set to 3.2

I think we can consider using this for a few small situations where it is likely to be safe and stable to use and gives better performance than our current solutions:

  • IP Address filtering - faster than the regular expression based solution we have now
  • is_email - better coverage than our current solution.

If someone was to create a patch for this that would be great - this includes creating a new IP address sanitisation wrapper function to replace the duplicated preg_ code we have at the moment.

#7 @nacin
9 years ago

Worth mentioning that it is possible to compile PHP with --disable-filter. Same deal as JSON. Need to be careful about compatibility here.

#8 @nacin
9 years ago

Closed #19988 as a duplicate.

#9 @iandunn
7 years ago

  • Cc ian.dunn@… added

This ticket was mentioned in IRC in #wordpress-dev by ocean90. View the logs.

6 years ago

#11 @chriscct7
5 years ago

  • Keywords dev-feedback added

#12 @aaroncampbell
5 years ago

So, here's my new take on this:
First, because you can compile PHP without these, anywhere we use these we'll really only be moving our exiting functions into compat.php. Which isn't necessarily bad, but it does mean to me that we need to see enough benefit from them to make it worth it.

The two most obvious places to use this are still is_email() and WP_Http::is_ip_address().

My tests on using it to replace is_email() surprised me. It looks like filter_var is not only slower, but it doesn't properly handle IP based E-Mail addresses like ace@ (straight from our unit tests).

My tests on IP addresses are a bit more limited, but are also more promising. It seems that filter_var( $ip, FILTER_VALIDATE_IP ) is about 2-2.5x faster than $http->is_ip_address(). I need a better set of test data for this though, to make sure it handles all the cases we do. It doesn't look like we have anything in our unit tests. Does anyone have a good list of valid and invalid IPs I could drop in to test?

#13 @dd32
5 years ago

Unless there's a method that is called often, I'd far prefer to just have our own implementation in use so as to be sure that any bugs in our implementation is found.

Branching in a sanitize/checking function such as WP_HTTP::is_ip_address based on the existence of filter_var() seems like a bad idea overall, not only does our sanitization then have to match whatever filter_var() does, but it's also likely to cause our code to be non-unit-testable (I know there's ways around that by using multiple methods, but that's more unreadable).

So count that as a -1 from me until the time where we can reliable rely upon filter_var() being present, and without any of the various bugs which have plagued the function in it's early days.

#14 @aaroncampbell
5 years ago

  • Keywords needs-patch dev-feedback removed
  • Milestone Future Release deleted
  • Resolution set to maybelater
  • Status changed from reopened to closed

Closing, as I agree with dd32 at this point. My stance is slightly different in that I would be all for branching the logic if it gave us a considerable speed improvement, but further tests simply aren't showing enough of an improvement to make it worth it.

This ticket was mentioned in Slack in #core-restapi by ocean90. View the logs.

4 years ago

Note: See TracTickets for help on using tickets.