Add email config for transferEncoding #11485 #11488
Conversation
src/Mailer/Email.php
Outdated
| * @param string|null $encoding Encoding set. | ||
| * @return $this | ||
| */ | ||
| public function setTransferEncoding($encoding) { |
There was a problem hiding this comment.
Opening brace should be on a new line
src/Mailer/Email.php
Outdated
| * | ||
| * @return string|null Encoding | ||
| */ | ||
| public function getTransferEncoding() { |
There was a problem hiding this comment.
Opening brace should be on a new line
tests/TestCase/Mailer/EmailTest.php
Outdated
| * | ||
| * @return string | ||
| */ | ||
| public function getContentTransferEncoding() { |
There was a problem hiding this comment.
Opening brace should be on a new line
tests/TestCase/Mailer/EmailTest.php
Outdated
| * | ||
| * @return void | ||
| */ | ||
| public function testTransferEncoding(){ |
There was a problem hiding this comment.
Opening brace should be on a new line
tests/TestCase/Mailer/EmailTest.php
Outdated
| public function testTransferEncoding(){ | ||
| // Test new transfer encoding | ||
| $this->Email->setTransferEncoding('quoted-printable'); | ||
| $this->assertSame($this->Email->getTransferEncoding(), 'quoted-printable'); |
There was a problem hiding this comment.
Order is wrong, should be: expected, result.
There was a problem hiding this comment.
Hum, sorry i fix this in #a006420
But i see, the tests above have the same issue.
src/Mailer/Email.php
Outdated
| * | ||
| * @var string|null | ||
| */ | ||
| public $transferEncoding; |
There was a problem hiding this comment.
Does it make sense to add another public properties and a pair of getter/setter for that?
There was a problem hiding this comment.
I think no, but i follow what was done.
You can see the charset or headerCharset properties.
There was a problem hiding this comment.
Those are public for BC reasons I suppose. I think we can make the new property protected.
There was a problem hiding this comment.
Yeah the property should be protected.
Codecov Report
@@ Coverage Diff @@
## master #11488 +/- ##
============================================
+ Coverage 93.37% 93.39% +0.01%
- Complexity 13015 13019 +4
============================================
Files 436 436
Lines 32751 32765 +14
============================================
+ Hits 30582 30600 +18
+ Misses 2169 2165 -4
Continue to review full report at Codecov.
|
| */ | ||
| public function setTransferEncoding($encoding) | ||
| { | ||
| $this->transferEncoding = $encoding; |
There was a problem hiding this comment.
Should we validate that for a valid/known transfer encoding?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I think checking the standard types (not x-token) is a good place to start.
|
Is possible add this pull request for cakephp v3.4.x or is only for 3.5.x ? |
We're only doing security fixes for 3.4 at this point in time. The upgrade from 3.4 to 3.5 should be pretty painless if you need this in your app. |
| * @param string|null $encoding Encoding set. | ||
| * @return $this | ||
| */ | ||
| public function setTransferEncoding($encoding) |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
I think they are two different parts. I don't know if have a link between transfer encoding and message encoding.
7 participants
Add getter/setter for transferEncoding and modify the
_getContentTranferEncodingfor use transferEncoding if is set.Write test in
EmailTestfor check it.