Rollbar\DataBuilder->getPerson() does not include context added by Rollbar\Laravel\MonologHandler->addContext()

[ArturMoczulski]

As stated in https://github.com/rollbar/rollbar-php-laravel/pull/75#issuecomment-445001279

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.

[TylerVigario]

Had some time to do some more debugging today.

I found that this line was stopping Rollbar-Laravel from adding Person context. if ($session = $this->app->session->all()) {

I think the issue stems from Rollbar-Laravel's assumption that a session's presence is an indication of a User being logged in or not.

[TylerVigario]

I've created a PR for this – #78

[TylerVigario]

I just received the same issue even with the above #78.

Would it be a good idea to allow Rollbar\Payload\Person to accept objects? It appears that when the context is not given by Rollbar-Laravel that Rollbar itself calls the 'person_fn'. Is there a specific reason as to why Rollbar-Laravel is supplying the Person context?

[ArturMoczulski]

Merged #78

Yes, rollbar-php will call person_fn if it has been provided and the person data does not exist yet. This happens here: https://github.com/rollbar/rollbar-php/blob/master/src/DataBuilder.php#L868-L875 This is very much intended behavior.

rollbar-php-laravel converts Laravel's user data format to that accepted by rollbar-php and configures the logger with it so it is sent appropriately to the Rollbar service.

Accepting objects into Rollbar\Payload\Person imho is not a good idea, as we need to decide on the interface of the data being passed there. An array seems the most flexible. If we start allowing more types of data there, we need to also adhere to different interfaces etc. and that's not the responsibility of Rollbar to understand those. That's why person_fn exists in the first place. So you can convert your data object model into an array understandable by rollbar-php. This is what happens here in rollbar-php-laravel: https://github.com/rollbar/rollbar-php-laravel/blob/master/src/MonologHandler.php#L45-L53

Does it make sense?

[ArturMoczulski]

I'm closing the issue for now. Will reopen if needed

[ArturMoczulski]

Changes from the PR released in v4.0.3

[TylerVigario]

Does it make sense?

Yes, I understand interfaces.

Take a look at a snippet of the changes from #75 and you'll see I handled objects by checking individually that the object has the property we are looking for.

if (is_object($data)) {
    $person = call_user_func($config['person_fn']);
    if (isset($data->id)) {
        $person['id'] = $data->id;
        if (isset($data->username)) {
            $person['username'] = $data->username;
        }
        if (isset($data->email)) {
            $person['email'] = $data->email;
        }
    }
}

Anywho, if the issue comes up again I will investigate.

[ArturMoczulski]

Yes, those are great changes and thank you for your PR. I merged it into master and included in the latest release.

However, going deeper to rollbar-php and allowing objects into Person opens the door for the PHP SDK to take on too much responsibility: trying to figure out how to interpret the data models supplied by the user app. This is what person_fn is for and in the case of rollbar-php-laravel - the logic of this PR :)

[TylerVigario]

Yes, those are great changes and thank you for your PR. I merged it into master and included in the latest release.

However, going deeper to rollbar-php and allowing objects into Person opens the door for the PHP SDK to take on too much responsibility: trying to figure out how to interpret the data models supplied by the user app. This is what person_fn is for and in the case of rollbar-php-laravel - the logic of this PR :)

Understood. :smile: