CRM-21521: fix nested multipart handling in bounce processing #11390
Conversation
| copy(__DIR__ . '/data/bounces/' . $mail, __DIR__ . '/data/mail/' . $mail); | ||
| $this->callAPISuccess('job', 'fetch_bounces', array()); | ||
| $this->assertFalse(file_exists(__DIR__ . '/data/mail/' . $mail)); | ||
| $this->checkMailingBounces(1); |
There was a problem hiding this comment.
@ejegg Just wondering if its worth while checking for a specific type and or message stored in the db associated with the bounce also?
|
Any thoughts on the 'style error'? I can't figure out what it's complaining about. |
CRM/Utils/Mail/EmailProcessor.php
Outdated
| } | ||
| elseif ($multipart instanceof ezcMailMultipartRelated) { | ||
| $text = self::getTextFromMultipartRelated($multipart, $recursionLevel); | ||
| } else { |
There was a problem hiding this comment.
else needs to be on a new line
CRM/Utils/Mail/EmailProcessor.php
Outdated
| foreach ($multipart->getParts() as $part) { | ||
| if (isset($part->subType) and $part->subType === 'plain') { | ||
| $text = $part->text; | ||
| } elseif ($part instanceof ezcMailMultipart) { |
There was a problem hiding this comment.
elseif needs to be on a new line
CRM/Utils/Mail/EmailProcessor.php
Outdated
| foreach ($related->getRelatedParts() as $part) { | ||
| if (isset($part->subType) and $part->subType === 'plain') { | ||
| $text = $part->text; | ||
| } elseif ($part instanceof ezcMailMultipart) { |
There was a problem hiding this comment.
elseif needs to be on a new line
| } | ||
| } | ||
| return $text; | ||
| } |
There was a problem hiding this comment.
need a blank line between this closing brace and the class closing brace
There was a problem hiding this comment.
@ejegg you need 1 complete blank line after this brace but before the } for the class
|
@ejegg see my comments just now |
|
Thanks seamuslee001! I can squash all those changes together to make the commit history cleaner |
ejegg
force-pushed
the
bounceProcessing
branch
from
b233375 to
b80b567
Compare
ejegg
changed the title
|
@seamuslee001 this is all squashed and passing tests! |
|
nice work @ejegg :-) I'll see if i can get a colleague of mine to review it |
CRM/Utils/Mail/EmailProcessor.php
Outdated
| $text = $part->text; | ||
| } | ||
| elseif ($part instanceof ezcMailMultipart) { | ||
| $text = self::getTextFromMultipart($part); |
There was a problem hiding this comment.
Don't we need to be passing through the $recussionLevel here?
ejegg
force-pushed
the
bounceProcessing
branch
2 times, most recently
from
cda8193 to
cfa8dbd
Compare
|
Good catch @seamuslee001, fixed! |
ejegg
force-pushed
the
bounceProcessing
branch
from
cfa8dbd to
cda8193
Compare
|
I can't merge this as @ejegg is a colleague but I merged the same patch into our wmf & want to add some notes as to the review I did there
This code will only be reached when processing a multipart email & we have assurances from the added unit test that we have not broken that. |
|
I'm a little wary of adding global constants because of potential namespace conflicts. Any reason MAIL_MAX_RECURSION can't be added to the class instead of globally? |
ejegg
force-pushed
the
bounceProcessing
branch
from
cda8193 to
ab48be7
Compare
When for example a multipart-report email body's first part is multipart-related, extract the text correctly instead of crashing. This logic should handle all kinds of weird multipart nesting, up to MIME_MAX_RECURSION levels deep.
ejegg
force-pushed
the
bounceProcessing
branch
from
ab48be7 to
01467ae
Compare
|
Thanks for taking a look @colemanw! I've changed the MAX_RECURSION bit to a class constant. |
|
Looks like some unrelated DB errors in the testing setup |
|
jenkins re-test this please |
1 similar comment
|
jenkins re-test this please |
|
test this please |
|
I'm satisfied that this has had enough unit testing and code review to merge. Good work @ejegg |
When for example a multipart-report email body's first part is
multipart-related, extract the text correctly instead of crashing.
This logic should handle all kinds of weird multipart nesting, up
to MIME_MAX_RECURSION levels deep.
Overview
Makes email bounce processing more robust
Before
Bounce processing crashes when a multipart-report email body's first part is multipart-related
After
Bounce processing correctly extracts arbitrarily nested multiparts
Technical Details
Pulls various kinds of multipart extraction into their own functions that can be called recursively. Gives up after a set recursion depth.