Hey!

Thanks for your PR. You are targeting branch "6.2" but it seems your PR description refers to branch "6.3".
Could you update the PR description or change target branch? This helps core maintainers a lot.

Cheers!

Carsonbot

The proposal here makes the UserProvider responsible for implementing all the token validation. To me, this risks encouraging bad implementations because the UserProvider is currently responsible for none of the validation.

I'd rather have a specific interface to be implemented by the AccessTokenHandler to return a UserBadge directly instead of only the identifier (or something like that).
Note that it might not even be a special interface. We could have getUserIdentifierFrom(string $accessToken): string|UserBadge allowing to return a UserBadge directly for the advanced case.

Note that it might not even be a special interface. We could have getUserIdentifierFrom(string $accessToken): string|UserBadge allowing to return a UserBadge directly for the advanced case.

Looks sensible to me.

I've changed it to use the string|UserBadge return types instead.

Widening the return type of the interface will be a BC break for consumers of the interface though. So if we delay that to 6.3, we'll have to use a separate method

@stof I thought a string returntype in implementations of the interface would be covariant with string|UserBadge?

@Jeroeny yes, it's OK for implementations, but consumers have to take care of another type when calling the method - which would be a BC break.

@Jeroeny the BC break concerns consumers, not implementors. For implementors, the covariance rule indeed makes it BC. Btw, that's the reason why a child class is not allowed to widen a return type as it would break consumers.

And also the main consumer is in the core and will be updated, any decorator class is both an implementation and a consumer and will be impacted by the widened type.

Since some on-going implementations already need this, I'd be fine merging this on 6.2 as a feature-freeze adjustment.

Ah or course, for consumers. Allowing it in 6.2 would be cool.

Should private readonly UserProviderInterface $userProvider of AccessTokenAuthenticator become nullable? What UserProviderInterface would you inject when you are already retrieving the UserBadge from the AccessTokenHandlerInterface ?

None indeed, can you make the change in this PR?

@chalasr Added, I have 2 open questions about the consequences, see comments :).

Thank you @Jeroeny.

Thanks all 👍