Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
Password Strength estimator extraction to dedicated service (no Backward Compatibility)#54882
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
…matorInterface to be reused outside
| /** @dataProvider getPasswords */ | ||
| public function testEstimateStrength(string|Stringable $password, int $expectedStrength): void | ||
| { | ||
| self::assertEquals($expectedStrength, (new PasswordStrengthEstimator())->estimateStrength($password)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
AsserSame?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Updated
yalit commentedMay 11, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I updated the title and the comment as I got the comment (which I made to myself also) that BC could be "Backward Compatibility" (the case here) or Breaking Change. This change is, according to me, breaking the backward compatibility as changing the Validator constructor signature and so I would not merge it in the 7.2 but only in the 8.0 (hence the creation of the#54881 to do the same but without breaking the compatibility) |
smnandre commentedMay 11, 2024
If this one does not ensure a Backward compatible way, let close it and work on the other one, no ? |
yalit commentedMay 12, 2024
I would indeed work currently on the other one but how to keep in mind that this one should be potentially meant to be the target one for the 8.0? |
xabbuh commentedMay 12, 2024
For every BC breaking change in 8.0 there must be a deprecation in Symfony 7 first. I haven’t looked into your changes yet, but the comments sound like the deprecation should then be part of your other PR. |
yalit commentedMay 12, 2024
@xabbuh thanks for the notification => I've been advised to do it indeed in the other change (pushed it just now). |
xabbuh commentedMay 12, 2024
👍 closing here then |
…trength()` public (yalit)This PR was merged into the 7.2 branch.Discussion----------[Validator] Make `PasswordStrengthValidator::estimateStrength()` public| Q | A| ------------- | ---| Branch? | 7.1| Bug fix? | no| New feature? | yes (allows for externally| Deprecations? | no| Issues | na| License | MITLinked to the PasswordStrength Constraint, the need is to be able to get the "strength" of a password and not just only if it's strong enough. For instance, the need is to show a "score" bar in the frontend.Currently, the strength is being computed by a private function within the PasswordStrengthValidator not accessible directly. The proposed change is to extract that computation in a specific `PasswordStrengthEstimator` service for it to be accessible from the "external" world.The best way to do so would be to inject the service in the Validator constructor but doing that it would break the compatibility as the current constructor is receiving a Closure to allow for self-strength computation.So here the service is called from within the Validator directly and so maintaining the Backward CompatibilityI created the same PR but with no Backward Compatibility (service injected in the constructor) here :#54882Commits-------e058d92 [Validator] Make `PasswordStrengthValidator::estimateStrength()` public
Uh oh!
There was an error while loading.Please reload this page.
Linked to the PasswordStrength Constraint, the need is to be able to get the "strength" of a password and not just only if it's strong enough. For instance, the need is to show a "score" bar in the frontend.
Currently, the strength is being computed by a private function within the PasswordStrengthValidator not accessible directly. The proposed change is to extract that computation in a specific
PasswordStrengthEstimatorservice for it to be accessible from the "external" world.This PR is proposing a no Backward Ccompatibility version with changing the constructor of the Validator. However, I created the same PR but without BC here :#54881