Skip to content
This repository has been archived by the owner on Jan 30, 2020. It is now read-only.

AddressList does not parse address with parens in name field properly #148

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Prev Previous commit
Next Next commit
Aggregate comments in addresses
This patch accomplishes several things:

- `Address` adds a method, `getComment()`, mapping to a new, optional
  constructor value `$comment`. When present, the generated address will
  include the comment: `Name (Comment) <Email>`.
- It moves the logic for parsing an address value in order to create an
  `Address` to the `Address` class itself.
- `addFromString()` (and the new `Address::fromString()`) now accepts an
  optional `$comment = null` value; when present, the value is passed to
  the `Address` constructor during instantiation.
- `AbstractAddressList` adds a new method, `getComments()`.
  `fromString()` was updated to replace its `array_walk()` with an
  `array_map()` that calls this method, and uses the value to pass to
  `Address::fromString()` (along with the filtered `$value`). This
  ensures we can aggregate the comments, if any, with the address they
  belong to.
  • Loading branch information
weierophinney committed Jun 6, 2018
commit c741371fb8399df9b0e7d8b728c52c6564557f79
82 changes: 78 additions & 4 deletions src/Address.php
Original file line number Diff line number Diff line change
Expand Up @@ -12,17 +12,56 @@

class Address implements Address\AddressInterface
{
protected $comment;
protected $email;
protected $name;

/**
* Create an instance from a string value.
*
* Parses a string representing a single address. If it is a valid format,
* it then creates and returns an instance of itself using the name and
* email it has parsed from the value.
*
* @param string $address
* @param null|string $comment Comment associated with the address, if any.
* @throws Exception\InvalidArgumentException
* @return self
*/
public static function fromString($address, $comment = null)
{
if (! preg_match('/^((?P<name>.*)<(?P<namedEmail>[^>]+)>|(?P<email>.+))$/', $address, $matches)) {
throw new Exception\InvalidArgumentException('Invalid address format');
}

$name = null;
if (isset($matches['name'])) {
$name = trim($matches['name']);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

you could write as: $name = trim($matches['name']) ?: null and drop next if block ;)

Copy link
Member

Choose a reason for hiding this comment

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

We'll get an error if $matches['name'] is not set, however. (Tried it previously which is why I went with this.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nah, $name is assigned null on line 37 before the isset block.

}
if (empty($name)) {
$name = null;
}

if (isset($matches['namedEmail'])) {
$email = $matches['namedEmail'];
}
if (isset($matches['email'])) {
$email = $matches['email'];
}
$email = trim($email);

return new static($email, $name, $comment);
}

/**
* Constructor
*
* @param string $email
* @param null|string $name
* @param null|string $comment
* @throws Exception\InvalidArgumentException
*/
public function __construct($email, $name = null)
public function __construct($email, $name = null, $comment = null)
{
$emailAddressValidator = new EmailAddressValidator(Hostname::ALLOW_DNS | Hostname::ALLOW_LOCAL);
if (! is_string($email) || empty($email)) {
Expand Down Expand Up @@ -51,6 +90,10 @@ public function __construct($email, $name = null)
}

$this->email = $email;

if (null !== $comment) {
$this->comment = $comment;
}
}

/**
Expand All @@ -73,19 +116,50 @@ public function getName()
return $this->name;
}

/**
* Retrieve comment, if any
*
* @return null|string
*/
public function getComment()
{
return $this->comment;
}

/**
* String representation of address
*
* @return string
*/
public function toString()
{
$string = '<' . $this->getEmail() . '>';
$name = $this->getName();
$string = sprintf('<%s>', $this->getEmail());
$name = $this->constructName();
if (null === $name) {
return $string;
}

return $name . ' ' . $string;
return sprintf('%s %s', $name, $string);
}

