getBodyHtml only returns first part of Mail #454
Conversation
| $this->mailbox->addMessage($this->getFixture('HTMLmultipartMessage')); | ||
|
|
||
| $message = $this->mailbox->getMessage(1); | ||
|
|
There was a problem hiding this comment.
This is a good start. What it's missing here are the expectation written in form of assertions, for example:
static::assertSame('<html>', $message->getBodyHtml());What should you assert? What you expect, so if you told in the issue only returns the first part of a multipart/mixed message test the returned content in a way you describe your expectations.
Added assert for <html>
tests/MessageTest.php
Outdated
|
|
||
| $message = $this->mailbox->getMessage(1); | ||
|
|
||
| static::assertSame('<html>', $message->getBodyHtml()); |
There was a problem hiding this comment.
I'm sorry, but this is not a game of copy-paste, I don't know your expectations, it is up to you to write them in a way the bug you reported could emerge.
There was a problem hiding this comment.
Hi,
first of all thanks for your patience. As I pointed out I'm new to this, so I simply didn't know that saving the file updates the pull request. Sorry for that.
I hope the asserts are correct now.
Kind regards
Kevin
There was a problem hiding this comment.
Your test is now clear and self-contained.
Finding the solution is another story though...
There was a problem hiding this comment.
Just out of curiosity, can you explain why this is happening?
Added asserts for all HTML Parts
|
Hi, I started to debug the library a little bit and found a solution for splitted HTML Messages. I tested it with ~100 different HTML Mails. Everything seems to work fine. I hope this helps. Kind reagrds |
|
Hi @Slamdunk, any Updates on this? Kind regards |
| if (self::SUBTYPE_MIXED === $part->getSubtype()) { | ||
| foreach ($part->getParts() as $mixedPart) { | ||
| if(self::SUBTYPE_HTML === $mixedPart->getSubtype()) { | ||
| $mixedHTML .= $mixedPart->getDecodedContent(); |
There was a problem hiding this comment.
Hi, with this you've effectively passed the test, but considering the fix as a whole it seems to me the solution is worsening the issue.
If you don't know the email has multiple HTML parts, with this PR the user gets an invalid HTML syntax (content simplified):
<html><body>First Part HTML Message</body></html>
<html><body>Second part HTML Message</body></html>
<html><body>Last Part HTML Message</body></html>The same issue is in place for getBodyText.
The only clear solution I have is to rename getBody(Html|Text) to getFirstBody(Html|Text), so the user knows the API returns something that could be partial, and maybe provide a new API to fetch all readable parts.
The current getBody(Html|Text) should be marked as deprecated, and to be removed in the next major version.
There was a problem hiding this comment.
Hi,
I'm aware of the invalid HTML Syntax problem. I assumed that this would be kind of desired because it is the original content of the Mail. I wasn't sure if you wanted the library to manipulate it.
In my own code I'm using DOMDocument() to fix this. Maybe another function implementing DOMDocument() into the library might be the solution here to.
Kind regards
Kevin
There was a problem hiding this comment.
There was a problem hiding this comment.
I'd like to retain the current behavior for the already implemented APIs.
If you still want to propose a solution, I'd prefer see new APIs for the issue.
There was a problem hiding this comment.
Hi @Slamdunk,
I can take a look at it, when I have some spare time. Would the following be in your interest:
1.) New Function to check if there are multiple HTML Parts (returns true/false)
2.) New Function to retrieve those original but invalid HTML Parts (Array)
Regarding the invalid HTML Structure:
Should there also be a function to transform the invalid HTML Structure to a valid Structure?
I'm not sure if there is a generally valid Solution for this. And even if there is, I'm not sure if it should be done by this library.
There was a problem hiding this comment.
1.) New Function to check if there are multiple HTML Parts (returns true/false)
The solution should also consider multiple TEXT/PLAIN parts.
I'm not sure if there is a generally valid Solution for this. And even if there is, I'm not sure if it should be done by this library.
Me neither.
There was a problem hiding this comment.
Regarding the Text/Plain Parts:
I've never seen such a mail. Are you sure this can happen?
These splitted HTML Parts can only exist if there are embedded attachments; at least as far as I know. I don't see a way how this can happen with a text/plain mail.
No description provided.