Posted by andremolnar on November 21, 2011 at 4:17pm
5 followers
| Project: | Dialog API |
| Version: | 7.x-1.x-dev |
| Component: | Code |
| Category: | bug report |
| Priority: | major |
| Assigned: | Unassigned |
| Status: | needs work |
Issue Summary
Problem/Motivation
When testing the dialog user module, form handling behaved in unexpected ways. Validation errors were not displayed correctly (returning raw JSON instead).
The password form did not load into the dialog window.
Errors are produced if you are logged in an manage to click a login link in another tab or window.
Proposed resolution
Rework the logic of the AJAX callback for the user forms.
Change the way Ajax behaviour is attached to user forms.
Update what happens on when ajax submit handler is executed.
Include a file to allow password form to load.
Alter the menu callback permissions to avoid errors on edge case described above.
See code comments in patch.
Remaining tasks
None.
User interface changes
None.
API changes
None
Comments
#1
Patch attached.
#2
This is an updated patch that also handles redirects properly.
This patch also includes the attachment of dialog links to the user_login_block form as well as links provided by comment module. In the case of comments module, there are redirects to take you to the appropriate location for leaving a comment which inspired the change for redirect handling.
#3
I'm attaching an updated patch that does the following:
- fixes the "Forgot your password" form so it works in the dialog (just needed a module_load_include())
- moves the user_login_block-specific part of the form_alter into its own dialog_user_form_user_login_block_alter() function
I also made some code style changes.
#4
Providing a follow up patch with fixes an issue with permissions on the menu callback. I am updating the issue to include this and #3.
In a nutshell there is an edge case where a user can have two tabs open. If they log into one tab, and then attempt to log into the other tab, the menu callback permissions won't allow the ajax request and will spew an error to the user. This updated patch includes a fix which checks if the user is anonymous before deciding what dialog action to perform. If the user is logged in the dialog will just refresh the page.
#5
+ 'access arguments' => 'access content',+ 'access callback' => TRUE,
The 'access arguments' property is supposed to be an array, like
array('access content'), rather than a string, so I'm seeing a number of PHP notices as a result of this code.Also, if 'access callback' is TRUE, doesn't that mean everyone has access, and the
'access content'check is therefore ignored? I think you want one or the other of those checks here, not both.#6
Thanks - right you are David - updated patch.
#7
Changing the status to needs review.
#8
$items['user/register/%dialog_js'] = array('page callback' => 'dialog_user_ajax_callback',
'page arguments' => array(1, 2),
- 'access callback' => 'user_register_access',
+ 'access callback' => TRUE,
This looks incorrect, since it provides a backdoor way for users to register new accounts even on sites that don't allow user registration.
I did some quick testing, and an account registered that way seemed to be treated as "needing admin approval" (rather than as a completely open registration), so it's not as bad as it could be. But it's still a problem that this will allow users to register accounts on their own if the site was configured such that only administrators are supposed to be able to create new accounts.
#9
Yeah, not sure that's the correct way around it.... Not sure this is either...
#10
Just found this issue after posting patches on #1457230: Login or Register does not validate. 7.x-1.x-dev and #863632: Demo: login and register do not work that make similar changes. The patches on those issues enable the three forms in dialog_user module. The patches on this issue address the permission (or access callback) and some refactoring that were not considered in my patches. Also some of the changes on this issue do not work (for me) such as that in #9 to dialog_page_alter().