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

BUG Prevent password validator min score producing false negatives #8013

Conversation

tractorcow
Copy link
Contributor

Replaces #7995

Fixes min score failing if no tests are set
Fixes min score failing if there are no failing tests
Adds unit tests to cover the above
Fix up bad phpdoc, code style, linting, and upgrade deprecated API usage

Copy link
Contributor

@robbieaverill robbieaverill left a comment

Choose a reason for hiding this comment

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

Looks like a great change to me!

@dhensby
Copy link
Contributor

dhensby commented Apr 18, 2018

OK - I assume this is a bug in 4.0 as well as 4.1, can we target 4.0 whilst it's still a supported version?

@tractorcow
Copy link
Contributor Author

Actually 4.0 is safe; We'd refactored PasswordValidator in 4.1, introducing the bug there.

Copy link
Contributor

@maxime-rainville maxime-rainville left a comment

Choose a reason for hiding this comment

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

While this is better than it used to be, I don't think it addresses the fundamental problem that you have TestNames disjointed from Tests.

Althought it might not be possible to address this without doing breaking changes. So it might have to wait until SS5.

@maxime-rainville maxime-rainville removed their assignment Apr 23, 2018
@tractorcow
Copy link
Contributor Author

tractorcow commented Apr 24, 2018

Yeah after I looked at this I realised my error. One is the list of all available tests, the other is the list of all enabled tests. The reason they are separate was correct and intentional.

@chillu
Copy link
Member

chillu commented Apr 29, 2018

@dhensby Are you happy to merge this?

@dhensby
Copy link
Contributor

dhensby commented Apr 30, 2018

I am, but does it fully address the initial bug?

@dhensby dhensby merged commit 62631dc into silverstripe:4.1 Apr 30, 2018
@dhensby dhensby deleted the pulls/4.1/fix-password-validator-fields branch April 30, 2018 11:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants