-
Notifications
You must be signed in to change notification settings - Fork 0
Return default when sanitization fails and refactor sanitize_deep to be non-mutating #5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,7 +30,9 @@ public static function get_server_var( $var, $default = null ) { | |
| } | ||
|
|
||
| $unsafe = Arr::get_in_any( $data, $var, $default ); | ||
| return static::sanitize_deep( $unsafe ); | ||
| $safe = static::sanitize_deep( $unsafe ); | ||
|
|
||
| return $safe === null ? $default : $safe; | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -47,7 +49,9 @@ public static function get_server_var( $var, $default = null ) { | |
| */ | ||
| public static function get_get_var( string $var, $default = null ) { | ||
| $unsafe = Arr::get( (array) $_GET, $var, $default ); | ||
| return static::sanitize_deep( $unsafe ); | ||
| $safe = static::sanitize_deep( $unsafe ); | ||
|
|
||
| return $safe === null ? $default : $safe; | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -64,7 +68,9 @@ public static function get_get_var( string $var, $default = null ) { | |
| */ | ||
| public static function get_post_var( string $var, $default = null ) { | ||
| $unsafe = Arr::get( (array) $_POST, $var, $default ); | ||
| return static::sanitize_deep( $unsafe ); | ||
| $safe = static::sanitize_deep( $unsafe ); | ||
|
|
||
| return $safe === null ? $default : $safe; | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -81,7 +87,9 @@ public static function get_post_var( string $var, $default = null ) { | |
| */ | ||
| public static function get_env_var( string $var, $default = null ) { | ||
| $unsafe = Arr::get( (array) $_ENV, $var, $default ); | ||
| return static::sanitize_deep( $unsafe ); | ||
| $safe = static::sanitize_deep( $unsafe ); | ||
|
|
||
| return $safe === null ? $default : $safe; | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -135,6 +143,7 @@ public static function get_raw_superglobal( string $superglobal ) { | |
| */ | ||
| public static function get_sanitized_superglobal( string $superglobal ) { | ||
| $var = static::get_raw_superglobal( $superglobal ); | ||
|
|
||
| return static::sanitize_deep( $var ); | ||
| } | ||
|
|
||
|
|
@@ -177,7 +186,9 @@ public static function get_var( $var, $default = null ) { | |
| } | ||
|
|
||
| $unsafe = Arr::get_in_any( $requests, $var, $default ); | ||
| return static::sanitize_deep( $unsafe ); | ||
| $safe = static::sanitize_deep( $unsafe ); | ||
|
|
||
| return $safe === null ? $default : $safe; | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -190,27 +201,50 @@ public static function get_var( $var, $default = null ) { | |
| * @param mixed $value The value, or values, to sanitize. | ||
| * | ||
| * @return mixed|null Either the sanitized version of the value, or `null` if the value is not a string, number or | ||
| * array. | ||
| * array. If the nested value can't be sanitized, it's removed. | ||
| */ | ||
| public static function sanitize_deep( &$value ) { | ||
| public static function sanitize_deep( $value ) { | ||
| if ( is_bool( $value ) ) { | ||
| return $value; | ||
| } | ||
|
|
||
| if ( is_string( $value ) ) { | ||
| $value = htmlspecialchars( $value ); | ||
| return $value; | ||
| return htmlspecialchars( | ||
| $value, | ||
| ENT_QUOTES | ENT_SUBSTITUTE, | ||
| 'UTF-8' | ||
| ); | ||
| } | ||
|
|
||
| if ( is_int( $value ) ) { | ||
| $value = filter_var( $value, FILTER_VALIDATE_INT ); | ||
| return $value; | ||
| return filter_var( $value, FILTER_VALIDATE_INT, FILTER_NULL_ON_FAILURE ); | ||
| } | ||
|
|
||
| if ( is_float( $value ) ) { | ||
| $value = filter_var( $value, FILTER_VALIDATE_FLOAT ); | ||
| return $value; | ||
|
|
||
| // Drop NAN and INF. | ||
| if ( ! is_finite( $value ) ) { | ||
| return null; | ||
| } | ||
|
|
||
| return filter_var( $value, FILTER_VALIDATE_FLOAT, FILTER_NULL_ON_FAILURE ); | ||
| } | ||
|
|
||
| if ( is_array( $value ) ) { | ||
| array_walk( $value, [ __CLASS__, 'sanitize_deep' ] ); | ||
| return $value; | ||
| $sanitized = []; | ||
|
|
||
| foreach ( $value as $key => $item ) { | ||
| $clean = self::sanitize_deep( $item ); | ||
|
|
||
| // Remove nested values that can't be sanitized. | ||
| if ( $clean === null ) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this should be a controllable behavior. It's fine for this to have a default of dropping, but I would add a
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @lucatume as in throw an exception if we failed to sanitize a value? |
||
| continue; | ||
| } | ||
|
|
||
| $sanitized[ $key ] = $clean; | ||
| } | ||
|
|
||
| return $sanitized; | ||
| } | ||
|
|
||
| return null; | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
$typeor usegettype($value). Then, if validation fails you would return the empty version of that type instead of the value itself.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jonwaldstein That would get very complicated once you start involving arrays and nested values of different types.
I agree
$defaultmay 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
$fallbackif 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
$defaultsince 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 😏There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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😄There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 😏