User Request Name

Welcome,

We are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and I'll take a look at your project right away :-)

Hello,

1. Line 80 of the user_requestname.module, the == check is not necassay, you could just say this "if (variable_get('user_requestname_check_enabled', USER_REQUESTNAME_CHECK_ENABLED))"
2. You have a spelling mistake at line 87 of the user_requestname.module ... Throw.. not Trow :P
3. Same deal as line 80 but on line 102 in user_requestname.module.
4. You placed the configuration page under admin/config/system/, IMO i think it'd be better under admin/config/people since it is related to users of the site.
5. You could try to wrap some pieces of code to 80 characters instead of letting them go over.

Overall, its a pretty clean module. I like it.

Number #5 is subjective, I just code on a 13' screen most of the time and so always try to put everything and anything under 80 characters. In the end, its up to you.

Here are some examples from your module that I would wrap.
1. user_requestname.admin.inc, lines 17, 23, 29 and 35.
2. user_requestname.module, line 69, 103, 139, 204 - 215, 302

The example you posted in your reply doesn't clean up very nicely, so I would leave it as is.

In user_requestname.module file
1. Spelling error at line 103: should be "does not disappear" and not "does not disappears".
2. I think that menu item "user/username" is accessible from all users, even though it doesn't have any use for authenticated users.
3.Also, wouldn't it be better if you separated the "attributes" array of the l() function calls, to separate lines?
4. In function user_requestname_mail() you don't check the $key variable. Since you have only one key, a check is almost unnecessary, but it's another measure to make sure everything will work correctly.

In user_requestname.admin.inc file
5. You could replace the double quote(") with single quotes(') if you used token replacements.
Eg. line 29:
t("Always return 'Further instructions have been sent to your e-mail address.' Even if the account does not exist.")
could be

 t('Always return %message even if the account does not exist.', array(
  '%message' => t('Further instructions have been sent to your e-mail address.')
)); 

The use of %message will also stylize the message with a <em>...</em> as stated in http://api.drupal.org/api/drupal/includes!bootstrap.inc/function/format_string/7

Hi Rob C! Your code seems clean from my side. I will expect someone else to mark it as reviewed and tested by the community though, since I'm kind of new to all this.
I have only one thing to ask: why do you use $GLOBALS['user'] and not global $user? I am kind of used to read the $user variable as always referring to the global $user variable of Drupal and the $account variable as always referring to a local user we have loaded through user_load() function. As a result, $_GLOBALS['user'] is totally correct but I was unfamiliar with it. In Drupal module coding, at least.

Hi...
1. user_requestname.module :
You are calling user_load_by_mail() both in _validate and _submit() in user_requestname_request_form form you can use form_set_value()
2. Not a major one but instead of using more than one hook_form_FORM_ID_alter() use can you one hook_form_alter().
3. user_requestname.install : if you using direct db quires then you can use

  global $conf;

  db_delete('variable')
    ->condition('name', db_like('user_requestname_') . '%', 'LIKE')
    ->execute();
  cache_clear_all('variables', 'cache_bootstrap');

  unset($conf[$name]);

1. You are using user_load_by_mail twice. Theoretically, it should be faster to call it once (in validate, since you need to validate that mail exists before submitting) and then pass the value directly to _submit through the form using form_set_value. Anyway, I am not sure if it's really faster, since user_load_by_mail uses entity_load functions, and it looks like they are "caching" loaded entities in memory.
2. You can differentiate many forms inside one hook_form_alter, using a switch with $form_id variable. I don't really know if it's better to use one hook_form_alter or many hook_form_FORM_ID_alter.
3. Right way is to use variable_del. But as you have many user_requestname_% variables, you need to use the query. All variables are loaded from the table once on the first page load after caches are cleared, and then cached. If you don't clear variable cache, those variables will be there until next flush, that why also variable_del use it.

Now my review:
1. I am not sure if this function should be improved: user_requestname_form_user_login_block_alter. You are recreating $form['links'] from the scratch, so if you have many modules that you as your module do, they will be overlapping and the functionallity will be destroyed: only the last executed will be shown. Anyway, I don't know if it should be a patch in core (for preserving original $items anywhere in the form withouth theming) or you should create another markup form field with your link.
2. Please, also try to fix those validation warnings and errors:
http://ventral.org/pareview/httpgitdrupalorgsandboxclusterfck1697168git

Pretty useful module for some use cases, thanks for contributing! I've done a manual review and it looks like there are still some old issues and a few new ones:

1. Readme indicates configuration is located at admin/config/people/user_requestname, hook_menu() implementation defines it at admin/config/system/user_requestname
2. Default language in mail is a bit confusing. Maybe "A request was made to remind you of your username on [site]."
3. README.txt line 44 "supress" should be "suppress"
4. README.txt line 51 "multilangual" should be "multilingual"
5. README.txt line 58 "corrent" should be "correct" (I think)
6. user_requestname.install line 13-15 uses method for deleting variables that *may* be problematic if others write a module that begins with your module name, for instance, "user_requestname_extended". Unlikely, and it's not necessarily *wrong*, just thought I'd bring it up.
7. user_requestname.admin.inc line 11 - even if you don't use them, FAPI form implementations should take at minimum the $form and &$form_state parameters
8. user_requestname.admin.inc line 16, 30 - consider using double quotes in order to avoid having to escape the single quotes
9. user_requestname.module line 102 - "Trow" should be "Throw"
10. user_requestname.module line 163 - missing $form_id parameter
11. user_requestname.module line 277-286 - is this an actual hook? unable to find it in Drupal api documentation

This looks like a great module. I hope to add it to my sites one day!

I have reviewed and tested the module. There appears to be a potential issue with the flood table. When I request my username, there are multiple identical entires added to the flood table. This may cause false flood warnings. See the attached file.

Manual Review

First thanks for your Work

i install the Module on a Live Site and test it a bit. Here my recommendations.

add link for the configuration here admin/modules.
in the email field make a prof if its a email. now nothing will checked and a "email" like <script src=http://evil.example.com/xss.js"></script> is accepted

its old but maybe it helps check http://drupal.org/node/279127
validation is major i think! here is a link http://api.drupalize.me/api/drupal/function/email_field_validate/7
see last link for d8 api ...it comes ;)

