[SecurityBundle] error helper added symfony/symfony#11147 #11324

i3or1s · Jul 5, 2014

Added helper that extracts last authentication error and username.

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	[ #11147 ]
License	MIT
Doc PR	symfony/symfony-docs#3996

weaverryan · Jul 5, 2014

Awesome! @i3or1s Can you create a pull request or an issue on the docs to reflect this change (which I'm happy to say will simplify the docs nicely)?

Thanks for the nice PR!

i3or1s · Jul 5, 2014

Created related issue in Docs symfony/symfony-docs#3996

stof · Jul 9, 2014

src/Symfony/Component/Security/Core/Util/AuthenticationErrorHelper.php

+
+    /**
+     * @param bool $clearSession
+     * @return string


this is wrong. It does not return a string but an AuthenticationException (or null)

Were you referring to new AuthenticationException($msg)?

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

weaverryan · Jul 10, 2014

Issues that stof reported have been fixed.

fabpot · Jul 10, 2014

src/Symfony/Component/Security/Core/Util/AuthenticationErrorHelper.php

+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\Security\Core\Util;


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

Since it is a AuthenticationErrorHelper should it go to Symfony\Component\Security\Http\Authentication?

i3or1s · Jul 17, 2014

Issues are fixed any news on the PR

fabpot · Jul 23, 2014

👍

i3or1s · Aug 11, 2014

Hi @weaverryan @stof @romainneutron could you check out the PR

xabbuh · Aug 11, 2014

src/Symfony/Component/Security/Http/Authentication/AuthenticationUtils.php

+
+    /**
+     * @param bool $clearSession
+     * @return null|AuthenticationException


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 the clearSession argument is used for, why I should set it to false 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 ;)

xabbuh · Aug 11, 2014

Shouldn't this also get a changelog entry?

stof · Aug 11, 2014

src/Symfony/Component/Security/Http/Authentication/AuthenticationUtils.php

+    {
+        $request = $this->getRequest();
+        $session = $request->getSession();
+        $msg = null;


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

$authenticationException what do you think about this one?

much better

Added helper that extracts last authentication error and username.

i3or1s · Aug 18, 2014

Issues have been fixed. This is my first contribution so i am eager to see how it all goes. I apologize if i am breaking some rules on this.
Could you point me to the correct direction (document) on what i should do next, if anything, so that PR is merged.

jakzal · Aug 18, 2014

@i3or1s I think you're done 👍

weaverryan · Aug 19, 2014

@i3or1s You're doing great :). You could add a new 2.6.0 CHANGELOG entry to this file: https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/SecurityBundle/CHANGELOG.md

But unless someone sees some other change, we're just waiting for merge (some people are on holiday right now).

Thanks!

i3or1s · Aug 20, 2014

@weaverryan Should i change Component CHANGELOG.md since class is added there or Bundle one because it is registered there or maybe both?

xabbuh · Aug 20, 2014

You also have to update the version of symfony/security in the bundle's composer.json file, don't you?

stof · Aug 20, 2014

@i3or1s the class is added in the component, so the changelog entry should be added in the component.

And indeed, @xabbuh is right about bumping the SecurityBundle requirement on the component for people installing the subtree repos

i3or1s · Aug 20, 2014

@stof @xabbuh Component is in 2.5 is this going to be 2.6 so i can change dependency to 2.6.
Current one is "symfony/security": "~2.4"

Added changelog entry

fabpot · Aug 28, 2014

@i3or1s Yes, you should change the dep to symfony/security": "~2.6

fabpot · Aug 31, 2014

Thank you @i3or1s.

fabpot · Aug 31, 2014

I've fixed the dep in c51f3f3

…ls (xabbuh) This PR was merged into the 2.6 branch. Discussion ---------- [Cookbook][Security] document the new AuthenticationUtils | Q | A | ------------- | --- | Doc fix? | no | New docs? | yes (symfony/symfony#11324) | Applies to | 2.6+ | Fixed tickets | #3996 Commits ------- dd7c1dd document the new AuthenticationUtils

stof reviewed Jul 9, 2014
View reviewed changes

fabpot reviewed Jul 10, 2014
View reviewed changes

i3or1s mentioned this pull request Jul 11, 2014

Update security for new login form helper symfony/symfony-docs#3996

Closed

romainneutron added the SecurityBundle label Jul 12, 2014

xabbuh reviewed Aug 11, 2014
View reviewed changes

stof reviewed Aug 11, 2014
View reviewed changes

[SecurityBundle] error helper added symfony#11147

44f700d

Added helper that extracts last authentication error and username.

[SecurityBundle] error helper added symfony#11147

693cc2f

Added changelog entry

fabpot closed this in 800b7b2 Aug 31, 2014

stof mentioned this pull request Sep 26, 2014

Updated documentation regarding the SecurityContext split symfony/symfony-docs#4188

Closed

javiereguiluz mentioned this pull request Oct 21, 2014

[DX] Proposal: Abstraction layer for functionnal testing #12274

Closed

xabbuh mentioned this pull request Dec 31, 2014

[Cookbook][Security] document the new AuthenticationUtils symfony/symfony-docs#4723

Merged

Search code, repositories, users, issues, pull requests...

Uh oh!

[SecurityBundle] error helper added symfony/symfony#11147 #11324

[SecurityBundle] error helper added symfony/symfony#11147 #11324

Uh oh!

Conversation

i3or1s commented Jul 5, 2014

Uh oh!

weaverryan commented Jul 5, 2014

Uh oh!

i3or1s commented Jul 5, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weaverryan commented Jul 10, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

i3or1s commented Jul 17, 2014

Uh oh!

fabpot commented Jul 23, 2014

Uh oh!

i3or1s commented Aug 11, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xabbuh commented Aug 11, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

i3or1s commented Aug 18, 2014

Uh oh!

jakzal commented Aug 18, 2014

Uh oh!

weaverryan commented Aug 19, 2014

Uh oh!

i3or1s commented Aug 20, 2014

Uh oh!

xabbuh commented Aug 20, 2014

Uh oh!

stof commented Aug 20, 2014

Uh oh!

i3or1s commented Aug 20, 2014

Uh oh!

fabpot commented Aug 28, 2014

Uh oh!

fabpot commented Aug 31, 2014

Uh oh!

fabpot commented Aug 31, 2014

Uh oh!

Uh oh!