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

Remove deprecated password validation rule #39030

Merged
merged 1 commit into from
Sep 29, 2021

Conversation

netpok
Copy link
Contributor

@netpok netpok commented Sep 29, 2021

In #37650 the password rule was renamed to current_password with the intention of removing the old rule in the next major version. This PR removes it.

The original reasoning behind this was that later password could be used as an alias for the new Password::defaults() while I'm still supporting the idea I'm not sure whether it should be done in the next version or in a later one.

The problem with implementing it right away is that it can pass for the provided password even if it's not valid for developers who does not follow the upgrade guide. By waiting a few minor or even a major version we could prevent security issues because it would simply throw an exception.

If it should be included in this version I'm willing to create a pull request for it.

@driesvints
Copy link
Member

Feel free to ignore the failing PHP 8.1 tests for now.

Btw, shouldn't we update the tests to use the current_password rule instead?

@netpok
Copy link
Contributor Author

netpok commented Sep 29, 2021

The current_password rule already have the same tests at line 749

@Jubeki
Copy link
Contributor

Jubeki commented Sep 29, 2021

I would support aliasing the new Password::defaults() rule to password in Laravel 9.

What about adding an validation config where you can configure the Password rule?

@driesvints
Copy link
Member

@netpok if you rebase with master test should pass now

@netpok netpok force-pushed the remove-password-validation branch from 3e2ef32 to 1f54941 Compare September 29, 2021 12:19
@netpok
Copy link
Contributor Author

netpok commented Sep 29, 2021

Rebased it


@Jubeki what do you mean by adding a validation config?

  1. If you mean to support the old or the new behavior I'm against that, developers should just simply migrate.
  2. If you mean to configure the default password rule you can already do that by using Password::default, adding a new config option (and file) is out of the scope for this PR.

@taylorotwell taylorotwell merged commit d6fd27c into laravel:master Sep 29, 2021
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.

4 participants