search

jdavidbakr / mail-tracker

Package for Laravel to inject tracking code into outgoing emails.
MIT License
567 stars 129 forks source link

Add getAllHeaders method and fix incorrect handling of multi-line headers #190

Closed Jason-Morcos closed 1 year ago

Jason-Morcos commented 2 years ago

The getHeader method currently does not handle headers that span multiple lines of the DB field. This is evident when getting the "CC" header value of an email Ccd to many (~8) users. This PR fixes the regex to count lines that start with a space as part of the previous header instead of as their own header.

Additionally, this PR introduces a new getAllHeaders helper method that encapsulates the logic held within the getHeader method to allow users to implement custom display or handling of all the headers, working with them in collection form

jdavidbakr commented 2 years ago

Thanks - it looks like a test is failing now, and also please include tests for your changes to verify that it works as previously expected as well as in the new condition.

Jason-Morcos commented 2 years ago

Thanks - it looks like a test is failing now, and also please include tests for your changes to verify that it works as previously expected as well as in the new condition.

Added tests for parsing of long/multi-line headers and confirmed they're failing on the current master branch and passing on my test branch.

That test you mentioned that's failing on my PR is also failing on the current master branch for me locally and seems completely unrelated to this PR: MailTrackerTest::testSendMessageWithRelatedPart

Jason-Morcos commented 2 years ago

@jdavidbakr I'm fairly certain that test is failing on the master branch. I added additional testing to my PR for the changes made. Is there anything further you'd like me to do with this PR?

jdavidbakr commented 2 years ago

This is the master branch build: https://app.travis-ci.com/github/jdavidbakr/mail-tracker/builds/251379626 - so I'm not sure why your pull request is failing. I did check out your PR locally and it tested fine so I'm not sure exactly what's happening, but unfortunately don't have time to investigate what is causing it to break right now.

Jason-Morcos commented 1 year ago

@jdavidbakr Hey! Thanks for merging the other PR related to multi-part messages. I think after this PR is updated from the fixed master branch it will be passing test cases. Was there a particular reason you closed the PR?

Jason-Morcos commented 1 year ago

I've updated this branch so, I believe, if you reopened this PR it should now be passing test cases. I just confirmed it is now passing locally

jdavidbakr commented 1 year ago

Great - thanks for resolving the tests!