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

Basic IDN support #1044

Closed
wants to merge 8 commits into from

Conversation

Projects
None yet
4 participants
@c960657
Copy link
Contributor

commented Jan 22, 2018

Q A
Bug fix? no
New feature? yes
Doc update? no
BC breaks? no
Deprecations? no
Fixed tickets #63, #541
License MIT

IDN domains have been around for more than a decade and are now widely used. Swiftmailer should support internationalised email addresses. Even if you don't use an IDN domain yourself, you may need to send email to people who do.

Calling setTo() with a non-ASCII email address does not throw an error (it does throw, if the email address is otherwise malformed). The egulias/EmailValidator package accepts addresses containing non-ASCII characters both in the local-part (the part of the address left of @) and domain (after @). However, when you try to send the email, the SMTP server rejects the address for being syntactically invalid.

Handling non-ASCII characters in local-part and domain are two different issues. Non-ASCII characters in the domain is supported by all email servers, as long as the domain is IDN-encoded by the sender. Non-ASCII characters in local-part requires support for the SMTPUTF8 SMTP extension (see RFC 6531).

This PR adds basic support for email addresses using IDN domains. It does not add support for SMTPUTF8 but simply IDN-encodes the domain, so mail can use the existing infrastructure. The implementation is backwards-compatible fashion.

Future work should add support for SMTPUTF8 in order to support for email addresses with non-ASCII characters in local-part also. This would probably require BC-breaking changes, so it needs to go into a new major release of Swiftmailer.

c960657 added some commits Jan 22, 2018

