Menuperformance

It appears you are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.
Review of the master branch:

• README.txt is missing, see the guidelines for in-project documentation.
• Remove "version" from the info file, it will be added by drupal.org packaging automatically.
• Drupal Code Sniffer has found some code style issues (please check the Drupal coding standards). See attachment.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. Go and review some other project applications, so we can get back to yours sooner.

manual review:

• menuperformance_install(): this is a hook implementation and needs to be documented as such, see http://drupal.org/node/1354#hookimpl
• menuperformance_install(): instead of juggling with unstable module weights you can make use of hook_module_implements_alter() the re-arrange the hook execution order of modules.
• "'#title' => 'Parent item',": make sure to raun all user facing text through t() for translation.
• do not use drupal_add_js() and drupal_add_css() in hook_form_alter(), use $form['#attached'] instead. See http://api.drupal.org/api/drupal/developer--topics--forms_api_reference....
• "form_set_error('menu][parent', 'Invalid parent menu string. Format should be "MENUNAME:MLID".');": again, this should be translated. Check all strings in your module.
• menuperformance_ajax(): do not use die() here, use drupal_json_output() and drupal_exit().
• menuperformance_get_link_children(): "@param String $menu_name": String should be lower case, description should be on the next line. And the function doc block should contain a short description on the first line.

Hi

1) menuperformance.install

Your install file is empty. If you don't need it you can remove it.

2) menuperformance.js
As of jQuery 1.7, the .live() method is deprecated. Use .on() to attach event handlers.
See http://api.jquery.com/on/

3) menuperformance.module
t() is not needed for hook_menu in title and description.

4) menuperformance.module
FAPI will NOT sanitize the '#options' attribute on other elements than select boxes. Could open up XSS exploits as the menu could be create by users.

418 $form['menu']['menu_options'] = array(
419 '#type' => 'checkboxes',
420 '#title' => t('Available menus'),
421 '#default_value' => variable_get('menu_options_' . $type->type, array('main-menu')),
422 '#options' => $menu_options,
423 '#description' => t('The menus available to place links in for this content type.'),
424 );

Please check http://drupal.org/node/28984

Thanks,

Tested this and found the following bug. Steps to reproduce is as follows. The test was done on a clean Drupal 7.12 installation.

1. Login as admin and enable the module according to the instructions in the README.txt
2. Go to Content > Add content > Basic Page
3. Add a title as "Test Bug" and the body of the content type Basic Page
4. Goto Menu Settings and click on the Provide a menu link check box in the node edit/create page
5. Select "Main menu from the parent item" and save/create the node
6. Now click on the edit tab on the created node again
7. Go to the Menu setting section and select 'Test Bug' Menu item from the second drop down list (child menu list) of the Parent item fieldset and save the node again.
8. Now click on the edit tab again and you can see plenty of child item select boxes has been generated for no reason.

Also I can see why you display the menu mlid before the Parent Item select box but personally think that you should not call two form elements by a same name/title (in this case 'Parent item') because this will be confusing to the end user in a UX perspective. Also removing the PAReview: review bonus tag as you can get it back by reviewing another three issues in the application queue.

@firebird: You are actually right about the security issue, it is not necessary to sanitize #options for checkboxes and radios in Drupal 7 anymore. Thank you for pointing that out, I have updated the documentation page to address that. http://drupal.org/node/28984

That's why I love this review process, always learning something new about Drupal :-)

For the record:
No coding standard issues can be found by Ventral.org's online PAReview http://ventral.org/pareview/httpgitdrupalorgsandboxfirebird1403852git

Please update your direct link to your git repository. Will make all of our lives easier.
Despite the code refactoring, I can't find any bugs so kudos ;)

+1

Added links to reviewed projects

Couple of quick notes,

There is an error in your README.txt file. Your how to use directions say to go to /config/user-interface/menuperformance it should be admin/config/user-interface/menuperformance

