Skip to content

Fix FrameContextifier - #1104

Merged
ste93cry merged 5 commits into
getsentry:masterfrom
IonBazan:bugfix/absolute-file-path
Oct 5, 2020
Merged

ste93cry merged 5 commits into
getsentry:masterfrom
IonBazan:bugfix/absolute-file-path

Conversation

@IonBazan

Copy link
Copy Markdown
Contributor

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

@ste93cry ste93cry left a comment

Copy link
Copy Markdown
Contributor

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

Comment thread src/Frame.php Outdated
@ste93cry ste93cry added this to the 3.0 milestone Sep 30, 2020
@IonBazan

Copy link
Copy Markdown
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));

Comment thread src/FrameBuilder.php
@ste93cry

ste93cry commented Oct 1, 2020

Copy link
Copy Markdown
Contributor

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 🤔

Comment thread src/Integration/FrameContextifierIntegration.php
@IonBazan

IonBazan commented Oct 2, 2020

Copy link
Copy Markdown
Contributor Author

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

@ste93cry ste93cry left a comment •

Copy link
Copy Markdown
Contributor

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

ste93cry commented Oct 4, 2020

Copy link
Copy Markdown
Contributor

@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

IonBazan commented Oct 5, 2020

Copy link
Copy Markdown
Contributor Author

@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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[3.0] Frame context is not populated

2 participants