*/
protected function idnToAscii($string)
{
if (function_exists('idn_to_ascii')) {

This comment has been minimized.

Copy link
@Rotzbua

Rotzbua Jan 22, 2018

Contributor

Needs documentation in doc files and composer.json

This comment has been minimized.

Copy link
@c960657

c960657 Jan 23, 2018

Author Contributor

I suppose https://swiftmailer.symfony.com/download is the best place in the docs to mention this. But I cannot find the source code for this page?

/**
* Creates a new MailboxHeader with $name.
*
* @param string $name of Header
* @param Swift_Mime_HeaderEncoder $encoder
* @param EmailValidator $emailValidator
*/
public function __construct($name, Swift_Mime_HeaderEncoder $encoder, EmailValidator $emailValidator)
public function __construct($name, Swift_Mime_HeaderEncoder $encoder, EmailValidator $emailValidator, Swift_AddressEncoder $addressEncoder = null)

This comment has been minimized.

Copy link
@Rotzbua

Rotzbua Jan 22, 2018

Contributor

Needs documentation

/**
* Creates a new IdentificationHeader with the given $name and $id.
*
* @param string $name
* @param EmailValidator $emailValidator
*/
public function __construct($name, EmailValidator $emailValidator)
public function __construct($name, EmailValidator $emailValidator, Swift_AddressEncoder $addressEncoder = null)

This comment has been minimized.

Copy link
@Rotzbua

Rotzbua Jan 22, 2018

Contributor

Needs documentation

/**
* Creates a new PathHeader with the given $name.
*
* @param string $name
* @param EmailValidator $emailValidator
*/
public function __construct($name, EmailValidator $emailValidator)
public function __construct($name, EmailValidator $emailValidator, Swift_AddressEncoder $addressEncoder = null)

This comment has been minimized.

Copy link
@Rotzbua

Rotzbua Jan 22, 2018

Contributor

Needs documentation

@@ -37,12 +37,13 @@ class Swift_Mime_SimpleHeaderFactory implements Swift_Mime_CharsetObserver
* @param EmailValidator $emailValidator
* @param string|null $charset
*/
public function __construct(Swift_Mime_HeaderEncoder $encoder, Swift_Encoder $paramEncoder, EmailValidator $emailValidator, $charset = null)
public function __construct(Swift_Mime_HeaderEncoder $encoder, Swift_Encoder $paramEncoder, EmailValidator $emailValidator, $charset = null, Swift_AddressEncoder $addressEncoder = null)

This comment has been minimized.

Copy link
@Rotzbua

Rotzbua Jan 22, 2018

Contributor

Needs documentation

*
* @throws Swift_AddressEncoderException If local-part contains non-ASCII characters
*/
public function encodeString($address)

This comment has been minimized.

Copy link
@Rotzbua

Rotzbua Jan 22, 2018

Contributor

php7 typehints can be used

* @param string $string
* @return string
*/
protected function idnToAscii($string)

This comment has been minimized.

Copy link
@Rotzbua

Rotzbua Jan 22, 2018

Contributor

php7 typehints can be used

return idn_to_ascii($string);
}
if (class_exists('TrueBV\Punycode')) {

This comment has been minimized.

Copy link
@Rotzbua

Rotzbua Jan 22, 2018

Contributor

Needs documentation in doc files and composer.json

This comment has been minimized.

Copy link
@c960657

c960657 Jan 23, 2018

Author Contributor

I added a suggest section to composer.json. I don't know if this is the best way to represent such either/or dependencies?

@@ -50,7 +50,7 @@ public function encodeString($address)
protected function idnToAscii($string)
{
if (function_exists('idn_to_ascii')) {
return idn_to_ascii($string);
return idn_to_ascii($string, INTL_IDNA_VARIANT_UTS46);

This comment has been minimized.

Copy link
@Rotzbua

Rotzbua Jan 22, 2018

Contributor

return idn_to_ascii($string, 0, INTL_IDNA_VARIANT_UTS46);

protected function idnToAscii($string)
{
if (function_exists('idn_to_ascii')) {
return idn_to_ascii($string, INTL_IDNA_VARIANT_UTS46);

This comment has been minimized.

Copy link
@Rotzbua

Rotzbua Jan 22, 2018

Contributor

return idn_to_ascii($string, 0, INTL_IDNA_VARIANT_UTS46);

c960657 added some commits Jan 23, 2018

@fabpot
Copy link
Member

left a comment

Looks great! Thanks for working on this. I've made only minor comments. Can you also add a note in the CHANGELOG and bump the version to the next minor one?

/**
* Email address encoder.
*
* @author Chris Corbyn

This comment has been minimized.

Copy link
@fabpot

fabpot Jan 23, 2018

Member

probably not :)

/**
* Encodes an email address.
*
* @param string $address

This comment has been minimized.

Copy link
@fabpot

fabpot Jan 23, 2018

Member

Can be removed. Let's not add phpdocs when not needed, same below.

throw new Swift_SwiftException('No IDN encoder found (install the intl extension or the true/punycode package');
}
}

This comment has been minimized.

Copy link
@fabpot

fabpot Jan 23, 2018

Member

extra blank line

*/
public function encodeString(string $address): string;
}

This comment has been minimized.

Copy link
@fabpot

fabpot Jan 23, 2018

Member

extra blank line

* The address encoder.
*
* @var Swift_AddressEncoder
*/

This comment has been minimized.

Copy link
@fabpot

fabpot Jan 23, 2018

Member

phpdoc can be removed

This comment has been minimized.

Copy link
@Rotzbua

Rotzbua Jan 23, 2018

Contributor

@fabpot but the type should stay. phpstorm use this for autocompletion :)

This comment has been minimized.

Copy link
@fabpot

fabpot Jan 23, 2018

Member

I don't use phpstorm, but IIRC, I think that the type hint in the constructor and the assignment is enough. No need to repeat it here.

@@ -27,6 +27,9 @@
/** The event dispatching layer */
protected $eventDispatcher;
/** The address encoder */

This comment has been minimized.

Copy link
@fabpot

fabpot Jan 23, 2018

Member

I think it's not needed, var name is clear enough.

*
* @param string $message
*/
public function __construct(string $address, string $message)

This comment has been minimized.

Copy link
@z38

z38 Jan 24, 2018

It seems that $this->address does not get filled in.

$domain = substr($address, $i + 1);
if (preg_match('/[^\x00-\x7F]/', $local)) {
throw new Swift_AddressEncoderException('Non-ASCII characters not supported in local-part', $address);

This comment has been minimized.

Copy link
@z38

z38 Jan 24, 2018

The arguments probably need to be the other way round (or in the constructor)

@c960657

This comment has been minimized.

Copy link
Contributor Author

commented Jan 24, 2018

I have removed some redundant PHPdocs, swapped the parameter order for Swift_AddressEncoderException, and added a changelog entry.

@fabpot

This comment has been minimized.

Copy link
Member

commented Jan 24, 2018

Thank you @c960657.

@fabpot fabpot closed this Jan 24, 2018

fabpot added a commit that referenced this pull request Jan 24, 2018

feature #1044 Basic IDN support (c960657)
This PR was squashed before being merged into the 6.0-dev branch (closes #1044).

Discussion
----------

Basic IDN support

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | yes
| Doc update?   | no
| BC breaks?    | no
| Deprecations? | no
| Fixed tickets | #63, #541
| License       | MIT

IDN domains have been around for more than a decade and are now widely used. Swiftmailer should support internationalised email addresses. Even if you don't use an IDN domain yourself, you may need to send email to people who do.

Calling `setTo()` with a non-ASCII email address does not throw an error (it does throw, if the email address is otherwise malformed). The [egulias/EmailValidator](https://github.com/egulias/EmailValidator) package accepts addresses containing non-ASCII characters both in the local-part (the part of the address left of @) and domain (after @). However, when you try to send the email, the SMTP server rejects the address for being syntactically invalid.

Handling non-ASCII characters in local-part and domain are two different issues. Non-ASCII characters in the domain is supported by all email servers, as long as the domain is IDN-encoded by the sender. Non-ASCII characters in local-part requires support for the `SMTPUTF8` SMTP extension (see [RFC 6531](https://tools.ietf.org/html/rfc6531)).

This PR adds basic support for email addresses using IDN domains. It does not add support for `SMTPUTF8` but simply IDN-encodes the domain, so mail can use the existing infrastructure. The implementation is backwards-compatible fashion.

Future work should add support for `SMTPUTF8` in order to support for email addresses with non-ASCII characters in local-part also. This would probably require BC-breaking changes, so it needs to go into a new major release of Swiftmailer.

Commits
-------

6a87efd Basic IDN support

fabpot added a commit that referenced this pull request Jan 29, 2018

bug #1053 Handle address encoder exceptions during send (c960657)
This PR was merged into the 6.1-dev branch.

Discussion
----------

Handle address encoder exceptions during send

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| Doc update?   | no
| BC breaks?    | no
| Deprecations? | no
| Fixed tickets |
| License       | MIT

After working some more with #1044 I noticed some inconsistencies between `SendmailTransport` and `EsmtpTransport`. First of all, I failed to add the address encoder to the constructor of `SendmailTransport`. And secondly, a `Swift_AddressEncoderException` throw in `doRcptToCommand()` because of a bad _recipient_ address was not caught and added to `$failedRecipients`. If the exception is thrown because of a bad sender address, we still throw the exception – I assume this is the expected behaviour.

I modified the other tests in `AbstractSmtpTest` to catch a Swiftmailer-specific exception, so they wont accidentally catch an exception thrown by PHPUnit in the `fail()` method.

Commits
-------

466bdd7 Handle address encoder exceptions during send

@c960657 c960657 referenced this pull request Feb 5, 2018

Closed

Support SMTPUTF8 extension #1056

fabpot added a commit that referenced this pull request Feb 14, 2018

feature #1056 Support SMTPUTF8 extension (c960657)
This PR was squashed before being merged into the 6.1-dev branch (closes #1056).

Discussion
----------

Support SMTPUTF8 extension

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | yes
| Doc update?   | no
| BC breaks?    | no
| Deprecations? | no
| Fixed tickets |
| License       | MIT

#1044 added support for non-ASCII characters in the email domain. This PR adds support for the SMTPUTF8 SMTP extension (see [RFC 6531](https://tools.ietf.org/html/rfc6531)) that allows UTF-8 characters in the local-part also.

This PR does _not_ implement an UTF-8 header encoder (e.g. `Swift_Mime_HeaderEncoder_Utf8HeaderEncoder`) as described in [RFC 6532](https://tools.ietf.org/html/rfc6532). For the purpose of sending to non-ASCII email addresses, the new `Swift_AddressEncoder_Utf8AddressEncoder` works fine with the existing header encoders – `Swift_Mime_Headers_MailboxHeader` does not encode the actual email address but simple passes through whatever is returned by the address encoder.

FWIW, SMTPUTF8 is supported by [Postfix](http://www.postfix.org/SMTPUTF8_README.html), [Gmail](https://googleblog.blogspot.dk/2014/08/a-first-step-toward-more-global-email.html), among others.

Commits
-------

673a626 Support SMTPUTF8 extension
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.