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

Allow numeric value in tags. #858

Merged
merged 2 commits into from
Jul 30, 2019
Merged

Allow numeric value in tags. #858

merged 2 commits into from
Jul 30, 2019

Conversation

leo108
Copy link
Contributor

@leo108 leo108 commented Jul 25, 2019

It's a very common scene that we use a numeric value as a tag, e.g. user id.

Developers are not always aware that should pass a real string value to Sentry's TagsContext, and InvalidArgumentException thrown in Sentry may not be caught by framework's exception handler because Sentry is called in the exception handler in most cases, thus no error logs in framework nor reports in Sentry, which makes it hard to track error.

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.

I generally agree with this change, thanks for the PR! Can you add tests so we no longer regress on this issue?

src/Context/TagsContext.php Outdated Show resolved Hide resolved
@ste93cry ste93cry modified the milestones: 2.2, 2.1 Jul 25, 2019
@ste93cry ste93cry modified the milestones: 2.1, 2.2 Jul 25, 2019
@ste93cry ste93cry changed the base branch from master to develop July 25, 2019 13:01
@leo108
Copy link
Contributor Author

leo108 commented Jul 26, 2019

@Jean85 Test added.

@ste93cry Method renamed.

@ste93cry
Copy link
Collaborator

Still missing the documentation of the private function, apart from that it seems fine to me 👍

@leo108
Copy link
Contributor Author

leo108 commented Jul 26, 2019

@ste93cry Doc added, sorry for the poor English.

@leo108
Copy link
Contributor Author

leo108 commented Jul 30, 2019

Any chance to merge this PR?

@Jean85 Jean85 requested a review from ste93cry July 30, 2019 07:26
@ste93cry
Copy link
Collaborator

@leo108 I did some changes to your PR, please let me know if you're ok with them

@leo108
Copy link
Contributor Author

leo108 commented Jul 30, 2019

@ste93cry LGTM, Thanks.

Copy link
Collaborator

@stayallive stayallive left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks!

@ste93cry ste93cry merged commit 98a07e7 into getsentry:develop Jul 30, 2019
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.

4 participants