Splitting this out from #1410844: Switch from hook_user_insert to submit handler so the code fires earlier.

Right now the module doesn't explicitly handle other modules that create users in a flow separate from the normal user flow, such as commerce.

It would be nice to figure out some way to handle those better. One idea is just to change the module to work on the submit handler of the normal user register form. That way it *wouldn't* fire during something like commerce's checkout. But is that really desirable for it to not interact with the commerce user creation process? My sense is that this module should still try to hide username fields and still try to generate usernames even if the user is being created by commerce.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

greggles’s picture

For what it's worth: I'm not affected by this bug so I don't plan to work on it, but I'd be happy to review a patch.

sportel’s picture

Wouldn't it be a good alternative if this module would handle the creation of the username the same way as commerce does? Just the e-mail address as the username? To me that seems much easier than johndoe_1, johndoe_2 ...

FiNeX’s picture

I agree with sportel.

mibfire’s picture

Why cant we use hook_user_presave?

/**
 * Implements hook_user_presave().
 * A user account is about to be created or updated.
 */
function MYMODULE_user_presave(&$edit, $account, $category) {
	if ($category == 'account' && valid_email_address($edit['name'])) {
		$edit['name'] = 'email_registration_' . user_password();
	}
}

One line and it solves the problem.

greggles’s picture

re #2 and #3: The username in Drupal has a way of "leaking" out of a site. See the official stance and username enumeration prevention for an attempt to prevent it from happening (though by no means is that complete).

If you put the email address into the username then you will have issues revealing people's email addresses to other users or, worse, to the world.

Re #4 - is that a proposed change for email_registration? If so can you provide it as a patch so the testbot can test it?

mibfire’s picture

It is proposed yes.

mibfire’s picture

greggles’s picture

Status: Active » Needs review

Setting to needs review so the bots will know to test it.

Status: Needs review » Needs work

The last submitted patch, 7: email_registration-creat_users_checkout-1899720-6-D7.patch, failed testing.

The last submitted patch, 7: email_registration-creat_users_checkout-1899720-6-D7.patch, failed testing.

greggles’s picture

Status: Needs work » Needs review
FileSize
741 bytes

Here it is re-rolled so it will at least apply.

mibfire’s picture

What was the problem? I used only the diff.

greggles’s picture

The diff was trying to patch a file that doesn't exist.

Basically if you create the diff not using git then it's less likely to apply using git. Whenever I want to propose a change for a module I go to the version control tab for it and make a clean checkout of the dev branch and make my change in that environment to ensure the patch applies to the latest version of dev code.

mibfire’s picture

thx greggles

mibfire’s picture

I was thinking on this. Maybe it is better to place this code into hook_user_insert

/**
 * Implements hook_user_insert().
 */
function email_registration_user_insert(&$edit, &$account, $category = NULL) {
  // Don't create a new username if one is already set.
  if (!empty($account->name) && strpos($account->name, 'email_registration_') !== 0 && !valid_email_address($account->name)) {
    return;
  }

What do you think?

greggles’s picture

I don't currently use commerce, so I don't have a strong opinion on this idea. Folks who do use it will need to provide and test out the patches and confirm they work and I can then test them to confirm that they don't break the normal use cases of the module.

mibfire’s picture

This is independent of commerce. My aim is to solve this globaly. The presave will work as well but that is unnecessary and we will have better performance using user_insert.

greggles’s picture

Well, I don't know what you mean by "globaly" because I use this module on a site with 113 other modules and don't have any problems. So, it's not a "global" thing, it's an issue that affects a subset of people.

So...if someone posts a simplified repeatable test case (ideally as a simpletest) I'll gladly review the patch they believe addresses the problem.

mibfire’s picture

I mean globaly when user is created somehow, like when user is created manually.

Advin’s picture

Use patch from #11 and receive such notice after checkout.

Notice: Undefined index: name in function email_registration_user_presave() (line 180 in file .../sites/all/modules/email_registration/email_registration.module).

commerce 7.x-2.1 and Email Registration 7.x-1.2.

Should i use dev version of Email Registration?