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

Fixed MonologHandler conflict while using rollbar agent #108

Closed
wants to merge 1 commit into from

Conversation

Lukasz93P
Copy link

@Lukasz93P Lukasz93P commented Mar 13, 2021

Description of the change

Resolver handler conflict while using rollbar agent handler. Bug which was describer here

Type of change

  • Bug fix (non-breaking change that fixes an issue)

Related issues

Bug #85

Checklists

Development

  • Lint rules pass locally
  • The code changed/added as part of this pull request has been covered with tests
  • All tests related to the changed code pass in development

Code review

  • This pull request has a descriptive title and information useful to a reviewer. There may be a screenshot or screencast attached
  • "Ready for review" label attached to the PR and reviewers mentioned in a comment
  • Changes have been reviewed by at least one other engineer
  • Issue from task tracker has a link to this pull request

@Lukasz93P
Copy link
Author

@bishopb @brianr @waltjones @jenssegers @ArturMoczulski can you please review and merge this change?
I know that this is maybe not ideal but I'm in a hurry I need working laravel rollbar agent for my production app ASAP.

@brianr
Copy link
Member

brianr commented Mar 13, 2021

Hi @Lukasz93P, we will aim to get this reviewed soon, but probably not until next week. The fastest path to resolve your production app issue is probably to install your branch directly, something like this: https://getcomposer.org/doc/05-repositories.md#loading-a-package-from-a-vcs-repository (note you'll probably need to rename the branch to start with dev-).

@Lukasz93P
Copy link
Author

@brianr I know that but I didnt want to have to do this.
Thanks for info when somebody will look at this 🙂

@Lukasz93P
Copy link
Author

Lukasz93P commented Mar 19, 2021

@brianr can someone review this pr ?

BTW I figured out that renaming branch is not necessary, now I have dev-dev ;) because dev prefix is not part of the branch name.

@bishopb bishopb self-assigned this Mar 20, 2021
@bishopb bishopb added Rank: 1 - Critical Tackle above all other actionable requests. Status: 1 - Discussion Stakeholders are discussing the scope and characteristics of the request. labels Mar 20, 2021
@bishopb
Copy link

bishopb commented Mar 21, 2021

Thanks for the contribution, @Lukasz93P. I've taken this in a slightly different direction. Would you mind trying Pull Request #109 in your environment to see if it works for you?

@Lukasz93P
Copy link
Author

@bishopb yes, I will try but I need at least few days for this.

@bishopb
Copy link

bishopb commented Mar 28, 2021

Thank you again for your contribution. I'm closing this one in favor of Pull Request #109. A new release will be cut soon.

@bishopb bishopb closed this Mar 28, 2021
@Lukasz93P
Copy link
Author

@bishopb unfortunately this solution also causes an error - Attempt to read property "session" on null (Most recent call first) - /app/vendor/rollbar/rollbar-laravel/src/MonologHandler.php line 62.
When I changed package version to my closed pull request in the same environment, then it works, so I'm still unable to use official version instead of my own fork.

@bxsx
Copy link

bxsx commented Nov 2, 2021

@bishopb unfortunately this solution also causes an error - Attempt to read property "session" on null (Most recent call first) - /app/vendor/rollbar/rollbar-laravel/src/MonologHandler.php line 62. When I changed package version to my closed pull request in the same environment, then it works, so I'm still unable to use official version instead of my own fork.

This seems to be related to #106 which I just merged. Would you mind giving it a try @Lukasz93P ?

@Lukasz93P
Copy link
Author

Hi @bxsx, any update here? Can my PR be merged or maybe you have better solution(mentioned above is not worki g) ?

@cyrusradfar
Copy link

Wasn't the issue resolved here #85 (comment)

@Lukasz93P
Copy link
Author

Lukasz93P commented Apr 11, 2022

No it jus introduced another issue which I described here #108 (comment)
Only working solution is my PR which has been ignored for long long time...

@danielmorell
Copy link
Collaborator

@Lukasz93P what version of rollbar/rollbar-laravel are you running?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Rank: 1 - Critical Tackle above all other actionable requests. Status: 1 - Discussion Stakeholders are discussing the scope and characteristics of the request.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants