Skip to content

[11.x] make password resets use the "cache driver" by default #6487

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

Closed
wants to merge 1 commit into from

Conversation

browner12
Copy link
Contributor

my intent with laravel/framework#53428 was to make the Cache driver the default for password resets, as I think it's the better option. if on board, this should not be merged until that PR is released.

wondering if we should add a dedicated cache store by default?

  • drop the password_reset_tokens migration.
  • update the auth config to use the 'cache' driver by default

- drop the `password_reset_tokens` migration.
- update the auth config to use the 'cache' driver by default
@Jubeki
Copy link
Contributor

Jubeki commented Nov 8, 2024

I am against this change.

There are many applications who clear the cache in between deployments using cache:clear

As far as I understand it, this will also purge the password reset tokens.

(Or am I misunderstanding how the cache driver works in this case?)

@browner12
Copy link
Contributor Author

you are correct with the default case. which is why I noted in the original PR that I highly recommend creating a dedicated cache "store" so you aren't clearing these.

I'm definitely open to creating an explicit "store" by default, but I was not sure what "driver" we would pick for that.

@Jubeki
Copy link
Contributor

Jubeki commented Nov 8, 2024

I think creating a different store (not exactly sure what that would imply) would be unneeded complexity for most of the Laravel Apps.

You currently only need one sqlite file for creating a new App (cache is also stored in the database). So no need for further complexity if a simple database table is enough for most use cases in my opinion.

@joshmanders
Copy link
Contributor

I think the store property addresses this concern and the only real argument against using it (which is far more superior) is "we have to educate people on the new changes" and that's flaky at best.

@taylorotwell
Copy link
Member

I don't think we should do this.

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