This would break every single null !== check though. I suggest to update the docs rather than the actual return type. Either you have a username or you don't, but you don't have a username or an empty username imo.

@iltar , what if it always returns an nonempty string or null? This shouldn't break BC
see commit above

The case that the session is not set, is one that in theory should not happen frequently

@iltar , I agree. But that's how it was done before, the null === $session check existed before my pull request. The only difference is that now the method never returns an empty string and we can safely use null !== AuthenticationUtils::getLastUsername() which will cover all cases.

My point is to make returning type consistent and more DX-friendly.

Not sure it will be accepted for 2.8, but +1 for this API :)

edit; on 2nd though Session::get defaults to null.. so we already should expect this.

Given that username should never be empty, I would prefer getLastUsername to always return a string and never null.

In any case, that would be for 3.4.

While username may not be empty, the "last" one might not exist (yet). Hence it's a nullable imo.

Right, if there is no last username, it's an empty string.

What about the case where the input type was named wrongly (e.g. username instead of _username)? Doesn't it make sense to return null then (because there was actually no submitted username and not an empty username)?

Let's finish this PR?
It doesn't require much effort but needs a final decision on the nullability.

👍 as well for always string (means session default changes)

I agree with @ro0NL - return a string always.

Same for me 👍 for a string return.

Rebased onto 3.4 and now it always returns a string.

But I think we should merge this into 4.0 because as @iltar said, we are breaking all the null === checks.

Could this be merged or smth else needs to be done?

Perhaps add a changelog entry?

Rebased, squashed, added a changelog entry, changed base to master, ready to merge :)

I think we should document this change in the upgrade file for 4.1 too.

Ready!

Thank you @vudaltsov.