Travis: fix failing tests #172
Conversation
| return ( | ||
| 'R' === $this->getHeaders()->get('recent') | ||
| || ('' === $this->getHeaders()->get('recent') && '' !== $this->getHeaders()->get('unseen')) | ||
| ); |
There was a problem hiding this comment.
| @@ -48,7 +48,7 @@ private function parseHeader($key, $value) | |||
| case 'draft': | |||
| // no break | |||
| case 'unseen': | |||
| return (bool)trim($value); | |||
| return $value; | |||
There was a problem hiding this comment.
Flags can't be force to boolean because they can have multiple values, like recent:
https://secure.php.net/manual/en/function.imap-headerinfo.php
| // $this->decodedContent, | ||
| // $this->getCharset() | ||
| // ); | ||
| // } |
There was a problem hiding this comment.
After commenting this code no test fails, and testEncoding7Bit passes: @ddeboer do you have any test case that justify this piece of code?
There was a problem hiding this comment.
This was a fix for #3, so also testSubjectEncoding().
There was a problem hiding this comment.
I don't know what changed since november 2013, but the rule is that if removing code doesn't break tests, it's either:
- the tests are not enough
- the code is unnecessary
But don't be afraid: we need a lot of test cases with different charset to test against, and I will gather and test them, in future PR, of couse before the any next release.
Just not now: before deeping into the charset topic, we need minor improvement like coding standards and re-enable code-coverage; one step at time
There was a problem hiding this comment.
Right: I hate commented code too and for sure will be removed soon.
One step at time, reminder added: #174
| // imap_utf8 doesn't seem to work properly, so use Transcoder instead | ||
| $decoded .= Transcoder::create()->transcode($part->text, $charset); | ||
| // $decoded .= Transcoder::create()->transcode($part->text, $charset); | ||
| $decoded .= $part->text; |
There was a problem hiding this comment.
Like Message\Part also headers are affected: when the raw value is used, no test fails and testEncoding7Bit passes. @ddeboer do you have any test case that justify this piece of code?
There was a problem hiding this comment.
No, so let’s disable it for now.
| @@ -101,6 +102,7 @@ public function testDelete() | |||
|
|
|||
| $message = $this->mailbox->getMessage(3); | |||
| $message->delete(); | |||
| $this->mailbox->expunge(); | |||
There was a problem hiding this comment.
Trivial bugfix. Manual says:
Marks messages listed in msg_number for deletion. Messages marked for deletion will stay in the mailbox until either imap_expunge() is called or imap_close() is called with the optional parameter CL_EXPUNGE.
There was a problem hiding this comment.
If we only fix this here, this may not be transparent to users. Should we also add a doc comment to Message::delete()?
There was a problem hiding this comment.
So, since delete doesn't delete the message, I suggest:
- Rename
deletemethod tomarkAsDelete - Add to the doc that
mailbox::expungemust be called for a complete delete
Reminder: #175
| . "--$boundary\r\n" | ||
| . "Content-Transfer-Encoding: quoted-printable\r\n" | ||
| . "Content-Type: text/html; charset=\"windows-1252\"\r\n" | ||
| . "\r\n" | ||
| . "<html><body>Espa=F1a</body></html>\r\n\r\n" | ||
| . "<html><body>Espa=C3=B1a</body></html>\r\n\r\n" |
There was a problem hiding this comment.
Why did you change this? The original text was received in a bug report.
There was a problem hiding this comment.
This is the change I was least comfortable with.
Even though F1 is the right UTF-8 code for n with tilde, I was unable to reproduce in a real email the character written like this. Instead every mail agent uses =C3=B1.
I think we need to refactor test assets as a whole:
- Write emails within PHP in pure UTF-8
- Use an external tool/library/ext to convert them into different charsets
- Send them to Dovecot (already done)
- Test this library reads the result in UTF-8 (already done)
String concatenation seems too weak to handle the encoding topic properly (in the tests).
There was a problem hiding this comment.
Fair enough. We can use ddeboer/transcoder (or some other lib) for step 2.
So, here we are. Comments on code.