mirror of
https://github.com/antonioribeiro/google2fa.git
synced 2026-04-26 16:45:49 +03:00
[GH-ISSUE #130] Suggestion - stricter returns #60
Labels
No labels
bug
pull-request
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
starred/google2fa#60
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Originally created by @ghost on GitHub (Oct 18, 2019).
Original GitHub issue: https://github.com/antonioribeiro/google2fa/issues/130
Just some food for thought!
Wouldn't make more sense to avoid mixed returns? E.g. bool|int when using verifyKey
And is this correct? https://github.com/antonioribeiro/google2fa/blob/master/src/Google2FA.php#L54 (int $startingTimestamp)
I found it when phpstan was giving me a lot of errors because I was treating verifyKey as return bool.
@antonioribeiro commented on GitHub (Oct 21, 2019):
Should be fixed in 6.1.1. @Khyber3737 would you mind testing it again and closing this?
Thanks for reporting!
@ghost commented on GitHub (Oct 21, 2019):
I have verified that with
--level 4there are no errors. With--level maxI get the usual errors that, from a library standpoint (not targeting >= 7.4, strict), makes perfect sense since it's just casting/coersion or a design decision. In hindsight since this library is used everywhere and is battle tested it would be difficult to change methods like findValidOTP and verifyKey. I will deal with that in a more proper opinionated layer.Thank you! :)
@antonioribeiro commented on GitHub (Oct 21, 2019):
Yeah, I tested it with level 4, sorry. I'm not really a PHPStan user, so I didn't know there were levels above it :/
@antonioribeiro commented on GitHub (Oct 21, 2019):
@Khyber3737 Would you please test 6.1.2, please? I think I managed to make them all pass. I had to make one change in code, but it should not break things. It doesn't make sense let people pass $secret as null here:
@ghost commented on GitHub (Oct 21, 2019):
👍
@antonioribeiro commented on GitHub (Oct 21, 2019):
It's now on 7.0.0. I ended up causing a BC break, so I had to roll them back and draft a new major release.