Move Identity Cookie code into separate functions and cleanup invalid identity cookies #11558
Conversation
ghost
commented
May 12, 2016
ghost
commented
| Q | A |
|---|---|
| Is bugfix? | no |
| New feature? | yes |
| Breaks BC? | no |
| Tests pass? | yes |
| Fixed issues | #8795 |
modified: framework/web/User.php modified: tests/framework/web/UserTest.php
|
I've this code in my application: https://ideone.com/oygl4K |
ghost
mentioned this pull request
| /** | ||
| * Removes the identity cookie. | ||
| * This method is used when [[enableAutoLogin]] is true. | ||
| * @param IdentityInterface $identity |
There was a problem hiding this comment.
Here it seems that this @param was forgotten since your method has no params.
There was a problem hiding this comment.
Thanks! I had originally intended to pass $identity in case it was needed, but someone told me that was a bad idea. So I took it out and neglected to update the comment.
SilverFire
added
status:to be verified
|
|
||
| /** | ||
| * Removes the identity cookie. | ||
| * This method is used when [[enableAutoLogin]] is true. |
|
Generally looks good. Could you add a CHNAGELOG line, please? |
SilverFire
removed
the
status:to be verified
|
OK. I made those changes. Thank you! |
| return; | ||
| } | ||
| $data = json_decode($value, true); | ||
| if (count($data) === 3 && isset($data[0], $data[1], $data[2])) { |
There was a problem hiding this comment.
I'm not sure count($data) check is needed. Also count() won't work with non-arrays which may appear after json_decode. issets should be enough.
There was a problem hiding this comment.
Additionally, it's better to use Json::decode because of enhanced error handling.
There was a problem hiding this comment.
I changed my code to use Json::decode and did some testing. If the contents of the cookie are malformed in some way and Json::decode is used, then an exception will be thrown. When json_decode is used, my code recognizes that the contents are not usable, no cookie login is performed, and a standard login screen is presented. I think the behavior using json_decode is preferable, so I change it back.
|
Thank you @maine-mike! |
