Skip to content

Properly handle Laravel model for "person_fn". #75

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

Merged
merged 2 commits into from
Dec 17, 2018
Merged

Properly handle Laravel model for "person_fn". #75

merged 2 commits into from
Dec 17, 2018

Conversation

TylerVigario
Copy link

@TylerVigario TylerVigario commented Dec 3, 2018

Fixes – #73

@TylerVigario TylerVigario mentioned this pull request Dec 3, 2018
@jessewgibbs
Copy link

@meeklogic thanks for the PR.

@ArturMoczulski can you review?

@TylerVigario
Copy link
Author

TylerVigario commented Dec 3, 2018

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
Copy link

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

@TylerVigario
Copy link
Author

TylerVigario commented Dec 6, 2018

@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
Copy link

ArturMoczulski commented Dec 17, 2018

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: #76

@ArturMoczulski ArturMoczulski merged commit ba561c3 into rollbar:master Dec 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants