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

Add user detector #7

Merged
merged 4 commits into from
Feb 18, 2023
Merged

Add user detector #7

merged 4 commits into from
Feb 18, 2023

Conversation

zepfietje
Copy link
Contributor

This PR adds a detector to get the locale from the currently authenticated user, if any.

@zepfietje zepfietje marked this pull request as draft February 17, 2023 14:28
@zepfietje zepfietje marked this pull request as ready for review February 17, 2023 14:29
@ivanvermeyen
Copy link
Contributor

Thanks for your contribution!

Before merging I'd like to make sure we support this Laravel feature:
laravel/framework#44283

Perhaps the attribute can be null by default?

@zepfietje
Copy link
Contributor Author

Hmm I'm not sure how we should handle that.
Could you explain your suggestion?

@ivanvermeyen
Copy link
Contributor

I was thinking if this is null in the config by default:

'user-attribute' => null,

Then we can check for it to avoid the MissingAttributeException:

 if ($attribute === null || $user === null) {
    return null;
}

@zepfietje
Copy link
Contributor Author

zepfietje commented Feb 18, 2023

Isn't it clearer then to not include the detector in the default array?
Though I was really hoping to make this a default out of the box solution.

@ivanvermeyen
Copy link
Contributor

ivanvermeyen commented Feb 18, 2023

Yeah, I understand.

Another option is perhaps to check if the attribute exists?
Then it can be default and yet not mandatory?

@ivanvermeyen
Copy link
Contributor

Unfortunately Laravel has no hasAttribute() method.
But I think we can do something like this:

if ( ! in_array($attribute, $user->getAttributes())) {
    return null;
}

@zepfietje
Copy link
Contributor Author

Yeah that should work.
Doesn't it have any performance implications though, for example calling all casts?

@zepfietje
Copy link
Contributor Author

zepfietje commented Feb 18, 2023

@ivanvermeyen, we might use $user->getAttributeValue() to prevent the MissingAttributeException?

Just tested, it should work.

@zepfietje
Copy link
Contributor Author

This PR should be ready now, @ivanvermeyen.

@ivanvermeyen
Copy link
Contributor

getAttributeValue() seems to defer to getAttributes().

https://github.com/laravel/framework/blob/15f419f751474262f3ced85ea4ccce18cbcdae8e/src/Illuminate/Database/Eloquent/Concerns/HasAttributes.php#L481-L495

I'm not sure how big of an impact this has on performance, like you stated.
There doesn't seem to be an easy way to check the $attributes property without triggering the exception.

@zepfietje
Copy link
Contributor Author

zepfietje commented Feb 18, 2023

Casting only seems to happen for the specific attribute you're accessing.
So performance-wise I don't think this is an issue really.

Edit: actually casting does happen for all attributes I think.

@zepfietje
Copy link
Contributor Author

Okay, this isn't any less performant than magically accessing properties like $user->locale, as it all uses the same getAttribute() and thus getAttributeValue() methods under the hood.

@ivanvermeyen
Copy link
Contributor

Great, thanks for putting this together 👍
I'll merge and tag a release.

@ivanvermeyen ivanvermeyen merged commit c2da093 into codezero-be:master Feb 18, 2023
@zepfietje
Copy link
Contributor Author

Thanks a lot for being so swift and maintaining these awesome packages!

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