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

Added new custom email validator #90

Merged
merged 3 commits into from Dec 16, 2022
Merged

Conversation

rishabhgrg
Copy link
Contributor

refs https://github.com/TryGhost/Team/issues/2235

The validator package used in our codebase is stuck on an old version due to some constraints. The isEmail check on the old version is unable to detect some invalid email addresses causing them to sneak through and get stored. This change adds a custom isEmail validator picked from the latest version of validator package, and allows us to gradually update the email checks to use the new version so we can prevent invalid email addresses to enter the system.

refs https://github.com/TryGhost/Team/issues/2235

The `validator` package used in our codebase is stuck on an old version due to some constraints. The `isEmail` check on the old version is unable to detect some invalid email addresses causing them to sneak through and get stored.
This change adds a custom `isEmail` validator picked from the latest version of `validator` package, and allows us to gradually update the email checks to use the new version so we can prevent invalid email addresses to enter the system.
validators.isEmail = function isEmail(str, options = {}) {
assertString(str);
// Use the latest email validator if the option is set
if (options?.latest) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ErisDS The only thing i'd like to get a sense check on this PR is naming for the option that determines if the new email validator should be used instead of default, have named it as latest to denote using the latest version of email validator.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to leave an issue open to clean this up, so we go through and change all the existing calls to explicitly declare they use the old version.

So I'd probably reverse this concept and have the option be called legacy and have it default to true?

So your call will do isEmail('xxx', {legacy: false}).

We can then later change the default to be legacy: false which makes more sense long term?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup that's way better, I wasn't warming up to latest! 🙏 I'll clean this up now and add an issue for us to clean up legacy usage across codebase in future.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

@rishabhgrg rishabhgrg merged commit 8af2f47 into main Dec 16, 2022
@rishabhgrg rishabhgrg deleted the added-new-email-validator branch December 16, 2022 09:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants