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
Fix FrameContextifier #1104
Fix FrameContextifier #1104
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.
Rather than making the abs_path
attribute required, which in the case of internal frames would report an incorrect value, I would rather change the frame builder to set its value regardless of whether the $file
is equal or not to its stripped value. Technically speaking if $file
is not an absolute path we would still report an incorrect value and the integration would try to read a non-existent file, but it should not happen if the frame is built from a real backtrace
@ste93cry Thanks for the review. I've applied the changes. Not sure if I should pass sentry-php/src/StacktraceBuilder.php Line 73 in 65030a1
|
Strictly speaking yes, but we should also strip the prefix from the path like we do for the rest of the frames. We should probably call the frame builder also for this "fake" frame 🤔 |
@ste93cry You're right. I've squashed my commits so only the required changes are there. Added necessary assertions to |
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.
Changes look great, thank you for both spotting and solving this bug 😎 Can you please add a CHANGELOG entry before I merge?
@IonBazan maybe it passed under the radar because I edited the comment after posting it, but would you be so kind to add an entry to the CHANGELOG? |
@ste93cry Added the changelog entry - feel free to modify it before release if it does not match the standards. |
This PR fixes #1103 when
$file
is same as absolute path.