Where is it appropriate to use filter_var

[aaroncampbell]

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.

[scribu]

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

[aaroncampbell]

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.

[nacin]

Think its safe to close this as maybelater.

[westi]

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.

[SergeyBiryukov]

Related: #18658

[nacin]

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

[nacin]

Closed #19988 as a duplicate.

[aaroncampbell]

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?

[dd32]

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.

[aaroncampbell]

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.

[martin.krcho]

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?

[dd32]

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

• ​https://pwning.systems/posts/php_filter_var_shenanigans/ + ​https://github.com/php/php-src/commit/771dbdb319fa7f90584f6b2cc2c54ccff570492d (2022; could have security implications)
• ​https://github.com/php/php-src/pull/6573 (2021; Where it's only fixed in PHP8+, although an unlikely code branch to used in WP, or unlikely to cause issues if so, but a change-in-behaviour between PHP versions)
• ​https://bugs.php.net/bug.php?id=71745 (2016; Unsure if this was fixed in other versions, but this is an example of where using it for security can have unexpected bypasses)

[desrosj]

Replying to dd32:

• ​https://github.com/php/php-src/pull/6573 (2021; Where it's only fixed in PHP8+, although an unlikely code branch to used in WP, or unlikely to cause issues if so, but a change-in-behaviour between PHP versions)

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).

[aaroncampbell]

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.

[martin.krcho]

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

[desrosj]

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.