Fixed ErrorException: Attempt to read property "id" on null (rollbar #3541) #13549
Conversation
PR Summary
|
There was a problem hiding this comment.
This is indeed an odd error and is so sporadic it probably lines up with the demo resetting.
Help my before-lunch brain process this: Is it possible that $event->checkoutable is null but $event->checkedOutTo matches a user and ends up deleting their pending acceptances? I don't think so because the database wouldn't match on checkoutable_id = null right?
Approving assuming that is true 😅
|
@marcusmoore Yeah... looks kind of dangerous. But assuming a normal behaviour, 'normally' we don't store any of those as null, I suppose a bug can happen somehow... maybe is safer to check the properties of $event before get the acceptances? If no checkoutTo nor checkoutable, we don't even query the DB... Thanks for your review!! |
|
That's good thinking but I think its ok to leave as is. |
|
@snipe good day, what you think. I should check the $event before the query? or is safe to merge as is? |
There was a problem hiding this comment.
maybe is safer to check the properties of $event before get the acceptances? If no checkoutTo nor checkoutable, we don't even query the DB...
Reading this again and knowing we've had a couple quirky rollbars recently I feel like checking $event to make sure checkoutTo and checkoutable are both populated before firing the query, like you said, is safer and will lead to a smaller chance of a bug slipping through in the future.
|
Sure!!, I just add that condition. |
Description
For some reason, the notification for license seats checkins breaks, the rollbar only shows occurrences over the demo so I think it's related to that factor. But I added the null coalescing operator anyway, doesn't hurt to be cautious.
Fixes rollbar #3541
Type of change
How Has This Been Tested?
Test Configuration: