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.

Sign up for GitHub

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

Jump to bottom

[RTM] Fix Validator::isEmail(). #313

Closed
wants to merge 3 commits into from
Closed

[RTM] Fix Validator::isEmail(). #313

wants to merge 3 commits into from

Conversation

discordier
Copy link
Contributor

This PR utilizes the PHP filter_var() method for validating email addresses and fixes a Bug in the Idna class.
For details, refer to contao/core#7783.

This should be backported to 3.5.

@discordier
Copy link
Contributor Author

I have no idea why the unit test is failing, I did NOT tamper with the AutomatorCommandTest:

1) Contao\CoreBundle\Test\Command\AutomatorCommandTest::testOutput
Undefined index: checkForUpdates
/home/travis/build/contao/core-bundle/vendor/symfony/phpunit-bridge/DeprecationErrorHandler.php:40
/home/travis/build/contao/core-bundle/vendor/symfony/console/Question/ChoiceQuestion.php:165
/home/travis/build/contao/core-bundle/vendor/symfony/console/Helper/QuestionHelper.php:391
/home/travis/build/contao/core-bundle/vendor/symfony/console/Helper/QuestionHelper.php:58
/home/travis/build/contao/core-bundle/src/Command/AutomatorCommand.php:161
/home/travis/build/contao/core-bundle/src/Command/AutomatorCommand.php:90
/home/travis/build/contao/core-bundle/src/Command/AutomatorCommand.php:59
/home/travis/build/contao/core-bundle/src/Command/AbstractLockedCommand.php:38
/home/travis/build/contao/core-bundle/vendor/symfony/console/Command/Command.php:259
/home/travis/build/contao/core-bundle/vendor/symfony/console/Tester/CommandTester.php:80
/home/travis/build/contao/core-bundle/tests/Command/AutomatorCommandTest.php:52

EDIT: I did not have my master rebased to upstream. Have done so now but did not solve the issue.

@aschempp
Copy link
Member

I like the idea of using filter_var, though I have zero experience with it…

@Toflar
Copy link
Member

Toflar commented Jul 21, 2015

filter_var is a good choice. 👍

@leofeyer
Copy link
Member

Can you elaborate on your changes to the Idna class please?

@leofeyer leofeyer added the bug label Jul 22, 2015
@leofeyer leofeyer added this to the 4.0.1 milestone Jul 22, 2015
@leofeyer leofeyer mentioned this pull request Jul 22, 2015
Closed
@discordier
Copy link
Contributor Author

The changes in Idna were necessary as it was breaking valid email adresses. I just stumbled upon this bug while writing unit tests before I started implementing any validation.

Imagine the valid address: "mycool@ddress"@example.com.
This was broken into chunks by the @ and the second one was idna encoded and then the first and the second were recombined.
Here this would be: "mycool, ddress" and example.com and therefore resulting in: "mycool@ddress" which is plain wrong as it should recombine to "mycool@ddress"@example.com.
I have altered it to only encode the last chunk (which must be the host or domain name in emails) and recombine all chunks later on.

For details, have a look at this unit test '"tes@t"@wähwähwäh.ümläüts.de' which covers the same subject including a real idna domain name. The previous handling would have simply returned "tes@t" as encoded address.

The only thing is, that it does also accept clearly broken e-mails for encoding but it was doing so before and coders will most likely call Validator::isEmail() anyway before accepting an address whereas the addresses getting encoded will most likely have been validated already.

@leofeyer
Copy link
Member

Merged in e873076.

@leofeyer leofeyer closed this Jul 23, 2015
leofeyer added a commit to contao/core that referenced this pull request Jul 23, 2015
@leofeyer
Fix the Validator::isEmail() method (see contao/core-bundle#313).
ad169aa
@leofeyer
Copy link
Member

Back ported in contao/core@ad169aa.

@leofeyer leofeyer self-assigned this Jul 23, 2015
leofeyer added a commit to contao/core that referenced this pull request Jul 24, 2015
@leofeyer
Fix the Validator::isEmail() method (see contao/core-bundle#313).
cdf90c4
jsonn pushed a commit to jsonn/pkgsrc that referenced this pull request Jul 27, 2015
Update contao35 to 3.5.2.
77497d9 
* Add Serbian language files.

Version 3.5.2 (2015-07-24)
--------------------------

### Fixed
Revert some of the PhpStorm code inspector changes (see #7937).


Version 3.5.1 (2015-07-24)
--------------------------

### Fixed
Add a `StringUtil` class to restore PHP 7 compatibility (see contao/core-bundle#309).

### Fixed
Fix the `Validator::isEmail()` method (see contao/core-bundle#313).

### Fixed
Strip tags before auto-generating aliases (see #7857).

### Fixed
Correctly encode the URLs in the popup file manager (see #7929).

### Fixed
Check for the comments module when compiling the news meta fields (see #7901).

### Fixed
Also sort the newsletter channels alphabetically in the front end (see #7864).

### Fixed
Disable responsive images in the back end preview (see #7875).

### Fixed
Overwrite the request string when generating news/event feeds (see #7756).

### Fixed
Store the static URLs with the cached file (see #7914).

### Fixed
Correctly check the subfolders in the `hasAccess()` method (see #7920).

### Fixed
Updated the countries list (see #7918).

### Fixed
Respect the `notSortable` flag in the parent (see #7902).

### Fixed
Round the maximum upload size to an integer value (see #7880).

### Fixed
Make the markup minification less aggressive (see #7734).

### Fixed
Filter the indices in `Database::getFieldNames()` (see #7869).

### Fixed
Back-ported two fixes from the upstream versions.
@leofeyer leofeyer mentioned this pull request Jul 29, 2015
Closed
leofeyer added a commit to contao/core that referenced this pull request Aug 25, 2015
@leofeyer
Fix the Validator::isEmail() method (see contao/core-bundle#313).
976bf6e
@discordier discordier deleted the fix/validator-is-email branch July 19, 2016 17:43
@leofeyer leofeyer modified the milestones: 4.0.3, 4.0 May 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@leofeyer leofeyer

Labels
bug
Projects
None yet
Milestone
4.0
Development

Successfully merging this pull request may close these issues.
4 participants
@discordier @aschempp @Toflar @leofeyer