In the modules code the form validation is appended by rewriting the array. This is very very bad since other modules could as well hook into the validation and of course their validator will not work because it is overwritten.

This

$form['#validate'] = array(
      'user_login_name_validate',
      'ga_login_user_login_validate',
      'user_login_authenticate_validate',
      'user_login_final_validate',
    );

should be

$form['#validate'][] = 'ga_login_user_login_validate';

To explain. If some other module adds a validator to the login form and your module comes next it removes that validator again
and the guy will have a hoot debugging it.

Comments

attiks’s picture

attiks commented
Version: 7.x-1.2 » 7.x-1.x-dev
Assigned: Unassigned » jelle_s

You're right, but the idea is that the validation happens before user_login_authenticate_validate, if we don't rewrite everything, we'll need to add some logic to detect what is defined and re-arrange it.

mbraun’s picture

mbraun commented

You could splice the array. I.e.

	// add our validator to the chain
												
	$replacement = array(
		'ga_login_user_login_validate',
		'user_login_authenticate_validate',
	);
				
	$key = 0;
	foreach ( $form['#validate'] as $validator )
	{
		if( $validator == 'user_login_authenticate_validate' )
			break;
		$key++;
	}
	array_splice( $form['#validate'], $key, 1, $replacement );

This way you won't mess with other functions in the chain.

If you plan to stick to rewriting it for now you should place a warning for that somewhere where people see it. The problem from a user's POV is that another module may be there and active but non functional. Specifically bad if that one doesn't require any input. For example an IP blacklist or whitelist.

Edit: You may want to also check if user_login_authenticate actually is IN the array first. :-)
It should but if it isn't that function up there will replace something else.

attiks’s picture

attiks commented

@mbraun can you provide a patch?

mbraun’s picture

mbraun commented

Yeah I can.

Guess tomorrow. Against -dev

attiks’s picture

attiks commented

k, thanks

mbraun’s picture

mbraun commented

HA! Case closed.

I must have checked the wrong archive when I looked up the dev version on Saturday to see if the problem still exists in dev. But dev does not overwrite the array anymore. You are "unshifting" it. Not much of a difference there between unshifting and splicing if you want it before user_login_authenticate_validate.

No patch required. The ticket is bs for current dev.

attiks’s picture

attiks commented
Assigned: jelle_s » Unassigned
Status: Active » Closed (works as designed)