Use rollbar v1.0.1 #85
Conversation
|
So how to cooperate? :) |
|
Hi @iwex, good to hear from you. I would suggest that we first decide which of the PRs we should proceed with. After that we can write tests to get a better coverage of what we have. I am emotionless on which PR we proceed, both look promising. Maybe the guys with write access to the repo can help with the decision. And I think we are not in a hurry. :) What do you think @jenssegers? |
|
Hey folks - there's also a similar effort that @ArturMoczulski (main developer on rollbar-php 1.0) did for the Laravel integration. It's over at https://github.com/rollbar/rollbar-php-laravel and is actually a fork of this library (jenssegers/laravel-rollbar). @ArturMoczulski is that ready for general consumption? |
|
@brianr I don't think it's ready https://github.com/rollbar/rollbar-php-laravel/blob/master/src/RollbarLogHandler.php#L78 :) |
src/RollbarLogHandler.php
Outdated
| @@ -73,55 +74,11 @@ public function log($level, $message, array $context = []) | |||
| return; | |||
| } | |||
|
|
|||
| $context = $this->addContext($context); | |||
There was a problem hiding this comment.
Removing addContext removes much of the usefulness of this package for providing Laravel-specific debug information.
What's your reasoning behind removing this valuable part of this package?
There was a problem hiding this comment.
I removed it because the function tried to access class variables which either do not exist any more or are not publicly accessible.
There was a problem hiding this comment.
There was a problem hiding this comment.
@olafnorge we should add person info, it's possible with $logger->configure() https://github.com/jenssegers/laravel-rollbar/pull/84/files#diff-e31215fa03abb6aa2a7a094e2286af78R134
There was a problem hiding this comment.
Ahh, but I see how it could work because @ArturMoczulski master...rollbar:master#diff-e31215fa03abb6aa2a7a094e2286af78L112 knows how to do it.
There was a problem hiding this comment.
I would take the changes from @ArturMoczulski but it doesn't feel right for me as long as he is not agreeing with it.
There was a problem hiding this comment.
In the spirit of Open Source, and how Rollbar is making many other packages available under MIT license, I'm guessing @ArturMoczulski is open to it ... but his is a fork from Jens' package and neither Jens nor Artur have specified a license.
There was a problem hiding this comment.
https://github.com/rollbar/rollbar-php-laravel/blob/master/composer.json#L6
rollbar/rollbar-php-laravel is licensed like other Rollbar packages under MIT and was developed as part of the core Rollbar.com development effort, so you're free to use it like any other package
|
It feels for me a little bit confusing and 'chaotic' to have now at least 3 version for the same thing. |
- bumped rollbar version to v1.0.1 - adapted `RollbarLogHandler` and `RollbarServiceProvider` to reflect rollbar changes - adapted tests to be green again - adapted changes of @ArturMoczulski from https://github.com/rollbar/rollbar-php-laravel
|
https://github.com/rollbar/rollbar-php-laravel is the official Laravel support package supported by Rollbar. It's a fork of jenssegers/laravel-rollbar, but that is the repo we will be focusing on moving forward. Yes, it is ready for general consumption. It's not in packagist repos yet, but README.md includes instructions how to |
|
In regards to ::addContext(), rollbar-php-laravel supports it: https://github.com/rollbar/rollbar-php-laravel/blob/master/src/RollbarLogHandler.php#L76 Also, adding Laravel session information is done automatically out of the box: https://github.com/rollbar/rollbar-php-laravel/blob/master/src/RollbarLogHandler.php#L86-L113 |
|
So then I don't think it makes sense to proceed here. But it was a good chance to learn about how the things work together. I will close this PR in favour of Arturs. Thank you guys for reviewing and making suggestions. |
|
@ArturMoczulski your README at https://github.com/rollbar/rollbar-php/blob/master/README.md still tells people to use the jenssegers package, instead of your own rollbar-php-laravel package. |
|
The Rollbar docs also still point to this repo for Laravel integration. https://rollbar.com/docs/notifier/rollbar-php/ |
|
@olafnorge So this repository will no longer update its Rollbar dependency? Will this repository be abandoned in favour of https://github.com/rollbar/rollbar-php-laravel, or is this still unclear? If yes, this might be something to add to the readme. |
|
@jarnovanleeuwen I can't tell because I am not the maintainer of this repo I just opened a PR and closed it. |
|
Rollbar officially announced/released its Laravel package (https://rollbar.com/blog/automated-laravel-error-reporting/). Their repo is ahead of this one and I think people should abandon this repo in favour of the official repo that is also relying on the latest |
RollbarLogHandlerandRollbarServiceProviderto reflect rollbar changes