-
-
Notifications
You must be signed in to change notification settings - Fork 452
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
Added passing data from context in monolog breadcrumb handler #683
Conversation
This is a very good catch! Can you add tests for this change? |
Thanks, glad you like it. I've been working with the breadcrumbs and it's a very powerful feature. Does this test seem good? I tested on my computer and made it similar to the other one with the added context assertion. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anything holding back this PR from merging? It would make my life so much easier if this was merged and tagged 🙏
Nope, it's good, let's merge! |
@Jean85 For my info, what is the release process for new tagged releases? Then I know if I can wait or need to use my own fork on production. |
I suggest you to use this fork, since it's the only difference from the last tagged version and I can't guarantee when the next tag will be done. |
I see 18 changes? 1.9.2...master |
Oh sorry, I was mixing up with the Symfony integration that I just released. In this case I think that we can ask @stayallive that did the last 2 or 3 tags. |
I think we can definitely whip up another release. Let me set that up today. |
Thanks guys !! 💪 |
This simple change is very useful for breadcrumbs when using monolog. My app uses Monolog via Laravel and I think others will want this behavior too.
Before:
After: