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 undefined index generated by missing function in class #823

Merged
merged 9 commits into from
Jul 31, 2019

Conversation

adnedelcu
Copy link
Contributor

Fixing issue #303 for version 2.x.

Copy link
Collaborator

@Jean85 Jean85 left a comment

Choose a reason for hiding this comment

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

Apart from a fix, LGTM 👍

composer.json Outdated Show resolved Hide resolved
src/Stacktrace.php Outdated Show resolved Hide resolved
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.

Thanks to @Nuranto in getsentry/sentry-laravel#244 we finally found a way to reproduce and validate the issue which, for knowledge, is present due to XDebug modifying the stacktrace when enabled. I've left a few comments on this PR and we would be more than happy to merge it as soon as everything has been fixed 🎉

tests/Fixtures/backtraces/anonymous_frame.json Outdated Show resolved Hide resolved
tests/Fixtures/backtraces/exception.json Outdated Show resolved Hide resolved
tests/StacktraceTest.php Outdated Show resolved Hide resolved
tests/Fixtures/frames/class.json Outdated Show resolved Hide resolved
tests/StacktraceTest.php Outdated Show resolved Hide resolved
tests/StacktraceTest.php Outdated Show resolved Hide resolved
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.

Just some minor changes left, the rest looks good to me!

src/Stacktrace.php Outdated Show resolved Hide resolved
tests/StacktraceTest.php Outdated Show resolved Hide resolved
tests/StacktraceTest.php Outdated Show resolved Hide resolved
tests/StacktraceTest.php Outdated Show resolved Hide resolved
tests/StacktraceTest.php Outdated Show resolved Hide resolved
@ste93cry
Copy link
Collaborator

I though we already merged this but I was wrong. @adnedelcu are you willing to finish the PR?

@adnedelcu
Copy link
Contributor Author

@ste93cry Yes, I am willing but I have been busy relocating. I will try to do the changes tomorrow.

@Jean85 Jean85 requested a review from ste93cry July 31, 2019 13:33
@ste93cry ste93cry merged commit 62ee726 into getsentry:master Jul 31, 2019
@ste93cry
Copy link
Collaborator

Thank you for the great patience and for your contribution of course 🎉

@adnedelcu
Copy link
Contributor Author

So sorry it took so long.

HazAT added a commit that referenced this pull request Aug 22, 2019
* release/2.1.2:
  meta: Update changelog
  meta: Changelog 2.1.2
  Skip integrations not bound to the current client and fetch their options from it (#861)
  Do not set the transaction attribute of the event when in CLI (#864)
  Added OXID eShop to the list of 3rd party integrations (#860)
  Fix error thrown when function name is missing in the stacktrace frame (#823)
  Fix sending of GZIP-compressed requests when the enable_compression option is on (#857)

# Conflicts:
#	CHANGELOG.md
#	tests/ClientBuilderTest.php
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 participants