Return default when sanitization fails and refactor sanitize_deep to be non-mutating

[defunctl]

Main Changes

• Fix - Default when sanitization fails: get_server_var, get_get_var, get_post_var, get_env_var, and get_var now return the provided $default when sanitize_deep() returns null (e.g. object, resource, or non-finite float) instead of returning null.

• Change - sanitize_deep no longer mutates: Signature changed from sanitize_deep( &$value ) to sanitize_deep( $value ). It returns a sanitized copy. Unsanitizable types return null.

• Change - Unsanitizable nested values are dropped: For arrays, we build a new array and skip any key whose value sanitizes to null (e.g. objects, NAN, INF).

• Change - Strings: htmlspecialchars() now uses ENT_QUOTES | ENT_SUBSTITUTE and 'UTF-8' for consistent encoding and quoting.

• Fix - Int/float: Int and float are now validated with filter_var( …, FILTER_VALIDATE_INT/FLOAT, FILTER_NULL_ON_FAILURE ) so invalid numeric values become null and callers get the default (or the key is dropped) instead of bad data. Non-finite floats (NAN, INF) are rejected with ! is_finite( $value ) before calling filter_var, so we never pass them in and avoid the PHP 8.5+ NAN coercion warning.

[johnhooks]

This looks good to me. Though I've never used this library, so I'll leave it to others to approve

... I am curious what invalid value was found in a super global.

[defunctl]

This looks good to me. Though I've never used this library, so I'll leave it to others to approve

... I am curious what invalid value was found in a super global.

https://wordpress.org/support/topic/_serverrequest_uri-set-to-invalid-type/ 😁

That is causing some fatals in my Kadence code because this library isn't returning the expected default string but null.

[lucatume]

I think this should be a controllable behavior. It's fine for this to have a default of dropping, but I would add a bool $drop_on_invalid = true parameter to control this. There are cases where it could make sense to not continue at all if any value is invalid.

[defunctl]

@lucatume as in throw an exception if we failed to sanitize a value?

[shvlv]

It makes sense to me. Maybe we should tweak the documentation a bit (at least remove passing by reference variable https://github.com/stellarwp/superglobals#superglobalssanitize_deep-value-)

[defunctl]

It makes sense to me. Maybe we should tweak the documentation a bit (at least remove passing by reference variable https://github.com/stellarwp/superglobals#superglobalssanitize_deep-value-)

Ahh good call! I'll push some changes.

[tarecord]

This all looks good to me 👍🏼

[jonwaldstein]

@defunctl I'm not sure returning the default value makes sense. It's really more philosophical though 😄 "why should the default value be returned if a value is set?", a default value is typically used when a value is not set - hence the word default. If the value is set then the default value doesn't really mean anything anymore.

It looks like the goal here is really to get the return value to match the intended type when validation fails to prevent type errors.

If that's the case, then how about either adding a param for $type or use gettype($value). Then, if validation fails you would return the empty version of that type instead of the value itself.

get_get_var('foo', 'bar', 'string')

// some kind of get empty $type function
return $safe === null ? '' : $safe;

[defunctl]

@jonwaldstein That would get very complicated once you start involving arrays and nested values of different types.

I agree $default may no longer be the best variable name though, but I think that just came from how Laravel methods have similar functionality.

I am happy to rename it to $fallback if that helps?

The goal here is to sanitize values (based on the initial way the library was built), and values that can't be sanitized should not be included in sanitized output.

[jonwaldstein]

@defunctl yeah I agree that approach would take some time.

Would you consider a sanitization validation fail as a value not being set since it will ultimately be null?
That could be enough from a philosophical standpoint to persuade my stance on the intention of $default since a value is technically not being set and thus should return the default. I imagine in most cases that default value will be used as an empty placeholder anyway 😏

[defunctl]

@jonwaldstein sanitization and validation are not the same thing though. This is just sanitizing and not validating.

But yes, I would said a failure or missing item is both null 😄

[jonwaldstein]

Right, it's just trying to sanitize the value which could ultimately return null if it's not able to. I was just using the word validation for that scenario which is not technically correct lol. I think your approach makes sense then - carry on 😏