Navigation Menu

Skip to content
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

Merged
merged 5 commits into from Oct 5, 2020
Merged

Fix FrameContextifier #1104

merged 5 commits into from Oct 5, 2020

Conversation

IonBazan
Copy link
Contributor

This PR fixes #1103 when $file is same as absolute path.

Copy link
Collaborator

@ste93cry ste93cry left a 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

src/Frame.php Outdated Show resolved Hide resolved
@IonBazan
Copy link
Contributor Author

@ste93cry Thanks for the review. I've applied the changes. Not sure if I should pass $file as $absoluteFilePath here too:

array_unshift($frames, new Frame(null, $file, $line));

@ste93cry
Copy link
Collaborator

ste93cry commented Oct 1, 2020

Not sure if I should pass $file as $absoluteFilePath here too

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 🤔

@IonBazan
Copy link
Contributor Author

IonBazan commented Oct 2, 2020

@ste93cry You're right. I've squashed my commits so only the required changes are there. Added necessary assertions to StacktraceBuilderTest to make sure it won't happen again.

Copy link
Collaborator

@ste93cry ste93cry left a 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?

@ste93cry
Copy link
Collaborator

ste93cry commented Oct 4, 2020

@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?

@IonBazan
Copy link
Contributor Author

IonBazan commented Oct 5, 2020

@ste93cry Added the changelog entry - feel free to modify it before release if it does not match the standards.

@ste93cry ste93cry merged commit e3d9a02 into getsentry:master Oct 5, 2020
@IonBazan IonBazan deleted the bugfix/absolute-file-path branch October 6, 2020 02:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[3.0] Frame context is not populated
2 participants