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.

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

Fix AddressList toString method to quote semicolon #93

Merged
merged 1 commit into from
Jun 30, 2020
Merged

Fix AddressList toString method to quote semicolon #93

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions src/Header/AbstractAddressList.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,9 @@ public function getFieldValue($format = HeaderInterface::FORMAT_RAW)
$email = $address->getEmail();
$name = $address->getName();

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.

$name = sprintf('"%s"', $name);
}

Expand Down Expand Up @@ -241,7 +243,7 @@ protected static function getComments($value)
* Supposed to be private, protected as a workaround for PHP bug 68194
*
* @param string $value
* @return void
* @return string
*/
protected static function stripComments($value)
{
Expand Down
26 changes: 26 additions & 0 deletions test/Storage/MessageTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

use Exception as GeneralException;
use Laminas\Mail\Exception as MailException;
use Laminas\Mail\Headers;
use Laminas\Mail\Storage;
use Laminas\Mail\Storage\Exception;
use Laminas\Mail\Storage\Message;
Expand Down Expand Up @@ -433,6 +434,31 @@ public function testSpaceInFieldName()
$this->assertEquals(Mime\Decode::splitHeaderField($header, 'baz'), 42);
}

/**
* splitMessage with Headers as input fails to process AddressList with semicolons
*
* @see https://github.com/laminas/laminas-mail/pull/93
*/
public function testHeadersLosesNameQuoting()
{
$headerList = [
'From: "Famous bearings |;" <skf@example.com>',
'Reply-To: "Famous bearings |:" <skf@example.com>',
];

// create Headers object from array
Mime\Decode::splitMessage(implode("\r\n", $headerList), $headers1, $body);
$this->assertInstanceOf(Headers::class, $headers1);
// create Headers object from Headers object
Mime\Decode::splitMessage($headers1, $headers2, $body);
$this->assertInstanceOf(Headers::class, $headers2);

// test that same problem does not happen with Storage\Message internally
$message = new Message(['headers' => $headers2, 'content' => (string)$body]);
$this->assertEquals('"Famous bearings |;" <skf@example.com>', $message->from);
$this->assertEquals('Famous bearings |: <skf@example.com>', $message->replyTo);
}

/**
* @group Laminas-372
*/
Expand Down