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

Add email config for transferEncoding #11485 #11488

Merged
merged 7 commits into from Dec 6, 2017
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
39 changes: 38 additions & 1 deletion src/Mailer/Email.php
Expand Up @@ -241,6 +241,14 @@ class Email implements JsonSerializable, Serializable
*/
public $headerCharset;

/**
* The email transfer encoding used.
* If null, the $charset property is used for determined the transfer encoding.
*
* @var string|null
*/
public $transferEncoding;
Copy link
Member

Choose a reason for hiding this comment

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

Does it make sense to add another public properties and a pair of getter/setter for that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think no, but i follow what was done.
You can see the charset or headerCharset properties.

Copy link
Member

Choose a reason for hiding this comment

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

Those are public for BC reasons I suppose. I think we can make the new property protected.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah the property should be protected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok i fix it


/**
* The application wide charset, used to encode headers and body
*
Expand Down Expand Up @@ -827,6 +835,29 @@ public function headerCharset($charset = null)
return $this->headerCharset;
}

/**
* TransferEncoding setter.
*
* @param string|null $encoding Encoding set.
* @return $this
*/
public function setTransferEncoding($encoding)
Copy link
Member

Choose a reason for hiding this comment

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

Should we be handling encoding the message based on the indicated transfer encoding? Right now someone could set the transfer encoding to base64, but the email message will still be in 8bit (generally).

Copy link
Contributor Author

@maratth maratth Dec 6, 2017

Choose a reason for hiding this comment

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

I think they are two different parts. I don't know if have a link between transfer encoding and message encoding.

Copy link
Member

Choose a reason for hiding this comment

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

Ok.

{
$this->transferEncoding = $encoding;
Copy link
Member

Choose a reason for hiding this comment

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

Should we validate that for a valid/known transfer encoding?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know all content transfer encoding. But I found this.
I understand that there is BASE64 / QUOTED-PRINTABLE / 8BIT / 7BIT / BINARY / x-token but I'm not sure, I'm not a expert.

If you think is good, I can implement it.

Copy link
Member

Choose a reason for hiding this comment

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

I think checking the standard types (not x-token) is a good place to start.


return $this;
}

/**
* TransferEncoding getter.
*
* @return string|null Encoding
*/
public function getTransferEncoding()
{
return $this->transferEncoding;
}

/**
* EmailPattern setter/getter
*
Expand Down Expand Up @@ -2224,6 +2255,7 @@ public function reset()
$this->_priority = null;
$this->charset = 'utf-8';
$this->headerCharset = null;
$this->transferEncoding = null;
$this->_attachments = [];
$this->_profile = [];
$this->_emailPattern = self::EMAIL_PATTERN;
Expand Down Expand Up @@ -2655,12 +2687,17 @@ protected function _renderTemplates($content)
}

/**
* Return the Content-Transfer Encoding value based on the set charset
* Return the Content-Transfer Encoding value based
* on the set transferEncoding or set charset.
*
* @return string
*/
protected function _getContentTransferEncoding()
{
if ($this->transferEncoding) {
return $this->transferEncoding;
}

$charset = strtoupper($this->charset);
if (in_array($charset, $this->_charset8bit)) {
return '8bit';
Expand Down
29 changes: 29 additions & 0 deletions tests/TestCase/Mailer/EmailTest.php
Expand Up @@ -90,6 +90,17 @@ public function render($content)
{
return $this->_render($content);
}

/**
* GetContentTransferEncoding to protected method
*
* @return string
*/
public function getContentTransferEncoding()
{
return $this->_getContentTransferEncoding();
}

}

/**
Expand Down Expand Up @@ -2487,6 +2498,24 @@ public function testHeaderCharset()
$this->assertSame($charset, 'Shift_JIS');
}

/**
* Test transferEncoding
*
* @return void
*/
public function testTransferEncoding()
{
// Test new transfer encoding
$this->Email->setTransferEncoding('quoted-printable');
$this->assertSame($this->Email->getTransferEncoding(), 'quoted-printable');
Copy link
Member

Choose a reason for hiding this comment

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

Order is wrong, should be: expected, result.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hum, sorry i fix this in #a006420
But i see, the tests above have the same issue.

$this->assertSame($this->Email->getContentTransferEncoding(), 'quoted-printable');

// Test default charset/encoding : utf8/8bit
$this->Email->reset();
$this->assertNull($this->Email->getTransferEncoding());
$this->assertSame($this->Email->getContentTransferEncoding(), '8bit');
}

/**
* Tests for compatible check.
* charset property and charset() method.
Expand Down