/**
* Constructs the name string
*
* If a comment is present, appends the comment (commented using parens) to
* the name before returning it; otherwise, returns just the name.
*
* @return null|string
*/
private function constructName()
{
$name = $this->getName();
$comment = $this->getComment();

if ($comment === null || $comment === '') {
return $name;
}

$string = sprintf('%s (%s)', $name, $comment);
return trim($string);
}
}
25 changes: 3 additions & 22 deletions src/AddressList.php
Original file line number Diff line number Diff line change
Expand Up @@ -88,32 +88,13 @@ public function addMany(array $addresses)
* - dev@zf.com
*
* @param string $address
* @param null|string $comment Comment associated with the address, if any.
* @throws Exception\InvalidArgumentException
* @return AddressList
*/
public function addFromString($address)
public function addFromString($address, $comment = null)
{
if (! preg_match('/^((?P<name>.*)<(?P<namedEmail>[^>]+)>|(?P<email>.+))$/', $address, $matches)) {
throw new Exception\InvalidArgumentException('Invalid address format');
}

$name = null;
if (isset($matches['name'])) {
$name = trim($matches['name']);
}
if (empty($name)) {
$name = null;
}

if (isset($matches['namedEmail'])) {
$email = $matches['namedEmail'];
}
if (isset($matches['email'])) {
$email = $matches['email'];
}
$email = trim($email);

return $this->add($email, $name);
$this->add(Address::fromString($address, $comment));
}

/**
Expand Down
63 changes: 51 additions & 12 deletions src/Header/AbstractAddressList.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

namespace Zend\Mail\Header;

use Zend\Mail\Address;
use Zend\Mail\AddressList;
use Zend\Mail\Headers;

Expand Down Expand Up @@ -53,38 +54,45 @@ public static function fromString($headerLine)
$values = AddressListParser::parse($fieldValue);

$wasEncoded = false;
array_walk(
$values,
function (&$value) use (&$wasEncoded) {
$addresses = array_map(
function ($value) use (&$wasEncoded) {
$decodedValue = HeaderWrap::mimeDecodeValue($value);
$wasEncoded = $wasEncoded || ($decodedValue !== $value);

$value = trim($decodedValue);

$comments = self::getComments($value);
$value = self::stripComments($value);

$value = preg_replace(
[
'#(?<!\\\)"(.*)(?<!\\\)"#', //quoted-text
'#\\\([\x01-\x09\x0b\x0c\x0e-\x7f])#' //quoted-pair
'#(?<!\\\)"(.*)(?<!\\\)"#', // quoted-text
'#\\\([\x01-\x09\x0b\x0c\x0e-\x7f])#', // quoted-pair
],
[
'\\1',
'\\1'
'\\1',
],
$value
);
}

return empty($value) ? null : Address::fromString($value, $comments);
},
$values
);
$addresses = array_filter($addresses);

$header = new static();
if ($wasEncoded) {
$header->setEncoding('UTF-8');
}

$values = array_filter($values);

/** @var AddressList $addressList */
$addressList = $header->getAddressList();
foreach ($values as $address) {
$addressList->addFromString($address);
foreach ($addresses as $address) {
$addressList->add($address);
}

return $header;
}

Expand Down Expand Up @@ -194,7 +202,38 @@ public function toString()
return (empty($value)) ? '' : sprintf('%s: %s', $name, $value);
}

// Supposed to be private, protected as a workaround for PHP bug 68194
/**
* Retrieve comments from value, if any.
*
* Supposed to be private, protected as a workaround for PHP bug 68194
*
* @param string $value
* @return string
*/
protected static function getComments($value)
{
$matches = [];
preg_match_all(
'/\\(
(?P<comment>(
\\\\.|
[^\\\\)]
)+)
\\)/x',
$value,
$matches
);
return isset($matches['comment']) ? implode(', ', $matches['comment']) : '';
}

/**
* Strip all comments from value, if any.
*
* Supposed to be private, protected as a workaround for PHP bug 68194
*
* @param string $value
* @return void
*/
protected static function stripComments($value)
{
return preg_replace(
Expand Down
3 changes: 2 additions & 1 deletion test/AddressListTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,8 @@ public function testLosesParensInName()
$to = Header\To::fromString('To:' . $header);
$addressList = $to->getAddressList();
$address = $addressList->get('support@example.org');
$this->assertEquals('Supports (E-mail)', $address->getName());
$this->assertEquals('Supports', $address->getName());
$this->assertEquals('E-mail', $address->getComment());
$this->assertEquals('support@example.org', $address->getEmail());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not testing the toString?

Copy link
Member

Choose a reason for hiding this comment

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

Probably should! I was primarily ensuring that the various accessors returned what was expected.

}

Expand Down