Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(password): Make password having custom length #235

Merged

Conversation

samijnih
Copy link

@samijnih samijnih commented Mar 5, 2020

Why

Hello :)

I'm currently using your api to make a php cli menu for my app.
I've been facing a problem when I wanted to make a user registration area.

  • I ask to users to type their passwords
    • the password length is hard-coded in the code and it's 16 characters length
    • I don't want to give a closure to custom validator method because I think it's too much effort

So the aim of my pull request is to allow developers like me to call a setter in order to give another length. Of course, I made it backward-compatible so the previous value is a default value if the new method is not called.

Description

  • fix phpdoc type error in password unit test
  • add new internal property about password length with default value int "16"
  • replace hard coded password length comparison with that internal property
  • add a new setter for a custom password length
  • add missing ext-mbstring to composer.json
  • add setValidator to Input interface
  • add tests in PasswordTest

@codecov-io
Copy link

codecov-io commented Mar 5, 2020

Codecov Report

Merging #235 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #235      +/-   ##
============================================
+ Coverage     92.86%   92.87%   +<.01%     
- Complexity      622      623       +1     
============================================
  Files            37       37              
  Lines          1864     1866       +2     
============================================
+ Hits           1731     1733       +2     
  Misses          133      133
Impacted Files Coverage Δ Complexity Δ
src/Input/Password.php 100% <100%> (ø) 15 <1> (+1) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1acf6fc...6f30f2d. Read the comment docs.

@AydinHassan
Copy link
Member

Looks good to me! Only one thing, adding the method setValidator to the interface would be a BC break so I'm not sure we want to do that.

@samijnih
Copy link
Author

samijnih commented Mar 5, 2020

Looks good to me! Only one thing, adding the method setValidator to the interface would be a BC break so I'm not sure we want to do that.

Hey :)

Are you sure about that? because there are only 3 classes which implement the Input interface:

  • Password
  • Text
  • Number

There are all located in namespace PhpSchool\CliMenu\Input.

Also, even if people have extended these classes, the setValidator is already public and inside all the classes. I don't see how it could make a breaking change. May you give me more details? :)

@AydinHassan
Copy link
Member

hey @samijnih if any body is implementing the interface to create their own inputs they might not have the setValidator method. If we add the method to the interface then their custom inputs will break.

@samijnih
Copy link
Author

samijnih commented Mar 5, 2020

hey @samijnih if any body is implementing the interface to create their own inputs they might not have the setValidator method. If we add the method to the interface then their custom inputs will break.

You're right, I'm gonna update it

- fix phpdoc type error in password unit test
- add new internal property about password length with default value int "16"
- replace hard coded password length comparison with that internal property
- add a new setter for a custom password length
- add missing ext-mbstring to composer.json
- add setValidator to Input interface
- add tests in PasswordTest
@samijnih samijnih force-pushed the feat/set-password-input-length branch from aee9d12 to 6f30f2d Compare March 5, 2020 21:13
@AydinHassan AydinHassan merged commit aab3b01 into php-school:master Mar 5, 2020
@samijnih samijnih deleted the feat/set-password-input-length branch March 5, 2020 21:15
@AydinHassan
Copy link
Member

thank you @samijnih !

@samijnih
Copy link
Author

samijnih commented Mar 5, 2020

You're welcome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants