-
-
Notifications
You must be signed in to change notification settings - Fork 463
feat: add attachment support #1925
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
base: 5.x
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Single suggestion but nothing major. Looks good to me!
switch ($event->getType()) { | ||
case EventType::event(): | ||
$items[] = EventItem::toEnvelopeItem($event); | ||
$items[] = AttachmentItem::toEnvelopeItem($event); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For clarity I think I would go for this (also in the transaction) instead and move the loop here. Yes it's a little more duplicated code but I think the clarity is a little better to also keep all the toEnvelopeItem
methods returning a single item instead of multiple or none.
$items[] = AttachmentItem::toEnvelopeItem($event); | |
foreach ($event->getAttachments() as $attachment) { | |
$items[] = AttachmentItem::toEnvelopeItem($attachment); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, very good point!
public function getData(): ?string | ||
{ | ||
return @file_get_contents($this->path) ?: null; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Null Handling Bug in File Operations
The ?: null
operator in getSize()
and getData()
incorrectly handles empty files. It converts filesize()
's 0
and file_get_contents()
's empty string ''
into null
. This causes empty files to be treated as unreadable, conflicting with tests expecting 0
for size and ''
for data.
} | ||
|
||
return implode("\n", $result); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Null Attachments Cause Malformed Envelope Data
The toEnvelopeItem
method includes null
results from toAttachmentItem
when attachment data is missing. These null
values are then converted to empty strings by implode("\n", $result)
, creating extra newlines and malformed envelope data that can cause parsing issues.
Introducing attachment support for PHP!
Attachments can be set on Scopes and will be added to Errors and Transaction events when sending them to sentry.
The PR currently does not enforce limits and reads the entire attachment content in memory before sending it. There is room for improvement such as respecting attachment limits as well as streaming the envelope to reduce the memory impact.