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

Removed LOWER() from email check #14

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mikoczy
Copy link
Contributor

@mikoczy mikoczy commented Mar 29, 2021

No description provided.

@markoph
Copy link
Contributor

markoph commented Mar 29, 2021

@mikoczy please add description to your PR. Why do you need to remove this validation check?

This transformation to lower case was added so we don't register two users with same email address - eg. [email protected] and [email protected]. Gmail (and most of the email providers) is not case sensitive so these emails are actually same account.

If you have valid use case we can move this validation to some kind of validation provider which could be disabled. But we don't want to remove it.

@mikoczy
Copy link
Contributor Author

mikoczy commented Mar 29, 2021

@markoph sorry, it is a follow up to an older pr (#6).. mysql is case insensitive too, so the use case you described, cannot happen... on the other hand, when using LOWER, mysql will not use the email index, so if the users table is big enough, this query can be very slow, timeouts can happen, when adding a user through api..

@markoph
Copy link
Contributor

markoph commented Apr 8, 2021

@mikoczy FYI: your commit was merged internally and changes will be part of next release.

@mikoczy
Copy link
Contributor Author

mikoczy commented Apr 8, 2021

@markoph great, thanks

rootpd pushed a commit that referenced this pull request Apr 12, 2021
MySQL is case insensitive. And when LOWER is used, MySQL will not use
the email index.

#14
@rootpd rootpd force-pushed the master branch 2 times, most recently from 1bb0b05 to 8e046aa Compare July 29, 2022 13:16
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.

2 participants