Skip to content

Conversation

@mapeveri
Copy link
Contributor

Q A
Is bugfix? ✔️
New feature?
Breaks BC?
Fixed issues #2

@samdark samdark added status:code review The pull request needs review. type:bug Bug labels Nov 22, 2020
@samdark
Copy link
Member

samdark commented Nov 22, 2020

It seems to break tests: https://github.com/yiisoft/validator/runs/1438130363

@samdark samdark added status:under development Someone is working on a pull request. and removed status:code review The pull request needs review. labels Nov 22, 2020
@mapeveri
Copy link
Contributor Author

@samdark Yep, the problem is with this emails:

image

I commeted the email with problems in the test. Any idea why this email passed the filter_var?

Thanks

@samdark
Copy link
Member

samdark commented Nov 23, 2020

@mapeveri seems that filter_var isn't suitable for the purpose: https://3v4l.org/4oslP

@samdark
Copy link
Member

samdark commented Nov 23, 2020

filter_var is RFC 5321 and we need way stricter validation.

@mapeveri
Copy link
Contributor Author

@samdark What validation do you recommend?

@samdark
Copy link
Member

samdark commented Nov 23, 2020

Regular expressions. Simplistic ones.

@mapeveri
Copy link
Contributor Author

@samdark thanks. I updated this PR :)

}

if ($this->enableIDN && $valid === false) {
$pattern = '/^([a-zA-Z0-9._%+-]+)@((\[[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.)|(([\w-]+\.)+))([a-zA-Z]{2,4}|[0-9]{1,3})(\]?)$/';
Copy link
Member

Choose a reason for hiding this comment

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

That could be moved to a customizable property same as other regex patterns.

Copy link
Contributor Author

@mapeveri mapeveri Nov 24, 2020

Choose a reason for hiding this comment

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

@samdark there is a private pattern regex, maybe a new property $patternIdnEmail as a private property, is ok?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@samdark i upated the PR ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@samdark is it ok?

Copy link
Member

Choose a reason for hiding this comment

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

Yes.

@samdark samdark added status:code review The pull request needs review. and removed status:under development Someone is working on a pull request. labels Nov 26, 2020
@samdark samdark merged commit 6f2e4f7 into yiisoft:master Nov 26, 2020
@samdark
Copy link
Member

samdark commented Nov 26, 2020

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status:code review The pull request needs review. type:bug Bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants