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

Fix the email validator #8531

Merged
merged 2 commits into from Nov 28, 2017

Conversation

Projects
None yet
5 participants
@Azouz-Jribi
Contributor

Azouz-Jribi commented Nov 22, 2017

Questions Answers
Branch? 1.6.1.x
Description? When you activate the one page checkout, you can create an account with wrong email which contains special characters.
Type? bug fix
Category? FO
BC breaks? no
Deprecations? no
Fixed ticket? http://forge.prestashop.com/browse/PSCSX-9282
How to test? BO > Preferences > Orders > set Order process type as One-page checkout, FO > Cart > in the New CUSTOMER block fill the email input with wrong email (example: pubé@prestashop.com) and click save, check if an error message apears

This change is Reviewable

@Quetzacoalt91

Tested and OK. It would be great to have the related unit tests in https://github.com/PrestaShop/PrestaShop/blob/1.6.1.x/tests/Unit/classes/ValidateCoreTest.php

Once done, we can cherry-pick this change on 1.7

@codacy-bot

This comment has been minimized.

Show comment
Hide comment
@codacy-bot

codacy-bot Nov 28, 2017

Codacy Here is an overview of what got changed by this pull request:

Complexity decreasing per file
==============================
+ tests/Unit/classes/ValidateCoreTest.php  -18
+ classes/Validate.php  -120
         

See the complete overview on Codacy

codacy-bot commented Nov 28, 2017

Codacy Here is an overview of what got changed by this pull request:

Complexity decreasing per file
==============================
+ tests/Unit/classes/ValidateCoreTest.php  -18
+ classes/Validate.php  -120
         

See the complete overview on Codacy

@PrestaShop PrestaShop deleted a comment from codacy-bot Nov 28, 2017

@Quetzacoalt91

Perfect 👌

@Quetzacoalt91 Quetzacoalt91 added this to the 1.6.1.18 milestone Nov 28, 2017

@Quetzacoalt91 Quetzacoalt91 merged commit b921c44 into PrestaShop:1.6.1.x Nov 28, 2017

2 checks passed

codacy/pr Good work! A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Quetzacoalt91

This comment has been minimized.

Show comment
Hide comment
@Quetzacoalt91
Member

Quetzacoalt91 commented Nov 28, 2017

Thank you @Azouz-Jribi

@rGaillard

This comment has been minimized.

Show comment
Hide comment
@rGaillard

rGaillard Nov 28, 2017

Member

@Azouz-Jribi @Quetzacoalt91 PrestaShop must support unicode for emails (this is the case prior this PR), this is needed for many countries which are using non standard chars (and accepted by the RFC), eg: http://forge.prestashop.com/browse/PSCFI-4878
You should revert it.

Member

rGaillard commented Nov 28, 2017

@Azouz-Jribi @Quetzacoalt91 PrestaShop must support unicode for emails (this is the case prior this PR), this is needed for many countries which are using non standard chars (and accepted by the RFC), eg: http://forge.prestashop.com/browse/PSCFI-4878
You should revert it.

@Quetzacoalt91

This comment has been minimized.

Show comment
Hide comment
@Quetzacoalt91

Quetzacoalt91 Nov 28, 2017

Member

Well that case was not present, and obviously fails:

1) Tests\Unit\Classes\ValidateCoreTest::testIsEmail with data set #10 (true, 'remi@домен.com.ua')
Failed asserting that false matches expected true.

/home/thomas/Documents/github/PrestaShop/tests/Unit/Classes/ValidateCoreTest.php:58

FAILURES!
Tests: 487, Assertions: 801, Failures: 1, Skipped: 1.
Member

Quetzacoalt91 commented Nov 28, 2017

Well that case was not present, and obviously fails:

1) Tests\Unit\Classes\ValidateCoreTest::testIsEmail with data set #10 (true, 'remi@домен.com.ua')
Failed asserting that false matches expected true.

/home/thomas/Documents/github/PrestaShop/tests/Unit/Classes/ValidateCoreTest.php:58

FAILURES!
Tests: 487, Assertions: 801, Failures: 1, Skipped: 1.
@Azouz-Jribi

This comment has been minimized.

Show comment
Hide comment
@Azouz-Jribi

Azouz-Jribi Nov 28, 2017

Contributor

@Quetzacoalt91
So, we must add all the test cases to support unicode for emails ?
Internationalization examples : https://en.wikipedia.org/wiki/Email_address#Internationalization_examples

Contributor

Azouz-Jribi commented Nov 28, 2017

@Quetzacoalt91
So, we must add all the test cases to support unicode for emails ?
Internationalization examples : https://en.wikipedia.org/wiki/Email_address#Internationalization_examples

@Quetzacoalt91

This comment has been minimized.

Show comment
Hide comment
@Quetzacoalt91

Quetzacoalt91 Nov 28, 2017

Member

Yes, it seems all these example are actually legit.

Member

Quetzacoalt91 commented Nov 28, 2017

Yes, it seems all these example are actually legit.

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