add common functions that return int/float to escaping functions #2082
Conversation
kkmuffme
force-pushed
the
int-float-escaping-functions
branch
from
f3f7d26
to
932fb94
Compare
|
Looks good, but I'd like to go through the added functions, just to be safe (no pun intended) 😅 |
|
Yes please, take your time. I grabbed them off of psalm only taking global, non-class functions |
| 'bzerrno' => true, | ||
| 'cal_days_in_month' => true, | ||
| 'cal_to_jd' => true, | ||
| 'calendar_week_mod' => true, |
There was a problem hiding this comment.
Turns out this function is not type safe, so the results can result in a fatal error on PHP 8+, and a warning on PHP < 8 (albeit it returns 0 along the warning).
I'd remove this from the list.
There was a problem hiding this comment.
This function is already there in the list right now, I didn't add this function.
Also it always returns a float, so it's safe/auto-escaped?
There was a problem hiding this comment.
Try passing it a non numerical string and see what will happen. It's not type safe 😅
There was a problem hiding this comment.
https://3v4l.org/H5evB returns float 0 in PHP 7 and fatal in PHP 8, both is safe for output. Am I missing something?
There was a problem hiding this comment.
I wasn't aware that throwing an error was considered safe. 👀
There was a problem hiding this comment.
It is. "Safe" is defined as any value that does not allow for an XSS attack vector - that's why we're escaping in the first place, isn't it?
| 'curl_errno' => true, | ||
| 'curl_multi_add_handle' => true, | ||
| 'curl_multi_errno' => true, | ||
| 'curl_multi_exec' => true, | ||
| 'curl_multi_remove_handle' => true, | ||
| 'curl_multi_select' => true, | ||
| 'curl_pause' => true, | ||
| 'curl_share_errno' => true, |
There was a problem hiding this comment.
I think all of these would throw a Using cURL functions is highly discouraged. Use wp_remote_get() instead. warning, not sure if we should include them in this list.
In theory, they are escaped (safe to use because they return integer), but not sure if the above error disqualifies them from being on this list 🤷🏼♂️
There was a problem hiding this comment.
Since these are mostly for "features" not supported by wp_remote_get I think it makes sense to keep those in.
| 'do_shortcode' => true, | ||
| 'do_shortcode_tag' => true, | ||
| 'doubleval' => true, | ||
| 'easter_date' => true, |
There was a problem hiding this comment.
As noted on the docs page, this function will generate a warning if the year is outside of the range for Unix timestamps, and if this isn't covered by tests or a sniff, I'd leave it out of this list.
There was a problem hiding this comment.
But a warning doesn't mean the returned value isn't escaped? The returned value is always a safe int.
| 'mb_ereg_search_getpos' => true, | ||
| 'mb_strwidth' => true, | ||
| 'mb_substr_count' => true, | ||
| 'mhash_count' => true, |
There was a problem hiding this comment.
This function is deprecated as of 8.1.0, should we keep it in the list?
There was a problem hiding this comment.
Yes, bc WP will support PHP < 8.1 for at least 5 more years it seems 😂
|
The code was moved to a different file with WPCS 3.0.0 and no more feedback given, will probably not get merged. |
|
@kkmuffme you could have rebased this PR, and we could have given it another look, no need to close it 🤷🏼♂️ |
|
It's easier to create a new PR than rebasing this one, bc there are tons of rebase conflicts since the files this code is in has changed and also they're sorted alphabetically now already (which this PR did too). If you still want a new PR, that's no problem, I can PR it :-) |
#1969
count
rand
random_int
ceil
floor
round