Experimental tidying-up branch #4
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…itten. Issue some deprecation warnings
…tance (and therefore does not validate them). It ought to, and now does
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I'm not really expecting you to accept this, which is completely fine. GAuth is obviously on packagist with people using it (myself gratefully included) who might not appreciate breaking API changes. It's more for discussion.
First of all, I don't understand when the base32 lookup table would ever change, or be changed. So, apart from saving some space, I'm not sure why you'd want to build it on construction, and why you have public getters and settings on it. So I've put it in hard-coded, and put some deprecation errors on the related methods.
Secondly, calling validateCode with the 2nd, 3rd and 4th optional params doesn't set them on the instance, which also means they are not validated. There's a change here to
setand thengetif they are passed, which is what I'd expect them to do. In fact, I'd remove the$initKeyand$rangeparameters completely. Alternatively, the validation could be extracted into separate methods and called there, or a Value Object for the params used (which might be over-engineering, and is a very big API change).Lastly, I've changed the visibility on some of the helper methods because I don't understand when you'd ever call those methods from the outside.
Apologies if my misunderstandings have led to any of these comments/changes. They are meant to be constructive. Also, do please let me know if you'd like any help with other aspects of this - for example, would a more detailed 'how to implement 2FA using this' document be useful?