Fix ReferenceEscaperLexer. #733
Conversation
Escape emails ending with a digit.
| @@ -44,8 +44,7 @@ public function __construct(LexerInterface $decoratedLexer) | |||
| */ | |||
| public function lex(string $value): array | |||
| { | |||
| $escapedValue = preg_replace('/(\\p{L})@/', '$1\\@', $value); | |||
|
|
|||
| $escapedValue = preg_replace('/((\s|^)[a-zA-Z0-9_.+-]+?[A-Za-z0-9])@/', '$1\\@', $value); | |||
There was a problem hiding this comment.
an email could have a non ASCII character, so a-zA-Z is wrong as à for example won't work, hence p{L}
There was a problem hiding this comment.
@theofidry something like
((\s|^)[^@]?(\p{L}|\p{N}|[+-_.])+?(\p{L}|\p{N}))@ should work just fine.
I will test this later.
| @@ -66,5 +66,7 @@ public function provideValues() | |||
| yield 'string with a reference with members' => ['bar @foo baz']; | |||
|
|
|||
| yield 'reference in a middle of a word' => ['email@example', 'email\\@example']; | |||
|
|
|||
| yield 'email ending with a digit' => ['email1@example', 'email1\\@example']; | |||
There was a problem hiding this comment.
given the issues we had, feels like we should add a few more tests with exotic emails (with non ASCII characters for example)
|
I think that's fine, it looks like more of and edge case that can very well wait #712 |
|
No worries, but it looks like this causes a slight parsing issue |
| @@ -44,7 +44,7 @@ public function __construct(LexerInterface $decoratedLexer) | |||
| */ | |||
| public function lex(string $value): array | |||
| { | |||
| $escapedValue = preg_replace('/((\s|^)[a-zA-Z0-9_.+-]+?[A-Za-z0-9])@/', '$1\\@', $value); | |||
| $escapedValue = preg_replace('/((\s|^)[^@]?(\p{L}|\p{N}|[+-_.])+?(\p{L}|\p{N}))@/', '$1\\@', $value); | |||
There was a problem hiding this comment.
If the regex was written without the optional [^@] it would work fine but only emails with at least 2 characters in the name would be matched.
There was a problem hiding this comment.
I could be ok with that, but I think we need way more tests regarding emails to be able to tell if it's alright or not. Also correct me if I'm wrong but it's about auto-escaping emails, so for tricky edge cases one could always fallback to manual escaping e.g. a\@b.c
There was a problem hiding this comment.
@theofidry Agreed as far as writing more tests goes. I would prefer no auto-escaping than "sometimes works" auto-escaping. You have the final say, this looks like it could create problems with time, so maybe remove auto-escaping with the next major release ?
There was a problem hiding this comment.
I think auto-escaping is fine. Most people use regular emails or faker generated emails, it's ok for the 1-2 people using complete alien emails to not benefit from it
|
@ndyakov any updates? No pressure if you don't have time, it's just to know if I can count on it to be included in the next RC release or if this will wait later |
|
Nothing at the moment, sorry.
…On Thu, Jun 29, 2017 at 2:35 AM Théo FIDRY ***@***.***> wrote:
@ndyakov <https://github.com/ndyakov> any updates? No pressure if you
don't have time, it's just to know if I can count on it to be included in
the next RC release or if this will wait later
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#733 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABebskhmwfiJCeTsFp-YUsyXsJbBoY_zks5sIuMPgaJpZM4NWa95>
.
|
|
That's ok :) |
|
Is this still relevant? As mentioned in #724 this now seems to be working fine |
|
@theofidry since then we have moved away from emails ending with numbers for our fixtures, but I can test this if it is working tomorrow. How do you feel about closing this one and probably open a new one with just few more tests regarding the numbers in emails? |
|
Fair enough :) |
Escape emails ending with a digit.
Related to #724.
Also @theofidry Keep in mind that the old Escaper would have escaped nested parameters such as
@user@user1.