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

Update EmailAddress.php #444

Closed
wants to merge 1 commit into from

Conversation

JorisDebonnet
Copy link

@JorisDebonnet JorisDebonnet commented Sep 29, 2022

Several email/calendar clients do not support %40 in place of @ in email addresses. For example, EmClient will show "nobody@invalid.invalid" instead of the actual email address. Also some other clients may work but show the %40 in plain text instead of converting it to @, confusing users.

So let's just not replace @ with %40. 😊

Fixes #434

Several email/calendar clients do not support %40 in place of % in email addresses. For example, EmClient will show "nobody@invalid.invalid" instead of the actual email address. Also some other clients may work but show the %40 in plain text instead of converting it to @. 

So let's just not replace @ with %40. 😊
@Bapawe
Copy link

Bapawe commented Nov 25, 2022

We've had a similar issue where users could not import the ics file (with encoded emails in Organizer and Attendee) in Outlook 2016. After applying this fix they could import the ics file again.

@navarr
Copy link
Contributor

navarr commented Nov 25, 2022

This is a decent solution for most occurences of the issue, and while I'm not sure if its important enough - this would ironically destroy an email of user-with-%40-in-their-address@domain.tld which is seemingly a valid email address.

A better solution might be to require a properly formatted email address and do no escaping to it, or otherwise force the creation of an EmailAddress object that maintains a separate user and host that can be encoded separately. (Although, are subject, cc, body, and other mailto: parameters.

@JorisDebonnet
Copy link
Author

This is a decent solution for most occurences of the issue, and while I'm not sure if its important enough - this would ironically destroy an email of user-with-%40-in-their-address@domain.tld which is seemingly a valid email address.

I understand what you mean, however, %40 us currently already being converted to %2540 because of the urlencode ... 😏

So although I agree this PR is not the best solution, it is better than it is now for many email clients.

Of course, if we can quickly find a better solution (with email validators or sanitizers), I would be fine with that of course, then there is no urgency in merging this PR :)

@markuspoerschke markuspoerschke self-assigned this Dec 23, 2022
markuspoerschke added a commit that referenced this pull request Dec 23, 2022
Do not url encode email addresses.

Fixes #444
Fixes #434
Fixes #390
@markuspoerschke
Copy link
Owner

Thanks for raising this issue.

This bug will be fixed in #479.

markuspoerschke added a commit that referenced this pull request Dec 23, 2022
Do not url encode email addresses.

Fixes #444
Fixes #434
Fixes #390
@JorisDebonnet JorisDebonnet deleted the patch-1 branch December 23, 2022 11:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The organizer's e-mail changes the @ to %40
4 participants