this is wrong. It does not return a string but anAuthenticationException (ornull)

Were you referring tonew AuthenticationException($msg)?

the existing code does not return a string. The request attribute stored inSecurityContextInterface::AUTHENTICATION_ERROR is an AuthenticationException instance, not a string

This should be part ofSymfony\Component\Security\Http instead. Core does not depend on HttpFoundation currently.

Since it is aAuthenticationErrorHelper should it go toSymfony\Component\Security\Http\Authentication?

Sure

I think it's more developer friendly to provide some more documentation comments here.

Any suggestion on what it should be?
"Retrieves last Authentication Error, if $clearSession is true it will also remove the error form session."?

"Retrieves last Authentication Error, if $clearSession is true it will also remove the error form session." - this can be read from the code and I don't think we need duplicating it in a comment.

@xabbuh could you explain what kind of a documentation you mean here?

I would write something like this:

/** * Returns the last authentication error. * * Authentication errors are read from the current request or fom the session. Keeping * errors in the session may cause issues when calling the method several times cause * you don't know if the errors has been read before. Therefore, after retrieving the error, * it is erased by default. If you want to keep the error in the session, you'll have to pass * false to this method. */

I know that the method name is quite self-explanatory, but when I look at the generated API documentation I find it useful if there is some more explanation.

I'm against documenting WHAT the code is doing. It's redundant, since you can read what the code is doing from the code itself... Also, this kind of comments often lie, since people tend to forget to update them.

We should rather explain WHY we're doing something, or document potential pitfalls. Anything that's not self-evident from reading the code.

Sure, you can have a look at the code. I just think that this isn't a nice experience from the user's (the developer that uses Symfony) point of view. I feel that the best experience is when one just reads the API documentation and you do know how to use a particular class and which implications its usage has.

Given this method, it's useful to know what theclearSession argument is used for, why I should set it tofalse and what it'll imply if I do that (so maybe my suggestion doesn't fit right how it should be done).

Though I'm not sure if this pull request is the right place to discuss this topic since it's somehow is related to the API documentation as a whole, isn't it?

You wouldn't be able to use Symfony's API docs this way, as it hardly contains comments of that kind. You're right it starts going out of the scope for this PR ;)

you should rename this local variable. It is not a message

$authenticationException what do you think about this one?

much better