Make WordPress Core

Opened 11 years ago

Closed 7 days ago

#16867 closed enhancement (maybelater)

Where is it appropriate to use filter_var

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

Description

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 (21)

#1 follow-up: @scribu
11 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
11 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
11 years ago

  • Cc dkikizas@… added

#4 @nacin
11 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
11 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
10 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
10 years ago

Closed #19988 as a duplicate.

#9 @iandunn
9 years ago

  • Cc ian.dunn@… added

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


8 years ago

#11 @chriscct7
6 years ago

  • Keywords dev-feedback added

#12 @aaroncampbell
6 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@204.32.222.14 (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
6 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
6 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.


6 years ago

#16 @martin.krcho
2 weeks ago

  • Resolution maybelater deleted
  • Status changed from closed to reopened

Minimum requirements for WordPress have changed since this discussion was closed:
https://wordpress.org/about/requirements/

Is your opinion about using the filter_var still the same, @dd32 and @aaroncampbell? Is it perhaps time to start using this built-in PHP function instead of doing some of the validation and sanitisation ourselves?

#17 follow-up: @dd32
13 days ago

I don't have a strong opinion right now - I don't know how stable the functions are in PHP 7+ as most of my experience with it was back with PHP 5.x.

If filter_var()'s implementation for a given 'thing' is stable and hasn't had any major bug fixes since the lowest PHP supported by WP, and isn't dependant upon modules that may not be compiled into all PHPs, then I see no harm in using it where appropriate.. but there can be security implications of relying upon it if PHP only fixed something in their supported branches. It can also make back-porting things to older WP's harder where they run on older versions of PHP as a result.

Here's some quick examples I pulled up, where an implementation in pure-PHP might be a better option

#18 in reply to: ↑ 17 ; follow-up: @desrosj
12 days ago

  • Keywords close added

Replying to dd32:

I think that this is ultimately going to prevent the use of filter_var() in WordPress. Even if things are stable and secure, the behavior could potentially be different depending on the version of PHP being used, even for supported versions like in this example (8.0 and 7.4).

We could determine that the use of certain filters is consistent across versions today, but there's nothing saying that won't change in the future. And that would have us trying to backfill the differences piecemeal, which may end up being more difficult to maintain and harder to see the full picture in the end.

I personally think this should just be closed out as one of those things WordPress can't realistically rely on without more consistency upstream, and a change in the project's PHP version support policy to only support versions that are actively maintained upstream (security).

#19 in reply to: ↑ 18 @aaroncampbell
12 days ago

Replying to desrosj:

I personally think this should just be closed out as one of those things WordPress can't realistically rely on without more consistency upstream, and a change in the project's PHP version support policy to only support versions that are actively maintained upstream (security).

I agree with this recommendation. I think our PHP version support policy, while having many benefits, precludes us from using these without taking on the unsustainable burden of version-specific compat fallbacks.

#20 follow-up: @martin.krcho
12 days ago

Thank you for your comments and reasoning, everyone. I will look into validating URL format without filter_var.

#21 in reply to: ↑ 20 @desrosj
7 days ago

  • Keywords close removed
  • Resolution set to maybelater
  • Status changed from reopened to closed

Replying to martin.krcho:

I will look into validating URL format without filter_var.

@martinkrcho To be clear, it could be entirely appropriate to use filter_var within your own code. But within the context of WordPress Core itself, it doesn't seem feasible.

I'm going to close this one out again. Discussion can always continue on closed tickets. And if the state of filter_var changes in the future, it can always be reopened.

Note: See TracTickets for help on using tickets.