others:
call watchdog to see whats happend and how many request are sended.

future:
try to get it working with caching modules like boost
this was happed on my site
Referrer ./user/username
Nachricht Notice: Undefined index: args in boost_deliver_html_page() (Zeile 1396 von ./sites/all/modules/boost/boost.module).

resource for d8 if u plan http://api.drupal.org/api/drupal/core!includes!form.inc/function/form_va...

Thanks for your time to get this to the community

Small review for .info file, please include configure = admin/config/people/user_requestname in the user_requestname.info file.

Will get back with more detailed review. For the time being you could review other projects and get review bonus. Though you have already reviewed one project, please review 2 more and add PAReview: review bonus tag.

Honestly, the single thing I've found wrong was this:
in user_requestname.module line 100 the error within form_set_error should be wrapped with the t() function

This module is well-written and clearly structured. Also, the functionality it provides seems like it would be important to most sites. I know I've forgotten my username on various sites/occasions.

Also, I love the focus on security, especially the flood control implementation. For instance, you check flood_is_allowed() in the form generation function and the validator. I also love that you have a setting to show a success message even if the user enters an invalid email. This is great for security, keeping people from enumerating registered emails on the site. Lastly, I appreciate that you wrote tests to verify your module's functionality.

All I could find were a couple typos:

user_requestname.module
Line 100: t('Reset password limit exceeded. Please contact support for further assistance.'));
Should it be "Request username limit exceeded" instead of "Reset password"?

Line 293: @param array $user should be @param object $account

Line 383: "Message when a user requests their name to be send to them via e-mail." should say "sent" not "send"

Line 427:

@param array
   *   $options

should be @param array $options

Since these are all minor things, and since the reviews before mine seemed positive with little to fault, I'm setting this to RTBC.

Sorry for the long waiting time!!

• Your readme is very informative! Maybe you can keep your project page in-sync with it (few people are actually reading the readme-files)
• Maybe you could provide a #description for the "admin/config/people/user_requestname"-path?
• user_requestname_form_user_login_block_alter(): It would be good if you could find a cleaner way (eg. theme-function) to build and replace that markup
• Your code is not restricted by 80-characters, only your comments are. (Putting strings over multiple lines with indentation, especially if they are processed by t() should not be indented! otherwise it gets really hard to translate these strings!)

Beside that I don't think there are any blocking issues, so let's get this promoted!

Thanks for your contribution!

I updated your account to let you promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and get involved!

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.