Skip to content

Dont overwrite Event properties - #1148

Merged
ste93cry merged 10 commits into
getsentry:masterfrom
matthijsthoolen:patch-1
Jan 7, 2021
Merged

ste93cry merged 10 commits into
getsentry:masterfrom
matthijsthoolen:patch-1

Conversation

@matthijsthoolen

Copy link
Copy Markdown
Contributor

Do not overwrite Event properties if they are already configured. Merge tags from options and event.

See bug #1146 the bug was only for tags, but also SdkIdentifier, sdkversion, serverName, release, and Environment are always overwritten.

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

Can you please add the CHANGELOG and (hopefully they are not missing) fix the tests?

Comment thread src/Client.php Outdated
Comment thread src/Client.php Outdated
Comment thread src/Client.php Outdated
Comment thread src/Client.php Outdated
@ste93cry ste93cry added this to the 3.1 milestone Dec 2, 2020
Comment thread src/Client.php Outdated
Comment thread CHANGELOG.md Outdated
Comment thread tests/ClientTest.php Outdated
@ste93cry

ste93cry commented Jan 5, 2021

Copy link
Copy Markdown
Contributor

@matthijsthoolen are you still open to making the necessary changes to merge this PR? Otherwise, can I continue your work?

@matthijsthoolen

Copy link
Copy Markdown
Contributor Author

@ste93cry I'm really sorry, having a busy period and couldn't find the time to fix this issue. I am still willing to fix it, but if you have the time to continue with this merge request earlier than next week. Feel free to do so!

@ste93cry
ste93cry force-pushed the patch-1 branch 2 times, most recently from 6b42a0a to 72976b2 Compare January 7, 2021 01:03
@ste93cry
ste93cry requested a review from stayallive January 7, 2021 01:04
@ste93cry
ste93cry merged commit 7b07a7e into getsentry:master Jan 7, 2021
soundsgoodsofar pushed a commit to soundsgoodsofar/sentry-php that referenced this pull request Jan 8, 2021
…ing `captureEvent()` (getsentry#1148)

Co-authored-by: Stefano Arlandini <sarlandini@alice.it>
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.

5 participants