-
Notifications
You must be signed in to change notification settings - Fork 60
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Ensure headers are strings, not NULL if not set #95
base: master
Are you sure you want to change the base?
Conversation
2326fe1
to
88fd961
Compare
I'll have a look at the patch tomorrow, but I'd be much happier if you could add test cases as well.
cheers
Derick
|
@derickr thanks for your super quick response - I think we need to do a bit more work on our side to figure out if we have hacked your package for good reasons or bad .... so tomorrow is a bit premature. I'll update this when I'm a bit clearer as to whether there is a solid use case for another option here or whether we did something weird a long time ago & have been carrying it every since |
It seems as though CiviCRM's use case where we want the body of the email processed as text no matter what but if there is .txt files or similar or basically any files attached to be processed as attachments is different to https://github.com/zetacomponents/Mail/blob/master/tests/parser/parser_test.php#L1769. I'm not sure if its something about how kmail works but maybe this is just an incompatibility between Civi's use case and this library's expectations. |
@derickr it would be great to get your feedback on the right approach - we are working with the sample data below - ie a multipart message with
The goal is to have the text attachment (which could be a data file or an ical I believe) to still be an attachment without turning the body text & body html into attachments If we set
Then we wind up with 5 attachments & no body text If we set
We wind up with 2 attachments and our attachment file appended to the body My suspicion is that we are hitting behaviour in the multipart flow that was intended for the non-multipart
|
OK. Here I need to dive in a lot deeper :-). It's been a while since I've really looked in-depth at this code. |
@derickr did you have a chance to think any more about this? |
I have indeed, and I think the best way here would be to introduce another flag. I'll see if I can come up with something. |
I have had a look again, and created a branch with two test cases, and I don't see your "We wind up with 2 attachments and our attachment file appended to the body" allegation: https://github.com/zetacomponents/Mail/blob/test-alternative-text-attachment/tests/parser/parser_test.php#L1866 In both cases, I see the following structure:
The Do you do any other processing, or any other way of parsing perhaps? |
@derickr I think the expectation differences is shown in these lines fc981b4#diff-657e3b25123d0680407b0c9651027d42fe4f46bb7199ff757550900b030ef008R1936 and specifically I think in CiviCRM's case we want to always have the HTML and Text Bodies as |
Right - the body of an email is not an 'attachment' from a user point of view - and if the body is bundled up as an attachment the same as the 'real' attachments it's hard to present that information in a way that is meaningful to a user |
OK. I added a new option to the branch in PR #96, which allows for creation of ezcMailFile objects for multipart/mixed, but leaves the sub-parts of multipart/alternative alone. |
This is a patch we have been carrying for a while - in our tests we have some examples of emails where these headers are not set and the attached file is (e.g. an .ics). I think that is possible for real emails. This was our original issue https://lab.civicrm.org/dev/core/-/issues/940