From a44ed78116c721da7ce79cbc2e43b450e325012b Mon Sep 17 00:00:00 2001 From: Maks3w Date: Mon, 11 Jun 2012 02:12:28 +0200 Subject: [PATCH 1/6] [Mail] Refactor headers * Use more HeadersInterface instances in Headers implementation * Centralize the render of headers newlines (CRLF) in Headers implementation rather than each HeaderInterface::toString() * Delegate headers render in each instance toString --- src/Header/AbstractAddressList.php | 2 +- src/Header/ContentType.php | 2 +- src/Headers.php | 135 ++++++++++---------------- test/Header/AddressListHeaderTest.php | 2 +- test/Header/ContentTypeTest.php | 2 +- test/HeadersTest.php | 2 +- 6 files changed, 55 insertions(+), 90 deletions(-) diff --git a/src/Header/AbstractAddressList.php b/src/Header/AbstractAddressList.php index 7072a663..debb62b3 100644 --- a/src/Header/AbstractAddressList.php +++ b/src/Header/AbstractAddressList.php @@ -208,6 +208,6 @@ public function toString() { $name = $this->getFieldName(); $value = $this->getFieldValue(); - return sprintf("%s: %s\r\n", $name, $value); + return sprintf('%s: %s', $name, $value); } } diff --git a/src/Header/ContentType.php b/src/Header/ContentType.php index ec4619ba..322a2e7b 100755 --- a/src/Header/ContentType.php +++ b/src/Header/ContentType.php @@ -141,7 +141,7 @@ public function getEncoding() */ public function toString() { - return 'Content-Type: ' . $this->getFieldValue() . "\r\n"; + return 'Content-Type: ' . $this->getFieldValue(); } /** diff --git a/src/Headers.php b/src/Headers.php index ef105e61..87c01808 100644 --- a/src/Headers.php +++ b/src/Headers.php @@ -52,13 +52,13 @@ class Headers implements Iterator, Countable protected $headersKeys = array(); /** - * @var array Array of header array information or HeaderInterface instances + * @var Header\HeaderInterface[] instances */ protected $headers = array(); /** * Header encoding; defaults to ASCII - * + * * @var string */ protected $encoding = 'ASCII'; @@ -76,26 +76,21 @@ class Headers implements Iterator, Countable */ public static function fromString($string) { - $headers = new static(); - $current = array(); + $headers = new static(); + $currentLine = ''; // iterate the header lines, some might be continuations foreach (explode("\r\n", $string) as $line) { - // check if a header name is present if (preg_match('/^(?P[^()><@,;:\"\\/\[\]?=}{ \t]+):.*$/', $line, $matches)) { - if ($current) { + if ($currentLine) { // a header name was present, then store the current complete line - $headers->headersKeys[] = str_replace(array('-', '_'), '', strtolower($current['name'])); - $headers->headers[] = $current; + $headers->addHeaderLine($currentLine); } - $current = array( - 'name' => $matches['name'], - 'line' => trim($line) - ); + $currentLine = trim($line); } elseif (preg_match('/^\s+.*$/', $line, $matches)) { // continuation: append to current line - $current['line'] .= trim($line); + $currentLine .= trim($line); } elseif (preg_match('/^\s*$/', $line)) { // empty line indicates end of headers break; @@ -107,9 +102,8 @@ public static function fromString($string) )); } } - if ($current) { - $headers->headersKeys[] = str_replace(array('-', '_'), '', strtolower($current['name'])); - $headers->headers[] = $current; + if ($currentLine) { + $headers->addHeaderLine($currentLine); } return $headers; } @@ -159,8 +153,8 @@ public function getPluginClassLoader() /** * Set the header encoding - * - * @param string $encoding + * + * @param string $encoding * @return Headers */ public function setEncoding($encoding) @@ -174,7 +168,7 @@ public function setEncoding($encoding) /** * Get the header encoding - * + * * @return string */ public function getEncoding() @@ -240,24 +234,13 @@ public function addHeaderLine($headerFieldNameOrLine, $fieldValue = null) )); } - $matches = null; - if (preg_match('/^(?P[^()><@,;:\"\\/\[\]?=}{ \t]+):.*$/', $headerFieldNameOrLine, $matches) - && $fieldValue === null - ) { - // is a header - $headerName = $matches['name']; - $headerKey = str_replace(array('-', '_', ' ', '.'), '', strtolower($matches['name'])); - $line = $headerFieldNameOrLine; - } elseif ($fieldValue === null) { - throw new Exception\InvalidArgumentException('A field name was provided without a field value'); + if ($fieldValue === null) { + $header = Header\GenericHeader::fromString($headerFieldNameOrLine); } else { - $headerName = $headerFieldNameOrLine; - $headerKey = str_replace(array('-', '_', ' ', '.'), '', strtolower($headerFieldNameOrLine)); - $line = $headerFieldNameOrLine . ': ' . $fieldValue; + $header = new Header\GenericHeader($headerFieldNameOrLine, $fieldValue); } - $this->headersKeys[] = $headerKey; - $this->headers[] = array('name' => $headerName, 'line' => $line); + $this->addHeader($header); return $this; } @@ -285,7 +268,8 @@ public function addHeader(Header\HeaderInterface $header) */ public function removeHeader(Header\HeaderInterface $header) { - $index = array_search($header, $this->headers, true); + $key = $this->normalizeFieldName($header->getFieldName()); + $index = array_search($key, $this->headersKeys, true); if ($index !== false) { unset($this->headersKeys[$index]); unset($this->headers[$index]); @@ -298,7 +282,7 @@ public function removeHeader(Header\HeaderInterface $header) * Clear all headers * * Removes all headers from queue - * + * * @return Headers */ public function clearHeaders() @@ -309,53 +293,45 @@ public function clearHeaders() /** * Get all headers of a certain name/type - * + * * @param string $name - * @return boolean|Header\HeaderInterface|ArrayIterator + * @return boolean|ArrayIterator|Header\HeaderInterface Returns false if there is no headers with $name in this + * contain, an ArrayIterator if the header is a MultipleHeadersInterface instance and finally returns + * HeaderInterface for the rest of cases. */ public function get($name) { $key = $this->normalizeFieldName($name); - if (!in_array($key, $this->headersKeys)) { - return false; - } + $results = array(); - $class = ($this->getPluginClassLoader()->load($key)) ?: 'Zend\Mail\Header\GenericHeader'; + foreach (array_keys($this->headersKeys, $key) as $index) { + $results[] = $this->headers[$index]; + } - if (in_array('Zend\Mail\Header\MultipleHeadersInterface', class_implements($class, true))) { - $headers = array(); - foreach (array_keys($this->headersKeys, $key) as $index) { - if (is_array($this->headers[$index])) { - $this->lazyLoadHeader($index); - } - } - foreach (array_keys($this->headersKeys, $key) as $index) { - $headers[] = $this->headers[$index]; - } - return new ArrayIterator($headers); - } else { - $index = array_search($key, $this->headersKeys); - if ($index === false) { + switch(count($results)) { + case 0: return false; - } - if (is_array($this->headers[$index])) { - return $this->lazyLoadHeader($index); - } else { - return $this->headers[$index]; - } + case 1: + if ($results[0] instanceof Header\MultipleHeadersInterface) { + return new ArrayIterator($results); + } else { + return $results[0]; + } + default: + return new ArrayIterator($results); } } /** * Test for existence of a type of header - * + * * @param string $name * @return bool */ public function has($name) { $name = $this->normalizeFieldName($name); - return (in_array($name, $this->headersKeys)); + return in_array($name, $this->headersKeys); } /** @@ -374,7 +350,7 @@ public function next() */ public function key() { - return (key($this->headers)); + return key($this->headers); } /** @@ -404,7 +380,7 @@ public function rewind() public function current() { $current = current($this->headers); - if (is_array($current)) { + if ($current instanceof Header\GenericHeader) { $current = $this->lazyLoadHeader(key($this->headers)); } return $current; @@ -432,17 +408,12 @@ public function count() public function toString() { $headers = ''; - foreach ($this->toArray() as $fieldName => $fieldValue) { - if (is_array($fieldValue)) { - // Handle multi-value headers - foreach ($fieldValue as $value) { - $headers .= $fieldName . ': ' . $value . "\r\n"; - } - continue; + foreach ($this->headers as $header) { + if ($str = $header->toString()) { + $headers .= $str . "\r\n"; } - // Handle single-value headers - $headers .= $fieldName . ': ' . $fieldValue . "\r\n"; } + return $headers; } @@ -463,14 +434,8 @@ public function toArray() $headers[$name] = array(); } $headers[$name][] = $header->getFieldValue(); - } elseif ($header instanceof Header\HeaderInterface) { - $headers[$header->getFieldName()] = $header->getFieldValue(); } else { - $matches = null; - preg_match('/^(?P[^()><@,;:\"\\/\[\]?=}{ \t]+):\s*(?P.*)$/', $header['line'], $matches); - if ($matches) { - $headers[$matches['name']] = $matches['value']; - } + $headers[$header->getFieldName()] = $header->getFieldValue(); } } return $headers; @@ -502,7 +467,7 @@ protected function lazyLoadHeader($index) $class = ($this->getPluginClassLoader()->load($key)) ?: 'Zend\Mail\Header\GenericHeader'; $encoding = $this->getEncoding(); - $headers = $class::fromString($current['line']); + $headers = $class::fromString($current->toString()); if (is_array($headers)) { $current = array_shift($headers); $current->setEncoding($encoding); @@ -523,8 +488,8 @@ protected function lazyLoadHeader($index) /** * Normalize a field name - * - * @param string $fieldName + * + * @param string $fieldName * @return string */ protected function normalizeFieldName($fieldName) diff --git a/test/Header/AddressListHeaderTest.php b/test/Header/AddressListHeaderTest.php index 07d538b6..c64f1b64 100644 --- a/test/Header/AddressListHeaderTest.php +++ b/test/Header/AddressListHeaderTest.php @@ -110,7 +110,7 @@ public function getExpectedFieldValue() public function testStringRepresentationIncludesHeaderAndFieldValue($header, $type) { $this->populateAddressList($header->getAddressList()); - $expected = sprintf("%s: %s\r\n", $type, $this->getExpectedFieldValue()); + $expected = sprintf('%s: %s', $type, $this->getExpectedFieldValue()); $this->assertEquals($expected, $header->toString()); } diff --git a/test/Header/ContentTypeTest.php b/test/Header/ContentTypeTest.php index 77da30ca..66a212bd 100644 --- a/test/Header/ContentTypeTest.php +++ b/test/Header/ContentTypeTest.php @@ -58,7 +58,7 @@ public function testContentTypeToStringReturnsHeaderFormattedString() { $contentTypeHeader = new ContentType(); $contentTypeHeader->setType('foo/bar'); - $this->assertEquals("Content-Type: foo/bar\r\n", $contentTypeHeader->toString()); + $this->assertEquals("Content-Type: foo/bar", $contentTypeHeader->toString()); } public function testProvidingParametersIntroducesHeaderFolding() diff --git a/test/HeadersTest.php b/test/HeadersTest.php index a7f61776..a47235f1 100644 --- a/test/HeadersTest.php +++ b/test/HeadersTest.php @@ -144,7 +144,7 @@ public function testHeadersAggregatesHeaderThroughAddHeaderLine() public function testHeadersAddHeaderLineThrowsExceptionOnMissingFieldValue() { - $this->setExpectedException('Zend\Mail\Exception\InvalidArgumentException', 'without a field'); + $this->setExpectedException('Zend\Mail\Header\Exception\InvalidArgumentException', 'Header must match with the format "name: value"'); $headers = new Mail\Headers(); $headers->addHeaderLine('Foo'); } From 675e021183cf0cb448cb81029b145fc0dd81d59e Mon Sep 17 00:00:00 2001 From: Maks3w Date: Sun, 10 Jun 2012 20:40:08 +0200 Subject: [PATCH 2/6] [Mail] Avoid render To: and Cc: headers if doesn't have addresses --- src/Header/AbstractAddressList.php | 2 +- test/Transport/SmtpTest.php | 33 +++++++++++++++++++++++++++++- 2 files changed, 33 insertions(+), 2 deletions(-) diff --git a/src/Header/AbstractAddressList.php b/src/Header/AbstractAddressList.php index debb62b3..8cb1a69d 100644 --- a/src/Header/AbstractAddressList.php +++ b/src/Header/AbstractAddressList.php @@ -208,6 +208,6 @@ public function toString() { $name = $this->getFieldName(); $value = $this->getFieldValue(); - return sprintf('%s: %s', $name, $value); + return (empty($value)) ? '' : sprintf('%s: %s', $name, $value); } } diff --git a/test/Transport/SmtpTest.php b/test/Transport/SmtpTest.php index d3ef2843..da199142 100644 --- a/test/Transport/SmtpTest.php +++ b/test/Transport/SmtpTest.php @@ -21,7 +21,8 @@ namespace ZendTest\Mail\Transport; -use Zend\Mail\Message, +use Zend\Mail\Headers, + Zend\Mail\Message, Zend\Mail\Transport\Smtp, Zend\Mail\Transport\SmtpOptions, ZendTest\Mail\TestAsset\SmtpProtocolSpy; @@ -36,7 +37,9 @@ */ class SmtpTest extends \PHPUnit_Framework_TestCase { + /** @var Smtp */ public $transport; + /** @var SmtpProtocolSpy */ public $connection; public function setUp() @@ -65,6 +68,34 @@ public function getMessage() return $message; } + public function testSendMailWithoutMinimalHeaders() { + $this->setExpectedException( + 'Zend\Mail\Transport\Exception\RuntimeException', + 'transport expects either a Sender or at least one From address in the Message; none provided' + ); + $message = new Message(); + $message->setBody('testSendMailWithoutMinimalHeaders'); + $this->transport->send($message); + } + + public function testSendMinimalMail() { + $headers = new Headers(); + $headers->addHeaderLine('Date', 'Sun, 10 Jun 2012 20:07:24 +0200'); + $message = new Message(); + $message + ->setHeaders($headers) + ->setSender('ralph.schindler@zend.com', 'Ralph Schindler') + ->setBody('testSendMailWithoutMinimalHeaders') + ; + $expectedMessage = "Date: Sun, 10 Jun 2012 20:07:24 +0200\r\n" + . "Sender: Ralph Schindler \r\n" + . "\r\ntestSendMailWithoutMinimalHeaders"; + + $this->transport->send($message); + + $this->assertSame($expectedMessage, $this->connection->getData()); + } + public function testReceivesMailArtifacts() { $message = $this->getMessage(); From 4a1e184cdc442d3f850131df815e46c9eea6dcac Mon Sep 17 00:00:00 2001 From: Maks3w Date: Tue, 12 Jun 2012 21:40:09 +0200 Subject: [PATCH 3/6] [Mail] Replace line endings for fields (\r\n) with a constant --- src/Header/AbstractAddressList.php | 5 +++-- src/Header/ContentType.php | 6 ++++-- src/Header/HeaderWrap.php | 8 +++++--- src/Header/Received.php | 4 +++- src/Headers.php | 10 ++++++++-- src/Message.php | 4 +++- src/Transport/Smtp.php | 2 +- 7 files changed, 27 insertions(+), 12 deletions(-) diff --git a/src/Header/AbstractAddressList.php b/src/Header/AbstractAddressList.php index 8cb1a69d..32316b80 100644 --- a/src/Header/AbstractAddressList.php +++ b/src/Header/AbstractAddressList.php @@ -22,6 +22,7 @@ namespace Zend\Mail\Header; use Zend\Mail\AddressList; +use Zend\Mail\Headers; /** * Base class for headers composing address lists (to, from, cc, bcc, reply-to) @@ -79,7 +80,7 @@ public static function fromString($headerLine) $header = new static(); // split value on "," - $fieldValue = str_replace("\r\n ", " ", $fieldValue); + $fieldValue = str_replace(Headers::FOLDING, ' ', $fieldValue); $values = explode(',', $fieldValue); array_walk($values, 'trim'); @@ -150,7 +151,7 @@ public function getFieldValue() $emails[] = sprintf('%s <%s>', $name, $email); } } - $string = implode(",\r\n ", $emails); + $string = implode(',' . Headers::FOLDING, $emails); return $string; } diff --git a/src/Header/ContentType.php b/src/Header/ContentType.php index 322a2e7b..83e0f9d2 100755 --- a/src/Header/ContentType.php +++ b/src/Header/ContentType.php @@ -21,6 +21,8 @@ namespace Zend\Mail\Header; +use Zend\Mail\Headers; + /** * @category Zend * @package Zend_Mail @@ -64,7 +66,7 @@ public static function fromString($headerLine) throw new Exception\InvalidArgumentException('Invalid header line for Content-Type string'); } - $value = str_replace("\r\n ", " ", $value); + $value = str_replace(Headers::FOLDING, " ", $value); $values = preg_split('#\s*;\s*#', $value); $type = array_shift($values); @@ -108,7 +110,7 @@ public function getFieldValue() foreach ($this->parameters as $attribute => $value) { $values[] = sprintf('%s="%s"', $attribute, $value); } - $value = implode(";\r\n ", $values); + $value = implode(';' . Headers::FOLDING, $values); return $value; } diff --git a/src/Header/HeaderWrap.php b/src/Header/HeaderWrap.php index d5d3605f..86f57fb2 100644 --- a/src/Header/HeaderWrap.php +++ b/src/Header/HeaderWrap.php @@ -21,6 +21,8 @@ namespace Zend\Mail\Header; +use Zend\Mail\Headers; + /** * Utility class used for creating wrapped or MIME-encoded versions of header * values. @@ -60,7 +62,7 @@ public static function wrap($value, HeaderInterface $header) */ protected static function wrapUnstructuredHeader($value) { - return wordwrap($value, 78, "\r\n "); + return wordwrap($value, 78, Headers::FOLDING); } /** @@ -84,7 +86,7 @@ protected static function wrapStructuredHeader($value, HeaderInterface $header) $temp = ''; } } - return implode("\r\n ", $lines); + return implode(Headers::FOLDING, $lines); } /** @@ -110,7 +112,7 @@ public static function mimeEncodeValue($value, $encoding, $splitWords = false) )); return str_replace('Header: ', '', $header); }, explode(' ', $value)); - return implode("\r\n ", $words); + return implode(Headers::FOLDING, $words); } $header = iconv_mime_encode('Header', $value, array( diff --git a/src/Header/Received.php b/src/Header/Received.php index 292059bc..d2ac8f42 100644 --- a/src/Header/Received.php +++ b/src/Header/Received.php @@ -21,6 +21,8 @@ namespace Zend\Mail\Header; +use Zend\Mail\Headers; + /** * @todo Allow setting date from DateTime, Zend\Date, or string * @category Zend @@ -135,6 +137,6 @@ public function toStringMultipleHeaders(array $headers) } $strings[] = $header->toString(); } - return implode("\r\n", $strings); + return implode(Headers::EOL, $strings); } } diff --git a/src/Headers.php b/src/Headers.php index 87c01808..bf641486 100644 --- a/src/Headers.php +++ b/src/Headers.php @@ -41,6 +41,12 @@ */ class Headers implements Iterator, Countable { + /** @var string End of Line for fields */ + const EOL = "\r\n"; + + /** @var string Start of Line when folding */ + const FOLDING = "\r\n "; + /** * @var PluginClassLoader */ @@ -80,7 +86,7 @@ public static function fromString($string) $currentLine = ''; // iterate the header lines, some might be continuations - foreach (explode("\r\n", $string) as $line) { + foreach (explode(self::EOL, $string) as $line) { // check if a header name is present if (preg_match('/^(?P[^()><@,;:\"\\/\[\]?=}{ \t]+):.*$/', $line, $matches)) { if ($currentLine) { @@ -410,7 +416,7 @@ public function toString() $headers = ''; foreach ($this->headers as $header) { if ($str = $header->toString()) { - $headers .= $str . "\r\n"; + $headers .= $str . self::EOL; } } diff --git a/src/Message.php b/src/Message.php index b512e0ed..a15c0721 100644 --- a/src/Message.php +++ b/src/Message.php @@ -546,6 +546,8 @@ protected function updateAddressList(AddressList $addressList, $emailOrAddressOr public function toString() { $headers = $this->headers(); - return $headers->toString() . "\r\n" . $this->getBodyText(); + return $headers->toString() + . Headers::EOL + . $this->getBodyText(); } } diff --git a/src/Transport/Smtp.php b/src/Transport/Smtp.php index d67a263e..79f667b8 100644 --- a/src/Transport/Smtp.php +++ b/src/Transport/Smtp.php @@ -243,7 +243,7 @@ public function send(Mail\Message $message) } // Issue DATA command to client - $connection->data($headers . "\r\n" . $body); + $connection->data($headers . Headers::EOL . $body); } /** From 2926e3e70c6ebad0346b3b3af330d6e678796f8d Mon Sep 17 00:00:00 2001 From: Maks3w Date: Tue, 12 Jun 2012 09:11:55 +0200 Subject: [PATCH 4/6] [Mail] Move Header loader outside from Headers class --- src/Header/HeaderLoader.php | 48 +++++++++++++++++++++++++++++++++++++ src/Headers.php | 23 ++---------------- 2 files changed, 50 insertions(+), 21 deletions(-) create mode 100644 src/Header/HeaderLoader.php diff --git a/src/Header/HeaderLoader.php b/src/Header/HeaderLoader.php new file mode 100644 index 00000000..6f8d514d --- /dev/null +++ b/src/Header/HeaderLoader.php @@ -0,0 +1,48 @@ + 'Zend\Mail\Header\Bcc', + 'cc' => 'Zend\Mail\Header\Cc', + 'contenttype' => 'Zend\Mail\Header\ContentType', + 'content_type' => 'Zend\Mail\Header\ContentType', + 'content-type' => 'Zend\Mail\Header\ContentType', + 'date' => 'Zend\Mail\Header\Date', + 'from' => 'Zend\Mail\Header\From', + 'mimeversion' => 'Zend\Mail\Header\MimeVersion', + 'mime_version' => 'Zend\Mail\Header\MimeVersion', + 'mime-version' => 'Zend\Mail\Header\MimeVersion', + 'received' => 'Zend\Mail\Header\Received', + 'replyto' => 'Zend\Mail\Header\ReplyTo', + 'reply_to' => 'Zend\Mail\Header\ReplyTo', + 'reply-to' => 'Zend\Mail\Header\ReplyTo', + 'sender' => 'Zend\Mail\Header\Sender', + 'subject' => 'Zend\Mail\Header\Subject', + 'to' => 'Zend\Mail\Header\To', + ); +} \ No newline at end of file diff --git a/src/Headers.php b/src/Headers.php index bf641486..a8e625af 100644 --- a/src/Headers.php +++ b/src/Headers.php @@ -25,7 +25,6 @@ Iterator, Countable, Traversable, - Zend\Loader\PluginClassLoader, Zend\Loader\PluginClassLocator; /** @@ -48,7 +47,7 @@ class Headers implements Iterator, Countable const FOLDING = "\r\n "; /** - * @var PluginClassLoader + * @var \Zend\Loader\PluginClassLoader */ protected $pluginClassLoader = null; @@ -134,25 +133,7 @@ public function setPluginClassLoader(PluginClassLocator $pluginClassLoader) public function getPluginClassLoader() { if ($this->pluginClassLoader === null) { - $this->pluginClassLoader = new PluginClassLoader(array( - 'bcc' => 'Zend\Mail\Header\Bcc', - 'cc' => 'Zend\Mail\Header\Cc', - 'contenttype' => 'Zend\Mail\Header\ContentType', - 'content_type' => 'Zend\Mail\Header\ContentType', - 'content-type' => 'Zend\Mail\Header\ContentType', - 'date' => 'Zend\Mail\Header\Date', - 'from' => 'Zend\Mail\Header\From', - 'mimeversion' => 'Zend\Mail\Header\MimeVersion', - 'mime_version' => 'Zend\Mail\Header\MimeVersion', - 'mime-version' => 'Zend\Mail\Header\MimeVersion', - 'received' => 'Zend\Mail\Header\Received', - 'replyto' => 'Zend\Mail\Header\ReplyTo', - 'reply_to' => 'Zend\Mail\Header\ReplyTo', - 'reply-to' => 'Zend\Mail\Header\ReplyTo', - 'sender' => 'Zend\Mail\Header\Sender', - 'subject' => 'Zend\Mail\Header\Subject', - 'to' => 'Zend\Mail\Header\To', - )); + $this->pluginClassLoader = new Header\HeaderLoader(); } return $this->pluginClassLoader; } From 15188e1b7bac3f5492441fa25f394c998709530f Mon Sep 17 00:00:00 2001 From: Maks3w Date: Tue, 12 Jun 2012 15:02:45 +0200 Subject: [PATCH 5/6] [Mail] Change removeHeader signature --- src/Headers.php | 6 +++--- src/Message.php | 6 +----- src/Transport/Sendmail.php | 19 +++++----------- src/Transport/Smtp.php | 44 +++++++++++++++++--------------------- test/HeadersTest.php | 12 ++++++++++- test/MessageTest.php | 3 +++ 6 files changed, 43 insertions(+), 47 deletions(-) diff --git a/src/Headers.php b/src/Headers.php index a8e625af..684b4553 100644 --- a/src/Headers.php +++ b/src/Headers.php @@ -250,12 +250,12 @@ public function addHeader(Header\HeaderInterface $header) /** * Remove a Header from the container * - * @param Header\HeaderInterface $header + * @param string $fieldName * @return bool */ - public function removeHeader(Header\HeaderInterface $header) + public function removeHeader($fieldName) { - $key = $this->normalizeFieldName($header->getFieldName()); + $key = $this->normalizeFieldName($fieldName); $index = array_search($key, $this->headersKeys, true); if ($index !== false) { unset($this->headersKeys[$index]); diff --git a/src/Message.php b/src/Message.php index a15c0721..1d7b9efe 100644 --- a/src/Message.php +++ b/src/Message.php @@ -475,11 +475,7 @@ protected function getHeader($headerName, $headerClass) */ protected function clearHeaderByName($headerName) { - $headers = $this->headers(); - if ($headers->has($headerName)) { - $header = $headers->get($headerName); - $headers->removeHeader($header); - } + $this->headers()->removeHeader($headerName); } /** diff --git a/src/Transport/Sendmail.php b/src/Transport/Sendmail.php index 268c35ea..8bb81619 100644 --- a/src/Transport/Sendmail.php +++ b/src/Transport/Sendmail.php @@ -222,25 +222,16 @@ protected function prepareBody(Mail\Message $message) */ protected function prepareHeaders(Mail\Message $message) { - $headers = $message->headers(); - // On Windows, simply return verbatim if ($this->isWindowsOs()) { - return $headers->toString(); + return $message->headers()->toString(); } // On *nix platforms, strip the "to" header - $headersToSend = new Headers(); - foreach ($headers as $header) { - if ('To' == $header->getFieldName()) { - continue; - } - if ('Subject' == $header->getFieldName()) { - continue; - } - $headersToSend->addHeader($header); - } - return $headersToSend->toString(); + $headers = clone $message->headers(); + $headers->removeHeader('To'); + $headers->removeHeader('Subject'); + return $headers->toString(); } /** diff --git a/src/Transport/Smtp.php b/src/Transport/Smtp.php index 79f667b8..88a7a225 100644 --- a/src/Transport/Smtp.php +++ b/src/Transport/Smtp.php @@ -22,15 +22,16 @@ namespace Zend\Mail\Transport; use Zend\Loader\Pluggable, - Zend\Mail, + Zend\Mail\Address, Zend\Mail\Headers, + Zend\Mail\Message, Zend\Mail\Protocol, Zend\Mail\Protocol\Exception as ProtocolException; /** * SMTP connection object * - * Loads an instance of \Zend\Mail\Protocol\Smtp and forwards smtp transactions + * Loads an instance of Zend\Mail\Protocol\Smtp and forwards smtp transactions * * @category Zend * @package Zend_Mail @@ -213,9 +214,9 @@ public function disconnect() * The connection via the protocol adapter is made just-in-time to allow a * developer to add a custom adapter if required before mail is sent. * - * @param Mail\Message $message + * @param Message $message */ - public function send(Mail\Message $message) + public function send(Message $message) { // If sending multiple messages per session use existing adapter $connection = $this->getConnection(); @@ -249,14 +250,14 @@ public function send(Mail\Message $message) /** * Retrieve email address for envelope FROM * - * @param Mail\Message $message + * @param Message $message * @throws Exception\RuntimeException * @return string */ - protected function prepareFromAddress(Mail\Message $message) + protected function prepareFromAddress(Message $message) { $sender = $message->getSender(); - if ($sender instanceof Mail\Address\AddressInterface) { + if ($sender instanceof Address\AddressInterface) { return $sender->getEmail(); } @@ -275,11 +276,11 @@ protected function prepareFromAddress(Mail\Message $message) /** * Prepare array of email address recipients - * - * @param Mail\Message $message + * + * @param Message $message * @return array */ - protected function prepareRecipients(Mail\Message $message) + protected function prepareRecipients(Message $message) { $recipients = array(); foreach ($message->to() as $address) { @@ -297,29 +298,24 @@ protected function prepareRecipients(Mail\Message $message) /** * Prepare header string from message - * - * @param Mail\Message $message + * + * @param Message $message * @return string */ - protected function prepareHeaders(Mail\Message $message) + protected function prepareHeaders(Message $message) { - $headers = new Headers(); - foreach ($message->headers() as $header) { - if ('Bcc' == $header->getFieldName()) { - continue; - } - $headers->addHeader($header); - } + $headers = clone $message->headers(); + $headers->removeHeader('Bcc'); return $headers->toString(); } /** * Prepare body string from message - * - * @param Mail\Message $message + * + * @param Message $message * @return string */ - protected function prepareBody(Mail\Message $message) + protected function prepareBody(Message $message) { return $message->getBodyText(); } @@ -327,7 +323,7 @@ protected function prepareBody(Mail\Message $message) /** * Lazy load the connection, and pass it helo * - * @return Mail\Protocol\Smtp + * @return Protocol\Smtp */ protected function lazyLoadConnection() { diff --git a/test/HeadersTest.php b/test/HeadersTest.php index a47235f1..3cbad3c4 100644 --- a/test/HeadersTest.php +++ b/test/HeadersTest.php @@ -200,7 +200,7 @@ public function testHeadersCanRemoveHeader() $headers->addHeaders(array('Foo' => 'bar', 'Baz' => 'baz')); $header = $headers->get('foo'); $this->assertEquals(2, $headers->count()); - $headers->removeHeader($header); + $headers->removeHeader($header->getFieldName()); $this->assertEquals(1, $headers->count()); $this->assertFalse($headers->get('foo')); } @@ -316,4 +316,14 @@ public function testDefaultPluginLoaderIsSeededWithHeaders($plugin, $class) $test = $loader->load($plugin); $this->assertEquals($class, $test); } + + public function testClone() { + $headers = new Mail\Headers(); + $headers->addHeader(new Header\Bcc()); + $headers2 = clone($headers); + $this->assertEquals($headers, $headers2); + $headers2->removeHeader('Bcc'); + $this->assertTrue($headers->has('Bcc')); + $this->assertFalse($headers2->has('Bcc')); + } } diff --git a/test/MessageTest.php b/test/MessageTest.php index 9d77770d..5d9ce61f 100644 --- a/test/MessageTest.php +++ b/test/MessageTest.php @@ -40,6 +40,9 @@ */ class MessageTest extends \PHPUnit_Framework_TestCase { + /** @var Message */ + public $message; + public function setUp() { $this->message = new Message(); From 7cf67f0df01fe94e4f55fba1ebe41a0e99c468a9 Mon Sep 17 00:00:00 2001 From: Maks3w Date: Tue, 12 Jun 2012 23:18:55 +0200 Subject: [PATCH 6/6] [Mail] Various improvements. - Fix a issue with headers being rendered as CRCRLF with SMTP Protocol (ZF2-185) - Add test case to detect the above issue and trace the full command sended. - Add check in SMTP Transport for require at least one recipient if at least one DATA (header or body) will send per RFC-2821 --- src/Message.php | 2 +- src/Protocol/Smtp.php | 6 +-- src/Transport/Smtp.php | 10 +++- test/MessageTest.php | 3 +- test/Protocol/SmtpTest.php | 76 +++++++++++++++++++++++++++ test/TestAsset/SmtpProtocolSpy.php | 82 ++++++------------------------ test/Transport/SmtpTest.php | 29 +++++++++-- 7 files changed, 129 insertions(+), 79 deletions(-) create mode 100644 test/Protocol/SmtpTest.php diff --git a/src/Message.php b/src/Message.php index 1d7b9efe..153ed7f2 100644 --- a/src/Message.php +++ b/src/Message.php @@ -441,7 +441,7 @@ public function getBody() public function getBodyText() { if ($this->body instanceof Mime\Message) { - return $this->body->generateMessage(); + return $this->body->generateMessage(Headers::EOL); } return (string) $this->body; diff --git a/src/Protocol/Smtp.php b/src/Protocol/Smtp.php index 30e9a95e..ce4afc13 100644 --- a/src/Protocol/Smtp.php +++ b/src/Protocol/Smtp.php @@ -22,8 +22,6 @@ namespace Zend\Mail\Protocol; -use Zend\Mime\Mime; - /** * SMTP implementation of Zend\Mail\Protocol\AbstractProtocol * @@ -294,14 +292,14 @@ public function rcpt($to) public function data($data) { // Ensure recipients have been set - if ($this->_rcpt !== true) { + if ($this->_rcpt !== true) { // Per RFC 2821 3.3 (page 18) throw new Exception\RuntimeException('No recipient forward path has been supplied'); } $this->_send('DATA'); $this->_expect(354, 120); // Timeout set for 2 minutes as per RFC 2821 4.5.3.2 - foreach (explode(Mime::LINEEND, $data) as $line) { + foreach (explode(self::EOL, $data) as $line) { if (strpos($line, '.') === 0) { // Escape lines prefixed with a '.' $line = '.' . $line; diff --git a/src/Transport/Smtp.php b/src/Transport/Smtp.php index 88a7a225..47a8eb18 100644 --- a/src/Transport/Smtp.php +++ b/src/Transport/Smtp.php @@ -235,6 +235,14 @@ public function send(Message $message) $headers = $this->prepareHeaders($message); $body = $this->prepareBody($message); + if ((count($recipients) == 0) && (!empty($headers) || !empty($body))) { + throw new Exception\RuntimeException( // Per RFC 2821 3.3 (page 18) + sprintf( + '%s transport expects at least one recipient if the message has at least one header or body', + __CLASS__ + )); + } + // Set sender email address $connection->mail($from); @@ -262,7 +270,7 @@ protected function prepareFromAddress(Message $message) } $from = $message->from(); - if (!count($from)) { + if (!count($from)) { // Per RFC 2822 3.6 throw new Exception\RuntimeException(sprintf( '%s transport expects either a Sender or at least one From address in the Message; none provided', __CLASS__ diff --git a/test/MessageTest.php b/test/MessageTest.php index 5d9ce61f..1f72db03 100644 --- a/test/MessageTest.php +++ b/test/MessageTest.php @@ -25,6 +25,7 @@ Zend\Mail\Address, Zend\Mail\AddressList, Zend\Mail\Header, + Zend\Mail\Headers, Zend\Mail\Message, Zend\Mime\Message as MimeMessage, Zend\Mime\Mime, @@ -596,7 +597,7 @@ public function testRetrievingBodyTextFromMessageWithMultiPartMimeBodyReturnsMim $this->message->setBody($body); $text = $this->message->getBodyText(); - $this->assertEquals($body->generateMessage(), $text); + $this->assertEquals($body->generateMessage(Headers::EOL), $text); $this->assertContains('--foo-bar', $text); $this->assertContains('--foo-bar--', $text); $this->assertContains('Content-Type: text/plain', $text); diff --git a/test/Protocol/SmtpTest.php b/test/Protocol/SmtpTest.php new file mode 100644 index 00000000..60a926f8 --- /dev/null +++ b/test/Protocol/SmtpTest.php @@ -0,0 +1,76 @@ +transport = new Smtp(); + $this->connection = new SmtpProtocolSpy(); + $this->transport->setConnection($this->connection); + } + + public function testSendMinimalMail() { + $headers = new Headers(); + $headers->addHeaderLine('Date', 'Sun, 10 Jun 2012 20:07:24 +0200'); + $message = new Message(); + $message + ->setHeaders($headers) + ->setSender('ralph.schindler@zend.com', 'Ralph Schindler') + ->setBody('testSendMailWithoutMinimalHeaders') + ->addTo('zf-devteam@zend.com', 'ZF DevTeam') + ; + $expectedMessage = "RSET\r\n" + . "MAIL FROM:\r\n" + . "DATA\r\n" + . "Date: Sun, 10 Jun 2012 20:07:24 +0200\r\n" + . "Sender: Ralph Schindler \r\n" + . "To: ZF DevTeam \r\n" + . "\r\n" + . "testSendMailWithoutMinimalHeaders\r\n" + . ".\r\n"; + + $this->transport->send($message); + + $this->assertEquals($expectedMessage, $this->connection->getLog()); + } +} diff --git a/test/TestAsset/SmtpProtocolSpy.php b/test/TestAsset/SmtpProtocolSpy.php index 618e42f4..6d349675 100644 --- a/test/TestAsset/SmtpProtocolSpy.php +++ b/test/TestAsset/SmtpProtocolSpy.php @@ -38,101 +38,59 @@ class SmtpProtocolSpy extends Smtp protected $helo; protected $mail; protected $rcpt = array(); - protected $data; + protected $_sess = true; - /** - * "Connect" to server - * - * @return void - */ public function connect() { $this->connect = true; + return true; } - /** - * Set server name we're talking to - * - * @param string $serverName - * @return void - */ public function helo($serverName = '127.0.0.1') { + parent::helo($serverName); $this->helo = $serverName; } - /** - * quit implementation - * - * Resets helo value and calls rset - * - * @return void - */ public function quit() { $this->helo = null; $this->rset(); } - /** - * Disconnect implementation - * - * Resets connect flag and calls rset - * - * @return void - */ public function disconnect() { $this->helo = null; $this->connect = false; $this->rset(); } - - /** - * "Reset" connection - * - * Resets state of mail, rcpt, and data properties - * - * @return void - */ + public function rset() { - $this->mail = null; + parent::rset(); $this->rcpt = array(); - $this->data = null; } - /** - * Set envelope FROM - * - * @param string $from - * @return void - */ public function mail($from) { + parent::mail($from); $this->mail = $from; } - /** - * Add recipient - * - * @param string $to - * @return void - */ public function rcpt($to) { + $this->_rcpt = true; $this->rcpt[] = $to; } - /** - * Set data - * - * @param string $data - * @return void - */ - public function data($data) + protected function _send($request) { - $this->data = $data; + // Save request to internal log + $this->_addLog($request . self::EOL); + } + + protected function _expect($code, $timeout = null) { + return ''; } /** @@ -174,14 +132,4 @@ public function getRecipients() { return $this->rcpt; } - - /** - * Get data value - * - * @return null|string - */ - public function getData() - { - return $this->data; - } -} +} \ No newline at end of file diff --git a/test/Transport/SmtpTest.php b/test/Transport/SmtpTest.php index da199142..19a84c20 100644 --- a/test/Transport/SmtpTest.php +++ b/test/Transport/SmtpTest.php @@ -68,13 +68,29 @@ public function getMessage() return $message; } + /** + * Per RFC 2822 3.6 + */ public function testSendMailWithoutMinimalHeaders() { $this->setExpectedException( 'Zend\Mail\Transport\Exception\RuntimeException', 'transport expects either a Sender or at least one From address in the Message; none provided' ); $message = new Message(); - $message->setBody('testSendMailWithoutMinimalHeaders'); + $this->transport->send($message); + } + + /** + * Per RFC 2821 3.3 (page 18) + * - RCPT (recipient) must be called before DATA (headers or body) + */ + public function testSendMailWithoutRecipient() { + $this->setExpectedException( + 'Zend\Mail\Transport\Exception\RuntimeException', + 'at least one recipient if the message has at least one header or body' + ); + $message = new Message(); + $message->setSender('ralph.schindler@zend.com', 'Ralph Schindler'); $this->transport->send($message); } @@ -86,14 +102,17 @@ public function testSendMinimalMail() { ->setHeaders($headers) ->setSender('ralph.schindler@zend.com', 'Ralph Schindler') ->setBody('testSendMailWithoutMinimalHeaders') + ->addTo('zf-devteam@zend.com', 'ZF DevTeam') ; $expectedMessage = "Date: Sun, 10 Jun 2012 20:07:24 +0200\r\n" - . "Sender: Ralph Schindler \r\n" - . "\r\ntestSendMailWithoutMinimalHeaders"; + . "Sender: Ralph Schindler \r\n" + . "To: ZF DevTeam \r\n" + . "\r\n" + . "testSendMailWithoutMinimalHeaders"; $this->transport->send($message); - $this->assertSame($expectedMessage, $this->connection->getData()); + $this->assertContains($expectedMessage, $this->connection->getLog()); } public function testReceivesMailArtifacts() @@ -105,7 +124,7 @@ public function testReceivesMailArtifacts() $expectedRecipients = array('zf-devteam@zend.com', 'matthew@zend.com', 'zf-crteam@lists.zend.com'); $this->assertEquals($expectedRecipients, $this->connection->getRecipients()); - $data = $this->connection->getData(); + $data = $this->connection->getLog(); $this->assertContains('To: ZF DevTeam ', $data); $this->assertContains('Subject: Testing Zend\Mail\Transport\Sendmail', $data); $this->assertContains("Cc: matthew@zend.com\r\n", $data);