8000 [Mime][BCBreak] Change of expected behaviour in address quoting since #39866 · Issue #41264 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Mime][BCBreak] Change of expected behaviour in address quoting since #39866 #41264

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
Closed
althaus opened this issue May 18, 2021 · 14 comments
Closed

[Mime][BCBreak] Change of expected behaviour in address quoting since #39866 #41264

althaus opened this issue May 18, 2021 · 14 comments
Labels
Bug Mime Stalled Status: Needs Review

Comments

@althaus
Copy link
Contributor
althaus commented May 18, 2021

Symfony version(s) affected: 4.4.23

Description

In PR #39866 a bugfix was introduced to add quoting of sender names to the Address::toString method. This breaks our current code which expects unquoted names. I understand why the change was applied, but I don't think that it should change the current behaviour of the toString method. We also rely on this for proper displaying where quotes aren't needed.

We now have doubled quoting in our test cases. In addition the fix quotes spaces which is not necessary at all.

How to reproduce

  1. Create a new address new Address('test@example.org', 'Test, Test')
  2. Have custom code that quotes the name if it contains special characters (see RFC 5322)
  3. The final string representation now contains doubled quotes: "\"Test, Test\" <test@example.org>"

Possible Solution

Introduce a new toQuotedString method or something similar.

Kind regards
Matthias
@althaus althaus added the Bug label May 18, 2021
@OskarStark
Copy link
Contributor

@YaFou could you please have a look? Thanks 🙏

@YaFou
Copy link
Contributor
YaFou commented May 23, 2021

@althaus So if I understood well, you want quotes to be displayed only if there are special characters in the address? In my opinion, a toQuotedString method is not necessary and will add complexity for nothing.

@althaus
Copy link
Contributor Author
althaus commented May 25, 2021

@YaFou My issue is that your PR changed the fundamental behaviour of the method and makes a lot of our tests fail.

Sending a properly quoted string before resulted in a valid string representation:

new Address('test@example.org', '"Test, Test"')

After your PR this renders double quoted.

Our main concern isn't what the method is doing, but rather that it changed in a patch release, where you can dicuss if it really was broken or not before.

@YaFou
Copy link
Contributor
YaFou commented May 28, 2021

I understand your issue. Nevertheless, there is still one issue: how do we know if the user adds intentionally quotes to escape special characters or it adds them because quotes are in the part of the name? I don't see any solutions except adding a new static method for example that takes into account quotes around the name and if they exist, don't add unnecessary quotes.
@OskarStark can we consider that it is a breaking change?

@OskarStark
Copy link
Contributor

TBH I am not sure how to proceed here.

cc @fabpot
8000

@althaus
Copy link
Contributor Author
althaus commented Jun 24, 2021

Any news how to get this resolved in any way? @YaFou @OskarStark @fabpot

This little issue is blocking as from upgrading from Symfony 4.4.22.

@YaFou
Copy link
Contributor
YaFou commented Jun 26, 2021

As @OskarStark, I wait a review from another maintainer for his opinion. However, I will give a try to fix this issue @althaus 😃

@carsonbot
Copy link

Hey, thanks for your report!
There has not been a lot of activity here for a while. Is this bug still relevant? Have you managed to find a workaround?

@althaus
Copy link
Contributor Author
althaus commented Dec 28, 2021

Yes, although I've work around this internally with some vodoo to detect what the library is doing, it'd be great to have a clean solution.

@carsonbot carsonbot removed the Stalled label Dec 28, 2021
@nicolas-grekas
Copy link
Member

Could you try sending a fix @althaus ?

@althaus
Copy link
Contributor Author
althaus commented Feb 23, 2022

@nicolas-grekas I'm not sure how to resolve this in a clean fashion now. The commit changed existing behaviour and I don't think that you can revert it again. So we'd either add a new factory method or a new toTbdString method.

@carsonbot
Copy link

Hey, thanks for your report!
There has not been a lot of activity here for a while. Is this bug still relevant? Have you managed to find a workaround?

< 8000 path d="M15 8a7.002 7.002 0 00-7-7" stroke="currentColor" stroke-width="2" stroke-linecap="round" vector-effect="non-scaling-stroke" />

@carsonbot
Copy link

Hello? This issue is about to be closed if nobody replies.

@carsonbot
Copy link

Hey,

I didn't hear anything so I'm going to close it. Feel free to comment if this is still relevant, I can always reopen!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug Mime Stalled Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@althaus @nicolas-grekas @OskarStark @carsonbot @YaFou
0