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

php 8.1 support #60

Merged
merged 19 commits into from
Dec 13, 2021
Merged

php 8.1 support #60

merged 19 commits into from
Dec 13, 2021

Conversation

Xon
Copy link
Contributor

@Xon Xon commented Dec 4, 2021

I maintain Password Tools, a free opensource add-on for XenForo forum software which uses this library to offer useful password complexity checks.

As part of updating my code to support php 8.1, this library was identified as having compatibility issues.

This change does the following;

  • Document a test case which fails on php 7.2/7.3/7.4/8.0 as-is
  • Apply php 7.2+ compatible type hinting to drive discovery of bad type handling
    • getGuesses was documented as returning int but could sometimes return a float. This behaviour was corrected, which required adjusting some tests. I failed to see how it returning a float value is very useful.
  • Use the null coalescing operator in a few spots to cleanup the code, and then pick non-null defaults which work with the increasingly type-strict php
  • A number of php 8.1 compatibility changes, mostly around using null on internal functions which no longer permit it

@Xon
Copy link
Contributor Author

Xon commented Dec 4, 2021

This looks to have ended up including #59's change

Copy link
Collaborator

@mkopinsky mkopinsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an awesome set of changes. The only thing I'm hesitant about is the int/float thing since it's changing behavior, but I think it's fine (except for the one specific thing I commented on for values between PHP_INT_MAX and PHP_FLOAT_MAX).

src/Matchers/BaseMatch.php Outdated Show resolved Hide resolved
src/Matchers/Bruteforce.php Outdated Show resolved Hide resolved
src/Matchers/DictionaryMatch.php Outdated Show resolved Hide resolved
test/ZxcvbnTest.php Outdated Show resolved Hide resolved
@Xon
Copy link
Contributor Author

Xon commented Dec 6, 2021

This is an awesome set of changes. The only thing I'm hesitant about is the int/float thing since it's changing behavior, but I think it's fine (except for the one specific thing I commented on for values between PHP_INT_MAX and PHP_FLOAT_MAX).

Bruteforce and SpatialMatch are the only two which return floats. The PHP_INT_MAX behaviour was a good catch which I overlooked, so I've checked a few other locations that got touched.

@Xon Xon requested a review from mkopinsky December 6, 2021 17:01
@Xon
Copy link
Contributor Author

Xon commented Dec 6, 2021

I think I might need to walk back those int => float typing. Looking at the readme;

Scores are integers from 0 to 4:
0 means the password is extremely guessable (within 10^3 guesses), dictionary words like 'password' or 'mother' score a 0
1 is still very guessable (guesses < 10^6), an extra character on a dictionary word can score a 1
2 is somewhat guessable (guesses < 10^8), provides some protection from unthrottled online attacks
3 is safely unguessable (guesses < 10^10), offers moderate protection from offline slow-hash scenario
4 is very unguessable (guesses >= 10^10) and provides strong protection from offline slow-hash scenario

32bit php can't represent 10^10 as an integer, while 64bit php can.

…isons which may vary depending on php compiling settings and platform
@Zenexer
Copy link
Contributor

Zenexer commented Dec 6, 2021

I've got some additional fixes related to PHP 8.1 that I'll be adding to this PR later today. For the sake of speeding up the changes, I've also added strict_types=1 to each file and replaced most instances of assertEquals with assertSame. Hopefully whatever bug was causing that odd discrepancy in testing will be resolved in the process.

@Xon
Copy link
Contributor Author

Xon commented Dec 9, 2021

I think this is ready for review after the last round of fixes

@Zenexer
Copy link
Contributor

Zenexer commented Dec 9, 2021

I can confirm that this seems to be working fine in production.

@mkopinsky mkopinsky merged commit db3256f into bjeavons:master Dec 13, 2021
@mkopinsky
Copy link
Collaborator

Thanks all! This is merged and tagged as 1.3.0.

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