Allow long email addresses per rfc 5322 #8

denis-sokolov · 2015-06-10T08:56:38Z

The addresses are not only theoretical,
I have recently received emails from Trello and Peerby that exceed the 64 octet limit for the local part already.

Maks3w · 2015-06-10T09:33:52Z

src/EmailAddress.php

+        $this->hostname = $this->idnToUtf8($this->hostname);
+
+        // https://tools.ietf.org/html/rfc5322#section-2.1.1
+        if ((strlen($this->localPart) + strlen($this->hostname)) > 994) {


I guess @ sign should be included to the length too.

I actually subtracted 4 from 998 to account for To: and @, but did not document it properly.
Thanks for noticing.

Maks3w · 2015-06-10T09:49:32Z

@denis-sokolov The length of 998 is not applicable here. It's just the maximum line length in the SMTP protocol and it's not applicable to email address. (In fact there is no any length limit in the content, just fold the header in multiple lines and content is allowed to exceed 998 characters)

Please choose a better RFC as reference, if you can't find a reference just remove the local part constraint.

Maks3w · 2015-06-10T09:51:49Z

test/EmailAddressTest.php

+            // implementation techniques that impose no limits on the
+            // length of these objects should be used.".
+            // http://tools.ietf.org/html/rfc5321#section-4.5.3.1
+            str_repeat('BobJones', 120).'@domain.com'


Instead use 'line length MM' => str_repeat('x', NN) . '@x',

NN will be make more clear what is the expected length (MM)

Thanks, updating.

denis-sokolov · 2015-06-10T09:56:05Z

Thanks for the correction, @Maks3w.

denis-sokolov · 2015-06-10T10:07:12Z

Finally, a passing test! Sorry for the noise - it passed locally initially, because I had no idn extension.

Please disregard the coveralls check, it's dumb. I removed 4 covered lines and the overall coverage now "fell". Stupid algorithm is stupid.

Maks3w · 2015-06-10T10:13:53Z

@denis-sokolov Please read my comment above. Your length limit assumption is wrong. RFC 5321 is not a reference for this.

denis-sokolov · 2015-06-10T10:20:53Z

@Maks3w, I did read it, it was helpful, thank you.
I think you were referring to the 998 limit of RFC 5322 that I did remove from the PR.

The remaining PR is about RFC 5321 (Internet Message Format) stating "To the maximum extent possible, implementation techniques that impose no limits on the limits of these objects should be used.".

Maks3w · 2015-06-10T10:28:01Z

src/EmailAddress.php

@@ -32,8 +31,7 @@ class EmailAddress extends AbstractValidator
        self::INVALID_SEGMENT    => "'%hostname%' is not in a routable network segment. The email address should not be resolved from public network",
        self::DOT_ATOM           => "'%localPart%' can not be matched against dot-atom format",
        self::QUOTED_STRING      => "'%localPart%' can not be matched against quoted-string format",
-        self::INVALID_LOCAL_PART => "'%localPart%' is not a valid local part for the email address",
-        self::LENGTH_EXCEEDED    => "The input exceeds the allowed length",
+        self::INVALID_LOCAL_PART => "'%localPart%' is not a valid local part for the email address"


Preserve the trailing comma

Maks3w · 2015-06-10T10:32:37Z

Yes. I was talking about the 998 line length limit

Maks3w · 2015-06-10T10:38:46Z

right now I have a couple of concerns:

Seems we have two options of perform a validation. A relaxed one with length limits removal and strict one with the recommended length limits.
The IDN to UTF-8 failure is enough for mark the address as invalid. Otherwise please open a bug ticket on PHP bug tracker.

So disable length limit changes should be a validator option.

denis-sokolov · 2015-06-10T10:52:13Z

Seems we have two options of perform a validation. A relaxed one with length limits removal and strict one with the recommended length limits.

The distinction between strict and lax validation is important and is unfortunate.
I am using Zend mail module to parse emails, and that's the use case when I need lax validation.
The documentation seems to suggest it's a normal use case.
If I was to send emails, I would expect the validation to be strict.

The IDN to UTF-8 failure is enough for mark the address as invalid. Otherwise please open a bug ticket on PHP bug tracker.

The same class a few lines above already uses the fallback:

return (idn_to_ascii($email) ?: $email);

If anything, the new change ensures consistency.

Maks3w · 2015-06-10T11:34:44Z

I was suspecting this come from Zend\Mail :)

