lib/bugsnag/middleware/rack_request: early load session for Rails 4 #149
Conversation
Fixes #144 (Rails 4 sessions appear in custom tab) Context: rails/rails#10813
kyrylo
force-pushed
the
144-fix-sessions
branch
from
30654b3
to
f72d846
Compare
|
Why have you removed the comments? |
|
Because the code is self-descriptive. P.S. It's not ready to merge, because example-apps fail. |
|
Thats a code style thing and your personal opinion. Add them back in please. |
|
Sure, no problem! Added them back. |
|
Nice! Exactly the kind of bug that the example apps are good at finding. |
| # Rails 4 | ||
| notification.add_tab(:session, session.to_hash) if session.loaded? | ||
| end | ||
| end |
There was a problem hiding this comment.
@kyrylo Why do we need to check if the session is loaded given that we already load it? What happens if you just call session.to_hash?
There was a problem hiding this comment.
I don't know that API very well, it's just to be sure that we don't pass gibberish.
There was a problem hiding this comment.
You can't just write code like this and assume it's going to work. Could you please do some reading and testing and work out what we actually need to do.
There was a problem hiding this comment.
If I remove if session.loaded?, then potentially, we can get a NoMethodError: undefined method to_hash'. I just realised that I should add session.respond_to?(:loaded?)` to avoid this.
Anyway, what I propose it to write it like this:
notification.add_tab(:session, session.to_hash) if session.respond_to?(:to_hash)I assume that since the time of the creation of that code the session object has always been a Hash here. So if it's not a Hash, then it's probably a Rails 4 session.
The worst that can happen is that the session tab won't be included at all.
|
Closed in favour of #152. |
Fixes #144 (#144)
Context: rails/rails#10813