dev/core#5569 Replace our hack on zeta components with upstream patch #31450
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷 Introduction for new contributors...
Quick links for reviewers...
|
|
The issue associated with the Pull Request can be viewed at https://lab.civicrm.org/dev/core/-/issues/5569 |
|
Thanks @eileenmcnaughton I can take a look. |
|
One problem I'm seeing is that if you have, say, a pdf attachment, but just a text body (or just an html body although technically that's invalid but it happens), then the result is no body and the body becomes a .unknown attachment. In other words, if it's multipart/mixed but there is no multipart/alternative within to "contain" the text or html, and the text or html is just its own section in the multipart/mixed. This works as expected with our current code. Like this: |
|
So it's not quite that it should decide based on multipart/mixed. If a text/plain section is in multpart/mixed and the disposition is attachment (like our old patch) then it should be an attachment. If a text/plain section is part of a multipart/alternative, then the odds are it's intended to be part of the body (although this wouldn't always be true - it would depend on the context like if there's only one other part and it's text/html - that's a bit hard to put into code). |
|
Maybe I don't understand something, but there seems to be some processing going on your side after the email has been parsed. So what would stop civicrm from setting the original Zeta Mail's PR (zetacomponents/Mail#96) did implement what you originally asked... |
|
Maybe we can open an internal discussion ticket, but taking a step back the underlying problem is that civi instead of storing the original source, stores a made-up pseudo-mime format that loses context, which has come up in other scenarios too (e.g. displaying line breaks). So we have to do guesswork to put it into that format. The request that you implemented does solve the specific example that was given at the time. Thank you. But I don't think we covered everything. |
|
Okay... then let me know if you think you need more changes to the Zeta Mail's PR? Otherwise I will merge it. |
|
If it's merged that's fine just we would still need our patch, which looks at disposition to determine whether it's a file or inline. I don't know if such a check makes sense upstream or not - it partly conflicts with the intent of the parseXXXAsFiles options as I understand them. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
The upstream maintainer has put up a patch to address our issue in the upstream repo - see zetacomponents/Mail#96 - this switches to using that (in preparation for upgrading to a branch which already has it merged)
Before
our special hack
After
a path forwards
Technical Details
There is specific test cover in
testFetchActivitiesWithManyAttachments()Comments