What about keeping isAuthenticated + a throwing getUser?

(i tend to prefer a random UnauthenticatedTokenException, rather than a random null value)

(i tend to prefer a random UnauthenticatedTokenException, rather than a random null value)

We should then make thatUnauthenticatedTokenException part of the abstraction, and there could be specific implementation bugs (eg when isAuthenticated returns true while getUser throws).

I think I prefer making getUser nullable personally.

Also we come fromgetUser(): string|Stringable|UserInterface wherestring can meananon.. Going withUserInterface|null looks like a smaller gap

Thanks! I surely prefer a nullable user over reintroducingisAuthenticated()/throwing an exception.

Nullable return types are not optimal. However, in this case 99% of the apps I see already do something likeassert($user instanceof AppUser); or/** @var AppUser $user */, as theUserInterface contract is often not the useful contract for applications. This means that nullable isn't that bad as they already check for nullability.

Btw, I'm personally not sure if adding a newUnauthenticatedVoterInterface::votePublicAccess(array $attributes, mixed $subject) and removingNullToken is better than this one. But I don't have the time to try this out myself, so I'm ok with this PR if others agree.

when isAuthenticated returns true while getUser throws

it be a contract violation. I proposed it because of less deprecation hassle actually 😅 but yes, it's a try/catch shortcut sure.

going from a non-nulllable to nullable is a bigger gap IMHO.

However, in this case 99% of the apps I see already do something like assert($user instanceof AppUser);

hence my proposal.

and there could be specific implementation bugs
it's a try/catch shortcut sure

actually, if isAuthenticated() is public API specific tokens can do a more sophisticated check. Where isAuthenticated is leveraged in getUser().

(this can be achieved in both styles btw, but i still tend to prefer explicit isAuthenticated/UnauthenticatedTokenException somewhat :/)

even without isAuthenticated, a voter doing the try/catch is still 100% explicit.

A try/catch is not a nice API to me. nullability works just great. I tend to prefer APIs that don't open traps for implementation bugs with to-enforce correlations between their methods. isAuthenticated should not be used in getUser because they are different views of the same state - and not just correlated states.

votePublicAccess(array $attributes, mixed $subject)

that would require knowing about both interfaces to cover the domain, and that would require another interface forAccessDecisionManagerInterface::decide(). nullability packs everything together in a simple to discover API.

sorry, im not convinced (yet). Still i believe as a consumer i'd prefer

tend to prefer a random UnauthenticatedTokenException, rather than a random null value

But we'll be null checking in 99% of authentcated cases 👍

https://symfony.com/doc/current/security.html#retrieving-the-user-object null check is missing here btw.

I think if Security/Controller::getUser can convey authenticated yes/no thru nullability, im not sure the tokenstorage needs to.

Nevertheless a nullable Security/Controller::getUser is also pesky :)

From the tokenstorage layer i think TokenNotFoundException + new UnauthenticatedTokenException fits yes.

basically what im saying is, if you want a boolean value there's a access decision manager for it. But 99% the check is encapsulated IMHO.

symfony.com/doc/current/security.html#retrieving-the-user-object null check is missing here btw.

not needed because of the call to denyAccessUnlessGranted

I think if Security/Controller::getUser can convey authenticated yes/no thru nullability, im not sure the tokenstorage needs to.

Can you be more explicit about that please? Do you mean to always return a NullToken instead of throwing?

Nevertheless a nullable Security/Controller::getUser is also pesky :)

yep, we already have this nullable API elsewhere

From the tokenstorage layer i think TokenNotFoundException + new UnauthenticatedTokenException fits yes.

That's technically possible, I'm just not convinced it's better.
You tell about missing nullability checks, but with your proposal phpstorm will complain about missing try/catch :)

Another opinion or argument to help decide?

but with your proposal phpstorm will complain about missing try/catch

it's about runtime. Do we want to halt, or let a null value bubble unexpectedly.

btw im 100% aligned with

this might only be the expression of my ignorance of this subsystem :)

Here we put such wisdom on real tiles :) (https://en.wikipedia.org/wiki/Tegelspreuken)

could we infer unauthenticated state here already? (edit: right, this will trigger our voter + NullToken issue)

what if we go back to an UnauthenticatedToken for internal usage (mostly)?

Thank you Nicolas!