I would also recommend putting this link and information into a drupal_set_message when the module is enabled. Not everyone reads the README.txt until something goes wrong. It does help to have a "how to test" section in the issue summery.

Also your README isn't formatted correctly. A couple of the lines end on the first letter. I like to give short words a little bit more of a buffer (78 chars wide) to help people who use vi and the console.

On the bright side it does pass PAReview with no errors and it works. I installed with no real issues (just not knowing why it wasn't working till I read the README.txt) and I like the concept.

Automated review issues:
Review of the 7.x-1.x branch:

• Run coder to check your style, some issues were found (please check the Drupal coding standards). See attachment.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. Get a review bonus and we will come back to your application sooner.

Severity minor, Drupal Commenting Standards, Internationalization, Drupal Security Checks, Drupal SQL Standards, Drupal Coding Standards

sites/all/modules/pareview_temp/./test_candidate/menuperformance.install:
 +12: [normal] The $message argument to drupal_set_message() should be enclosed within t() so that it is translatable.
 +12: [critical] Potential problem: drupal_set_message() only accepts filtered text, be sure to use check_plain(), filter_xss() or similar to ensure your $variable is fully sanitized.

Status Messages:
 Coder found 1 projects, 1 files, 1 critical warnings, 1 normal warnings, 0 warnings were flagged to be ignored

Source: http://ventral.org/pareview - PAReview.sh online service

Fix this and I will rtbc, the issue wasn't so much the misformed README.txt file as much as it was the wrong url in the README.txt file.

The filter_xss() and t() is there. Marking rtbc.

1. As installation and usage instructions are quite important for us to review, please take a moment to make your project page follow the tips for a great project page. Also make sure your README.txt follows the guidelines for in-project documentation.
2. The error about the possible security issue (mentioned in #14) is a false positive - don't trust these automated reports too much ! they're pretty dumb when detecting possible security issues!drupal_set_message(filter_xss(t('Menuperformance module activated. To take the module into use, <a href="@baseurl/admin/config/user-interface/menuperformance">go to the settings page</a> and check the checkbox.', array('@baseurl' => $base_url))));
   It makes no sense to add both, t() and filter_xss() as t() already filters all used variables (prefixed with @ or %) and the text your filtering is static => contains no user input and is therefore harmless.
   Also there's no need to add the @baseurl to the path - but you have to put the path though url().drupal_set_message(t('Menuperformance module activated. To take the module into use, <a href="!url">go to the settings page</a> and check the checkbox.', array('!url' => url('admin/config/user-interface/menuperformance'))));
3. You've got a settings page -> add it with "configure = " to your .info
4. please put a newline in the comment block between "implements.. " and the documentation what it's good for
5. if possible you should rather use the "weight" field in the system table to control in which order hooks should be implemented instead of using "hook_module_implements_alter()" which is more performance intensive
6. wouldn't it make sense to reset the "menu_override_parent_selector" variable to FALSE when disabling the module ?

Anyways, there are a lot of TODO's marked, also related with access checking, you really should resolve these issues before thinking about creating a release. Also don't show developer information to normal users (you probably planned to remove that anyway) but a debug mode is always good ^^

Beside that I think you know what your doing and I like how detailed your documenting your code inline ;)

Thanks for your contribution and welcome to the community of project contributors on drupal.org.

I've granted you the git vetted user role which will let you promote this to a full project and also create new projects as either sandbox or "full" projects depending on which you feel is best.

Thanks, also, for your patience with the review process and for your help in the queue! 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.

As you continue to work on your module, keep in mind: Commit messages - providing history and credit and Release naming conventions.

Thanks to the dedicated reviewer(s) as well.

One correction to #5 in Patrick's review: it is quite the opposite: do not juggle with module weights where possible, as this is unreliable and unpredictable. Hooks are cached, so there is no performance penalty for using hook_module_implements_alter().

Ohkay, good point! (shit I've got to patch my modules...)
thanks :)