Fix translation of expires_in to Unix timestamp #320
Conversation
Also some clarification in docblocks, clearer function name
| { | ||
| if (!is_numeric($expires)) { | ||
| if (!is_numeric($expiresIn)) { |
There was a problem hiding this comment.
Unrelated to your PR, but it may be feasible to use ctype_digit() here rather than is_numeric(). According to https://tools.ietf.org/html/rfc6749#section-5.1 , this field contains the number of seconds so is_numeric() may allow a bit more than we would want.
There was a problem hiding this comment.
Again, I actually just wanted to remove this function.
I don't think Exact would ever return anything other than an int. And if they do, it's a horrible bug on their side which you shouldn't attempt to handle IMO.
There was a problem hiding this comment.
At the edges of the interface of your application, you want to ensure as much as possible that you're getting input you can work with. Never trust any input imho, be it from a user or an API.
It's like crossing a street when your traffic light becomes green without checking first. You don't think a car would ever run a red light, and even if they do, it's a horrible mistake on their side. And technically, you are right. But you are still the one with the problem :)
Anyway, probably the package maintainer will have an opinion too. I just happened to come across this PR because I am in the process of using this package, and I thought your issue was a good catch 👍
There was a problem hiding this comment.
Although on a lot of other places we do rely on what Exact returns as documented, I think in this case ctype_digit is a better check. Could you please change the check? I'd love to have that improvement within this PR.
There was a problem hiding this comment.
Well, since the + operator is used on it, anything invalid and potentially dangerous would still just yield an int or a float and therefore be pretty much harmless, but I digress.
|
Very good catch, thank you! |
| private function getDateTimeFromExpires($expires) | ||
| /** | ||
| * Translates expires_in to a Unix timestamp. | ||
| * @param int $expiresIn Number of seconds until the token expires. |
There was a problem hiding this comment.
Sorry, slight nuance - the value returned in the JSON is a string (which is also what ctype_digit() needs, as feeding it ints may yield unexpected results; see http://php.net/manual/en/function.ctype-digit.php#example-6311)
Also some clarification in docblocks, clearer function name
Fixes #319