I think this is almost done.

Please add a option named strict => true for toggle on/off length validation.

Then we can create a PR against Zend\Mail turning the option to false.

denis-sokolov · 2015-06-10T13:15:41Z

Done, @Maks3w.

Allow long email addresses per rfc 5322

Close #8

Forward port #8

denis-sokolov force-pushed the long-email branch 5 times, most recently from dc2dbd9 to 154dc03 Compare June 10, 2015 09:33

Maks3w reviewed Jun 10, 2015
View reviewed changes

denis-sokolov force-pushed the long-email branch 2 times, most recently from a767c9e to c593863 Compare June 10, 2015 09:49

Maks3w added bug awaiting author updates labels Jun 10, 2015

Maks3w reviewed Jun 10, 2015
View reviewed changes

denis-sokolov force-pushed the long-email branch 2 times, most recently from e071c29 to 38d2097 Compare June 10, 2015 09:55

denis-sokolov force-pushed the long-email branch from 38d2097 to a451313 Compare June 10, 2015 10:00

Maks3w reviewed Jun 10, 2015
View reviewed changes

denis-sokolov force-pushed the long-email branch from a451313 to e42c180 Compare June 10, 2015 10:38

Maks3w added this to the 2.5.2 milestone Jun 10, 2015

denis-sokolov force-pushed the long-email branch from e42c180 to db59fcd Compare June 10, 2015 12:54

Allow long email addresses per rfc 5321

b55aaa0

denis-sokolov force-pushed the long-email branch from db59fcd to b55aaa0 Compare June 10, 2015 12:59

weierophinney removed the awaiting author updates label Jun 23, 2015

weierophinney merged commit b55aaa0 into zendframework:master Jun 23, 2015

weierophinney added a commit that referenced this pull request Jun 23, 2015

Merge pull request #8 from SlikNL/long-email

1ebde6c

Allow long email addresses per rfc 5322

weierophinney added a commit that referenced this pull request Jun 23, 2015

Added CHANGELOG for #8

a29a5ff

weierophinney added a commit that referenced this pull request Jun 23, 2015

Merge branch 'hotfix/8'

a1054be

Close #8

weierophinney added a commit that referenced this pull request Jun 23, 2015

Merge branch 'hotfix/8' into develop

fb139be

Forward port #8

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow long email addresses per rfc 5322 #8

Allow long email addresses per rfc 5322 #8

denis-sokolov commented Jun 10, 2015

Maks3w Jun 10, 2015

denis-sokolov Jun 10, 2015

Maks3w commented Jun 10, 2015

Maks3w Jun 10, 2015

Maks3w Jun 10, 2015

denis-sokolov Jun 10, 2015

denis-sokolov commented Jun 10, 2015

denis-sokolov commented Jun 10, 2015

Maks3w commented Jun 10, 2015

denis-sokolov commented Jun 10, 2015

Maks3w Jun 10, 2015

Maks3w commented Jun 10, 2015

Maks3w commented Jun 10, 2015

denis-sokolov commented Jun 10, 2015

Maks3w commented Jun 10, 2015

denis-sokolov commented Jun 10, 2015

Allow long email addresses per rfc 5322 #8

Allow long email addresses per rfc 5322 #8

Conversation

denis-sokolov commented Jun 10, 2015

Maks3w Jun 10, 2015

Choose a reason for hiding this comment

denis-sokolov Jun 10, 2015

Choose a reason for hiding this comment

Maks3w commented Jun 10, 2015

Maks3w Jun 10, 2015

Choose a reason for hiding this comment

Maks3w Jun 10, 2015

Choose a reason for hiding this comment

denis-sokolov Jun 10, 2015

Choose a reason for hiding this comment

denis-sokolov commented Jun 10, 2015

denis-sokolov commented Jun 10, 2015

Maks3w commented Jun 10, 2015

denis-sokolov commented Jun 10, 2015

Maks3w Jun 10, 2015

Choose a reason for hiding this comment

Maks3w commented Jun 10, 2015

Maks3w commented Jun 10, 2015

denis-sokolov commented Jun 10, 2015

Maks3w commented Jun 10, 2015

denis-sokolov commented Jun 10, 2015