corowne / lorekeeper

A dA ARPG masterlist framework
MIT License
113 stars 128 forks source link

Suggestion/Discuss: Standardize use of direct Auth::User #907

Open SpeedyD opened 7 months ago

SpeedyD commented 7 months ago

We should at some point consider to replace all (Auth::check() ? Auth::user() : null) with (Auth::user() ?? null).

Having Auth::check(), which sole purpose is to see if Auth:user() is not null, in the same line as checking Auth::user(), is not great. It would be better if we were to replace it by a singular check, which immediately gave the required result as well.

On another note, there is different kind of check, the if(Auth:check() && Auth:user()) variant- we may want to consider looking into a solution for those as well, though that is a different story entirely.

I'd like to open this issue for discussion.

itinerare commented 7 months ago

This is arguably one of those things that can be done whenever, since it's not a breaking change. It's also really not that dire a thing, but would be nice since it makes things a little tidier.

SpeedyD commented 2 weeks ago

Wanted to get back to this discussion, since we never spoke of the latter part: if(Auth:check() && Auth:user())..

But.. I've been thinking about it, and I can't really find a straight up way to replace it. If check() isn't there then user() will cause problems when there's no user..

So I'd like some thoughts on this. Should we just let this sit (which in my opinion isn't the smartest of ideas) or does someone have a good way to replace this check?

(Been looking into php booleans and I wonder if if(Auth:user() ?? false) wouldn't just do it.. since false is always considered false..?)

itinerare commented 2 weeks ago

Auth::user() ?? false might do it, but would need testing, naturally.

SpeedyD commented 2 weeks ago

It's one of the most common usages, I think literally the navbar has one of these, so it shouldn't be hard to test.

I'll see if I can do a simple test later today..