-
Notifications
You must be signed in to change notification settings - Fork 39
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
Adding compatibility with Lumen without sessions #90
Adding compatibility with Lumen without sessions #90
Conversation
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.
I think this change is important for Laravel apps without sessions, like Lumen or Laravel Zero (https://laravel-zero.com).
But I think there is a problem in the condition.
@@ -59,7 +59,8 @@ protected function addContext(array $context = []) | |||
} | |||
|
|||
// Add session data. | |||
if ($session = $this->app->session->all()) { | |||
$session = isset($this->app) ? $this->app->session->all() : null; |
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.
Should'nt you check for $this->app->has('session')
here instead of isset($this->app)
?
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.
No, you shouldn't because if $this->app
is null
you cannot call the method has
, and that's why it's failing now and it's better to check first with an isset()
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.
In my use cases it was only failing because session
does not exist on $this->app
, not because $this->app
does not exist at all. In which situation $this->app
is not defined?
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.
I've tested again and $this->app
does exists in Lumen, that is the ->session
part that is failing.
Changing the if
condition to $this->app->has('session') && $session = $this->app->session->all()
solves the problem.
this fix was originally proposed on issue rollbar#83 and PR rollbar#84 and rollbar#90 without further interaction/response from the original contributor.
Hey @victormln @tristanjahier |
No description provided.