Skip to content

Conversation

@pabzm
Copy link
Member

@pabzm pabzm commented Feb 19, 2025

This probably wasn't implemented previously because HTML-parts usually didn't run through get.php.

Picked from #9519

@pabzm pabzm requested a review from alecpl February 19, 2025 16:13
This probably wasn't implemented previously because HTML-parts usually
didn't run through get.php.
@pabzm pabzm force-pushed the fix-washing-html-from-rcmail_attachment_handler branch from 675a2fb to 899344c Compare February 20, 2025 09:02
@pabzm pabzm requested a review from alecpl February 20, 2025 09:36
$is_safe = $this->is_safe();

return rcmail_action_mail_index::wash_html($body, ['safe' => $is_safe, 'inline_html' => false]);
if (isset($this->part->replaces)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be isset($this->part) or could be $this->part instanceof rcube_message_part, I suppose.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? isset() also checks the "intermediate" object and returns false without a warning, if that isn't set.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm.. you're right, it's just not what I'm used to do. If I was about to write this code, it would be: $replaces = $this->part ? $this->part->replaces : [];, but I will not insist.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to keep it as it is, since the current way also protects against replaces being null (for whatever reason).

@alecpl alecpl merged commit 99236f3 into master Mar 16, 2025
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants