-
Notifications
You must be signed in to change notification settings - Fork 791
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
LoginController only for login flow #747
Conversation
Develop update from head fork
Logout action should be do in other way. Repeat this 3 lines every time we need valid CSRF, will be confusing.
All pages (user actions) in UserController are visible for logged-in users
Wow, thanks, this looks really good! :) Please gimme some days for testing! |
No problem! ;) |
* Register page action | ||
* POST-request after form submit | ||
*/ | ||
public function register_action() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't this be better as index_action() to fit the naming pattern used for action-action_post_handler theme used throughout this framework.
Corresponding change would be needed here: https://github.com/panique/huge/pull/747/files?diff=unified#diff-5ac2d5faa50e65202dd471632e2632f2R11 (application/view/register/index.php L11)
Major changes missed:
Minor changes missed:
Potential changes to discuss:
Summary:There are a few small quickly fixable errors that I have listed above but otherwise this pull request is solid. I hope this analysis helps get this request accepted faster. |
Thanks, I'll have a look on this on the weekend |
Major and Minor changes missed Potential changes to discuss Captcha can be used in other functionalities not register user only. |
@slaveek The captcha functionality is probably okay in the register controller because captchas as I've seen captcha have only been used in while registering user accounts. I made the major and minor on a clean branch that I pulled |
OK! I'll fix it and I'll add more commits in this branch. |
Bugs found by @geozak see: panique#747 (comment)
As pointed in the very first comment in https://github.com/panique/huge/blob/develop/application/core/Csrf.php
This reverts commit 851d594.
Thanks a lot, looks really good! :) |
LoginController only for login flow
Bugs found by @geozak see: panique/huge#747 (comment)
This pull request contains changes related to #549 issue.
Sorry for a small typo in branch name ;)