Skip to content
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

Only capture attributes when fetching user data #244

Merged
merged 1 commit into from
Aug 15, 2017

Conversation

kattrali
Copy link
Contributor

When a user is connected to many other models, calling toArray can be intensive, and the data gathered may not be much more useful from a debugging perspective. This change reduces the overhead of fetching the user.

@@ -270,7 +270,7 @@ protected function setupCallbacks(Client $client, Container $app, array $config)
$client->registerCallback(new CustomUser(function () use ($app) {
if ($user = $app->auth->user()) {
if (is_callable([$user, 'toArray'])) {
return $user->toArray();
return $user->attributesToArray();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should check if attributesToArray is callable then, instead of toArray.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True that. Easy one to miss even when testing it. 😅

@kattrali kattrali force-pushed the kattrali/avoid-fetching-all-user-relationships branch from cb5cac7 to b885efe Compare August 11, 2017 17:29
@@ -269,8 +269,8 @@ protected function setupCallbacks(Client $client, Container $app, array $config)
if (!isset($config['user']) || $config['user']) {
$client->registerCallback(new CustomUser(function () use ($app) {
if ($user = $app->auth->user()) {
if (is_callable([$user, 'toArray'])) {
return $user->toArray();
if (is_callable([$user, 'attributesToArray'])) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should actually be:

if (method_exists($user, 'attributesToArray') && is_callable([$user, 'attributesToArray'])) {
    return $user->attributesToArray();
}

This was a bug in the original code too.


The reason here is that eloquent models have a __call method, so is_callable returns true even for methods that aren't really callable, because it just throws a runtime exception if not callable. We can't just have a method_exists check here either, because that also returns true for private methods, and we are only interested in ones we can actually call.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I see. Only models then? Would that be the case for the logger too? That's the only other place we're using is_callable.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically yeh, it could be changed there. I don't think any of the out of the box loggers implement __call, but that check could be added there, yeh.

When a user is connected to many other models, calling toArray can be
intensive, and the data gathered may not be much more useful from a
debugging perspective. This change reduces the overhead of fetching
the user.
@kattrali kattrali force-pushed the kattrali/avoid-fetching-all-user-relationships branch from b885efe to 4e74ecf Compare August 14, 2017 19:59
@kattrali kattrali merged commit accc94f into master Aug 15, 2017
@kattrali kattrali deleted the kattrali/avoid-fetching-all-user-relationships branch August 15, 2017 20:35
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.

2 participants