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

Fix AddressList toString method to quote semicolon #93

Merged
merged 1 commit into from
Jun 30, 2020

Conversation

glensc
Copy link
Contributor

@glensc glensc commented Jun 27, 2020

This is port of zendframework/zend-mail#230 fixing regression from zendframework/zend-mail#147

Fixes #14


Certain input of AddressList headers, cannot be converted to string and back to header object because of incorrect quoting:

From: "Foo;" <root@example.com>

gets incorrectly converted as

From: Foo; <root@example.com>

but ; is address separator, so it needs to be quoted:

From: "Foo;" <root@example.com>

The problem I discovered internally when using Storage\Message with Headers input, therefore testing that method is included in the unit test:

$message = new Message(['headers' => new Headers(), 'content' => (string)$body]);

// Mime\Decode::splitMessage calls toString on headers object which creates invalid result:
if ($message instanceof Headers) {
    $message = $message->toString();
}

@glensc
Copy link
Contributor Author

glensc commented Jun 27, 2020

NOTE: This is needed to switch to laminas-mail in the project I maintain: eventum/eventum#876

@michalbundyra michalbundyra added the Bug Something isn't working label Jun 27, 2020
@michalbundyra
Copy link
Member

@glensc thanks for your contribution. Before we can accept the PR with need DCO to be valid on PRs in Laminas org. Please see https://github.com/laminas/laminas-mail/pull/93/checks?check_run_id=813642578 for more details. Thanks!

@glensc
Copy link
Contributor Author

glensc commented Jun 27, 2020

@michalbundyra sure, those were fixup commits that I intended to flatten once Travis passed

Signed-off-by: Elan Ruusamäe <glen@pld-linux.org>
if (! empty($name) && false !== strstr($name, ',')) {
// quote $name if value requires so
if (! empty($name) && (false !== strpos($name, ',') || false !== strpos($name, ';'))) {
// FIXME: what if name contains double quote?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@glensc would be great, if you can provide another PR with a proper test case for that, thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@michalbundyra what you mean proper? what's wrong with the provided one? please be specific what you mean, test case for what exactly?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@glensc sorry, I mean test with name which contains double quote, so separate issue.

As I understand this is about ; in the name only.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps I remove this "FIXME" comment then it being out of scope? as the changeset does not introduce that as a new problem, the problem was there already before, I just noticed that and left a note.

besides I don't know how it should be encoded and at this point and I'm not interested in researching on, this PR has been waiting to be accepted by upstream way too long to get sidetracked with this.

weierophinney added a commit that referenced this pull request Jun 30, 2020
Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
@weierophinney weierophinney merged commit 05a9368 into laminas:master Jun 30, 2020
weierophinney added a commit that referenced this pull request Jun 30, 2020
artemii-karkusha pushed a commit to artemii-karkusha/laminas-mail that referenced this pull request May 24, 2023
Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
artemii-karkusha pushed a commit to artemii-karkusha/laminas-mail that referenced this pull request May 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix AddressList toString method to quote semicolon
3 participants