path: fix input type checking regression#5244
Conversation
|
@thealphanerd Yes, it should. |
|
yay for smoke testing! yay for more tests! this is a great outcome of more thorough processes. |
|
oh, and fix lgtm, 🚢 |
|
@mscdex so it looks like this patch is not applying cleanly to v5.x (although it is for master). Would you be able to take a peak? |
|
LGTM |
|
|
||
| dirname: function dirname(path) { | ||
| assertPath(path); | ||
| if (typeof path !== 'string') |
There was a problem hiding this comment.
We can skip this check and simply do path = '' + path, right?
There was a problem hiding this comment.
It probably doesn't matter either way. I just did it this way in case v8 doesn't/can't optimize it by not creating a new string if path is already a string.
There was a problem hiding this comment.
Just thinking out loud here. Will path = path.toString() be better then?
There was a problem hiding this comment.
That's effectively the same as path = '' + path but less safe (e.g. won't work on null and other values).
There was a problem hiding this comment.
Oh yeah. Totally forgot about them.
|
LGTM |
Before b212be0, input types were not checked in some path functions and the inputs were passed directly to `regexp.exec()` which implicitly converts its argument to a string. This commit both removes the type checking added in b212be0 and adds string coercion for those functions. PR-URL: #5244 Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
|
Landed in 9209bf6. |
|
@rvagg Do you want me to submit a separate PR for inclusion into v5.x or will it get cherry-picked at some point (and should be tagged with |
Before b212be0, input types were not checked in some path functions and the inputs were passed directly to `regexp.exec()` which implicitly converts its argument to a string. This commit both removes the type checking added in b212be0 and adds string coercion for those functions. PR-URL: #5244 Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
5 participants
Before b212be0, input types were not checked in some path functions and the inputs were passed directly to
regexp.exec()which implicitly converts its argument to a string.This commit both removes the type checking added in b212be0 and adds string coercion for those functions.
/cc @thealphanerd