Making into disposition string to lowercase to fix the comparision issue - #566
Making into disposition string to lowercase to fix the comparision issue#566ambrous wants to merge 1 commit into
Conversation
|
@ambrous this change seems to break something. Please rebase and solve this issue. Otherwise I'll close this pull request in the future. |
|
@Sebi94nbg , I've rebased. However, I don't know how to recheck again by codeclimate. |
|
@bapcltd-marv I'm kinda unsure about this change. Does it make sense to you? Would you merge it? |
|
I think it is useful fix as sometime attachments are lost due to the low case of the disposition. |
Sebbo94BY
left a comment
Sebbo94BY
left a comment
There was a problem hiding this comment.
Sorry, forgot to publish my review. 😞
| * @return IncomingMailAttachment $attachment | ||
| */ | ||
| public function downloadAttachment(DataPartInfo $dataInfo, array $params, object $partStructure, bool $emlOrigin = false): IncomingMailAttachment | ||
| public function downloadAttachment(DataPartInfo $dataInfo, array $params, object $partStructure, bool $emlOrigin = false, bool $dispositionAttachment = false): IncomingMailAttachment |
There was a problem hiding this comment.
You are adding a new argument here and set it by default to false, but it seems like as it's never used, so it's always false. Isn't it?
There was a problem hiding this comment.
No, please see the whole changed function not only my changes.
| public function downloadAttachment(DataPartInfo $dataInfo, array $params, object $partStructure, bool $emlOrigin = false, bool $dispositionAttachment = false): IncomingMailAttachment | ||
| { | ||
| if ('RFC822' == $partStructure->subtype && isset($partStructure->disposition) && 'attachment' == $partStructure->disposition) { | ||
| if ('RFC822' == $partStructure->subtype && isset($partStructure->disposition) && $dispositionAttachment) { |
There was a problem hiding this comment.
I'm still not sure, if this is a good replacement. I think, this will break (also) something.
Before: string comparisation
After: boolean comparisation
There was a problem hiding this comment.
This function skipped attachments in some emails due to case sensitive comparison.
I've just put almost the same comparison (but now it is not case sensitive) in one place and then use the boolean replacement of it, kind of some refactoring)
|
Superseeded by #750. This issue should be solved now. |
2 participants
Hi!
I faced an issue that in some cases field disposition comes in upper case and due to that comparisions of
'attachment' == $partStructure->dispositionfaild. It seems that it happens with outlook attachments.Also, I'm not sure that is was a good idea to add an additional param to function of downloadAttachment, but I suppose it looks a bit more clear.