nullcheck for $this->email

[StrykeSlammerII]

Without the nullcheck, a permissions test against !is_master(self.id) results in the error trim(): Argument #1 ($string) must be of type string, null given here in getAvatarAttribute().

With the nullcheck, there is no error and userfrosting.log gives

[2024-01-31T18:47:36.554796-05:00] auth.DEBUG: Evaluating access condition '!is_master(self.id)' with parameters: {
    "user": {
        "UserFrosting\\Sprinkle\\Account\\Database\\Models\\User": {
            "full_name": " ",
            "avatar": "https://www.gravatar.com/avatar/?d=mm"
        }
    },
    "self": {
[etc]

While $this->email appears elsewhere to be expected as type string rather than ?string, the auth.DEBUG output demonstrates that it is somehow fed a blank $user from somewhere.

This PR is a bandaid fix. Searching for the source of the blank $user and resolving that would be a better fix.

[lcharette]

I was able to reproduce locally by passing an empty user to AccessConditionEvaluator::evaluate directly, however I'm not sure how you encountered this bug in normal code?

The only way a user doesn't have an email is if, well, he doesn't have one defined. And email is (Should?) be mandatory in the UI... Other way is for the "not logged in" user to be used, which shouldn't happen either.

Actually, empty string is fine... email needs to be null, which should only happen when User is not saved:

$user = new User();
$user->toArray(); //<-- Exception

Can you share more how to replicate this bug?

[StrykeSlammerII]

I ran into it when accidentally injecting an empty user.
(chat reference: https://chat.userfrosting.com/channel/support?msg=NZvHNcLrHemfW3zNW)

That may not be normal code 😁 but hitting an error in getAvatarAttribute() made tracing the injection failure more difficult.
I don't have a copy of the error handy, but IIRC Whoops error handler fails if getAvatarAttribute() fails.
$this->logger->debug($user) also fails--again, it just makes debugging more difficult.

If we want getAvatarAttribute() to throw an error in the unexpected case where $user->email is null, I'd like to add a comment instead regarding injecting an empty user.
Let me know if that's the preferred solution and I'll change this PR.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (4715f7a) to head (ed8568e).
Report is 4 commits behind head on 5.0.

Additional details and impacted files

@@             Coverage Diff             @@
##                 5.0       #15   +/-   ##
===========================================
  Coverage     100.00%   100.00%           
+ Complexity       552       551    -1     
===========================================
  Files             93        93           
  Lines           2104      2108    +4     
===========================================
+ Hits            2104      2108    +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[lcharette]

Alright, just to be sure it wasn't another underlying issue.

I added a test and simplify the syntax, should be good to go.