search

rollbar / rollbar-php-laravel

Rollbar error monitoring integration for Laravel projects
https://docs.rollbar.com/docs/laravel
140 stars 39 forks source link

Properly handle Laravel model for "person_fn". #75

Closed TylerVigario closed 5 years ago

TylerVigario commented 5 years ago

Fixes – #73

jessewgibbs commented 5 years ago

@meeklogic thanks for the PR.

@ArturMoczulski can you review?

TylerVigario commented 5 years ago

I am still getting an error in my API controllers (haven't fully debugged it yet).

I think Rollbar\\DataBuilder->getPerson() runs prior to receiving context from this package. Thus it calls 'person_fn' itself and gets an object (App\User).

ArturMoczulski commented 5 years ago

@MeekLogic what error are you getting in your API controllers?

TylerVigario commented 5 years ago

@MeekLogic what error are you getting in your API controllers?

73

Argument 4 passed to Rollbar\Payload\Person::__construct() must be of the type array or null, object given, called in C:\inetpub\wwwroot\oim-laravel\vendor\rollbar\rollbar\src\DataBuilder.php on line 894

The above PR seemingly fixed the issue but for some reason, I have noticed that it still occurs on a specific API route. It was an image upload route and the error that I was actually having was with NTFS permissions.

I have fixed the NTFS issue and have not noticed an issue since.

This may or may not be caused by an "API" route or by a low-level error that occurs in Laravel prior to hooking Rollbar-Laravel.

I did find that the data returned by Rollbar\\DataBuilder->getPerson() did not include context that should have been given prior from Rollbar\Laravel\MonologHandler->addContext().

However, I do not currently have enough experience with Laravel to make a proper assumption as to the root cause.

ArturMoczulski commented 5 years ago

LGTM. Merging into master and reproduced the change can be reproduced in release line v3.*.

I created an additional issues for the context problem here: https://github.com/rollbar/rollbar-php-laravel/issues/76