Webmaster menu

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:

• Run coder to check your style, some issues were found (please check the Drupal coding standards). See attachment.
• 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. Get a review bonus and we will come back to your application sooner.

Now you have a 7.x-1.x branch, instead of master branch.
You must delete all files in master branch. Follow guidelines, see step 5 in http://drupal.org/node/1127732.

You must remove useless comments like:
// ------------------f Line 14 (webmaster_menu.config_page.inc

Have you consider user system_settings_form() instead of manually set system variables?

Hi!
Master branch still contains files.
See #2 and follow guidelines to set master branch only with README.txt file.

I cloned the D7 version of the webmaster menu module on a fresh localhost install. The module installed fine via Drush.

I then created an example menu called 'Quick Toolbar' and added a few basic links to it (login, register). I then went to the webmaster menu configure form and specified my 'Quick Toolbar' menu to be used as the webmaster menu and specified it could be seen by anonymous users.

This was my first test case, and it worked as expected. I then began flipping on/off the other options available on the webmaster menu screen, adjusting the user role visibility settings, the 'add home link', the 'add logout link' and performing tests for each. The behavior worked as expected for each test case.

One odd thing I noticed was the webmaster menu still rendered itself even when there were no links to show. For example, an authenticated user still sees the webmaster menu (empty) when the only links in the 'Quick Toolbar' menu are user/login and user/register. I would expect the webmaster menu to be hidden in this case.

I then tried the 'add an extra menu to the webmaster toolbar' and added the 'Main menu' (which I placed a few links to devel generate nodes in) to the webmaster toolbar as well. It did bring in the links from the 'Main menu' and also provided the links from the 'Quick toolbar' menu, as expected.

This is a neat little module, I could see it being useful for beginners as an easy way to get a toolbar. I would like to see it have drop down capabilities. For example, I would picture my 'Quick Toolbar' to be listed in the toolbar as a menu link, then when a user clicks/hovers over it it would pop up the mneu items (login,register). Then if the 'Main Menu' was clicked/hovered, it would popup the links I had added to my nodes. This is how I pictured the module would work in my head before playing around with it. (but that's just me) Additionally, it would be nice to see some selectable color options for the background of the webmaster menu (in the future of course).

The module uninstalled cleanly via drush as well.

There are some minor suggestions from PAReview here: http://ventral.org/pareview/httpgitdrupalorgsandboxrosell1417602git

Review of the 7.x-1.x branch:

• Drupal Code Sniffer has found some issues with your code (please check the Drupal coding standards).
  

  FILE: ...pal-7/sites/all/modules/pareview_temp/test_candidate/webmaster_menu.css
  --------------------------------------------------------------------------------
  FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
  --------------------------------------------------------------------------------
   46 | ERROR | Expected 1 space before opening brace of class definition; 0
      |       | found
  --------------------------------------------------------------------------------
  

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.

manual review:

• "'#options' => user_roles(FALSE),": user roles need to be sanitized before printing to avoid XSS vulnerabilities (take a look at the drupal core code for the roles page). See http://drupal.org/node/28984
• webmaster_menu_config_form(): you could use system_settings_form(), then you don't need the submit function.

The security issue is a blocker, otherwise this looks nearly ready to me.

IMHO this module looks ok. I don't see any security flaws, and no bad practices.

There is only one minor thing which should be corrected as detected by drupalcs:

• 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. Get a review bonus and we will come back to your application sooner.

FILE: ...l7/sites/all/modules/pareview_temp/test_candidate/webmaster_menu.module
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 65 | WARNING | Line exceeds 80 characters; contains 97 characters
--------------------------------------------------------------------------------

... well yes probably variables should all be sanitized on output ... see http://drupal.org/node/28984
... as I do not have much time now, I'll recheck this later an leave the status as-is.

  $html = '<li class="' . implode(" ", $menu_item['classes']) . '">';
  $html .= l($menu_item['title'], $menu_item['href'], $menu_item['localized_options']);
  if (isset($menu_item['children'])) {
    $html .= theme('webmaster_menu_tree', array('tree' => $menu_item['children']));
  }
  $html .= '</li>';

Undoing my status change.

Hi,
Manual Review:

webmaster_menu.install :

1. Do not use t() or st() in installation phase hooks, use $t = get_t() to retrieve the appropriate localization function name.
2. function webmaster_menu_install() contains a Potential problem .drupal_set_message() only accepts filtered text, so be sure the !placeholders for $variables in t() are fully sanitized.
3. If your Line exceeds 80 characters, make it as 2 lines.

webmaster_menu.css :
You should follow alphabetical order for Multiple CSS properties.

Use of st should be ok in this situation.

Use t() if your code will never run during the Drupal installation phase. Use st() if your code will only run during installation and never any other time. Use get_t() if your code could run in either circumstance.

I looked this over - few things to say, as it generally looks good.

1) That st() in webmaster_menu_install() should definitely be done via $t = get_t() then use $t() as the function call. Install hooks can be called either during a profile installation of Drupal or after Drupal is fully installed. Ergo, use get_t(), as t() is not going to work when modules are installed during an installation of Drupal.

2) It looks like you could give anonymous users access to the menu, and also configure it to have a logout link - and do both at the same time. So should you prevent that from happening somewhere in the options, or by checking to see if the user is logged in when showing that link in the menu?

I don't see any issues with sanitization of output in the theme functions - things that luxpaparazzi pointed out are getting passed into l(), which does check_plain() the text and check_url() the url.

Ok then, don't see why we can't set this to reviewed and tested by the community.

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 :-)

Thanks for your contribution, rosell.dk!

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 as well to the participants here for your reviews and assistance to rosell.dk!!

Thanks for your patience, Bjørn. You should be all set now.

Formatted the description for better overview