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

Deprecate the password rule and use illuminate password rule #511

Merged
merged 1 commit into from
Dec 11, 2023

Conversation

ricklambrechts
Copy link
Contributor

Created a new PR with the feedback of Dries onto #510

This will deprecate the Fortify password rule and use the Illuminate\Validation\Rules\Password rule.

The Illuminate\Validation\Rules\Password rule has more functionality than the Fortify rule. For example this rule has mixed case and compromised check available.

@taylorotwell taylorotwell merged commit 1dde858 into laravel:1.x Dec 11, 2023
14 checks passed
@ricklambrechts ricklambrechts deleted the deprecate-password-rule branch December 11, 2023 16:23
@onlime
Copy link

onlime commented Dec 18, 2023

Hi @ricklambrechts I very much favor deprecating any duplicate code and I was always wondering why Laravel\Fortify\Rules\Password was ever created. The problem though is, that Illuminate\Validation\Rules\Password offers more features like the compromised check, but provides less detailed validation messages. So, there is no feature parity.

Look at this example, when validating password "password" with below rules:

Laravel\Fortify\Rules\Password:

The password must be at least 10 characters and contain at least one uppercase character and one number.

vs. Illuminate\Validation\Rules\Password:

The string must be at least 10 characters.

Personally, I was always using a combination (composition rule) of both like this, for exactly that reason:

use Illuminate\Validation\Rules\Password;
use Laravel\Fortify\Rules\Password as FortifyPassword;

class PasswordStrict extends CompositeRule
{
    protected function rules(): array
    {
        return [
            'string',
            'max:150',
            // Fortify Password rule: provides more specific validation error messages
            // same as: Password::min(10)->mixedCase()->numbers()
            (new FortifyPassword)
                ->length(10)
                ->requireUppercase()
                ->requireNumeric(),
            // Illuminate Password rule: simpler individual error messages, but provides uncompromised()
            // see Password::defaults() in AppServiceProvider::boot() method
            Password::defaults(),
            new ZxcvbnRule,
            'confirmed',
        ];
    }

For better UX, I totally need such compound validation messages like in Fortify's Password rule. But I understand it's quite tricky to build them (see Password.php#L85-L139).

How should we proceed? PR that feature from Fortify into Illuminate Password?

@driesvints
Copy link
Member

Hi @onlime. Sorry about that. We've decided to leave things be for now, sorry.

@onlime
Copy link

onlime commented Dec 19, 2023

Hi @onlime. Sorry about that. We've decided to leave things be for now, sorry.

What does that mean, removing the @deprecated again?

@driesvints
Copy link
Member

No, we're not going to make additional changes sorry.

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.

None yet

4 participants