Folder Menu

Removing review bonus tag, you have not done any manual review, you just posted the output of an automated review tool. Make sure to do read through the source code of the other projects and follow http://drupal.org/node/1587704 . Single coding standard errors are surely no blockers.

Mannual Review

In .module file

1)The menu path for the folder setting page should be like 'admin/settings/folder_menu' instead of 'admin/build/folder_menu' so that you can see the name in the site configuration
2)Use 'Implements hook_menu().' instead of 'Hook_menu.' and the same applies with all the hook function
3)Use drupal_add_js in hook_init
4)Variables used in the .module file must be uninstalled in .install file eg 'folder_menu_menu_parent'

In .info file

1)You have to change the path of configure

In your git repository link should be 'http://git.drupal.org/sandbox/webdorado/1768438.git'.The one you have mentioned is wrong

I have downloaded and installed your module but i am not able to view the effect as per settings in folder menu.Might be i am missing something,Please check at your end whether it is working

Thanks,
Suhani Jain

Setting to "needs work" per #4. The review bonus tag was removed, you can add it again if you have done another 3 reviews of other projects.

Added 3 more Reviews of other projects

Thanks for your reviews, just make sure that you pick the oldest applications first.

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

1. Your commit messages only contain numbers, please use some more meaningful in the future. See http://drupal.org//node/52287
2. AC_RunActiveContent.js appears to be 3rd party code. 3rd party code is not generally allowed on Drupal.org and should be deleted. This policy is described in the getting involved handbook. It also appears in the terms and conditions you agreed to when you signed up for Git access, which you may want to re-read, to be sure you're not violating other terms. The Libraries API module is a recommended method for adding 3rd party dependencies without directly including the code on Drupal.org. Same for jscolor.
3. menu.php: why is this file needed? Please improve your @file comment.
4. "variable_get('menu_background_color'": all variables that your module defines have to be prefixed with your module's name to avoid name colissions.
5. menu.php: this will only work if I place the module directly into sites/all/modules, but not if I place it sites/all/modules/contrib for example.
6. README.txt: you have some leftovers from git merge conflicts in there.
7. Advertising on admin screens is not allowed for Drupal projects, see http://drupal.org/node/439226
8. menu.php: this is vulnerable to XSS exploits. If I enter something like "><scrip>alert('XSS');</script></menu><menu cap=" as menu title, then the flash plugin runs amok and slows down my browser. Not sure how this could be exploited in a better way, as I'm no a Flash expert. You need to sanitize user provided input before printing, see http://drupal.org/node/28984

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Added 3 more Reviews of other projects:
http://drupal.org/node/1789676#comment-6499784
http://drupal.org/node/1758810#comment-6499800
http://drupal.org/node/1775004#comment-6499828

manual review of the 7.x branch:

1. I still don't understand why this file is needed. You are doing a full Drupal bootstrap anyway, so you could just define a route in hook_menu() that returns the XML?
2. folder_menu_uninstall(): all variables that your module defines have to be prefixed with your module's name to avoid name colissions.
3. folder_menu_createFlashDiv(): You should put all your javascript into dedicated JS files. You should use Drupal.settings to pass variables from PHP down to javascript. See http://drupal.org/node/756722
4. folder_menu_createFlashDiv(): this is vulnerable to XSS exploits. If I enter "><script>alert('XSS');</script> for "Distance from top" I get a nasty javascript popup. You have sanitize all user provided input properly before inserting it into HTML. See http://drupal.org/node/28984

The security issue is a blocker, so I have to put this back to "needs work". Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Added 3 more Reviews of other projects:
http://drupal.org/node/1758048#comment-6537282
http://drupal.org/node/1416752#comment-6537300
http://drupal.org/node/1706922#comment-6537316

Long term, you should consider using swfobject instead of AC_FL_RunContent, as it has been a defacto standard for a while.

The comment on folder_menu_admin() is incorrect; it isn't a hook, it's the menu callback from the hook_menu page.

You may also want to consider adding a hook_requirements() to check for the JS libraries so users see errors in the Status Report.

Minor typo in error message "Distance from top must bu numeric."

May want to consider using the Javascript API instead of directly using onmouseover/onmouseout on the div with the menu, since you may have a race condition where the JS events for these will fire before the JS files have loaded (esp if a themer puts JS at the bottom of the page).

The menu entry for the XML shouldn't live under admin/. Some times secure all of these pages with Secure Pages, and this would end up mixing HTTP and HTTPS.

The menu entry for the XML is being served up as text/html instead of text/xml

.../folder_menu/menu_xml is still being served up as text/html and not text/xml. You need to set the Content-Type header.

JS. Even if you do ensure that your code doesn't have a race condition, eventually changing the code to use the Javascript API is a best practice, and will also help with namespace pollution. Upon closer examination, it does look like you have one function (initFlash_vertical), that isn't namespaced by your module.

While not necessary, using hook_requirements() let you do two things. One, you can display the error about the missing libraries when the module is enabled so the user can see the message immediately. Two, you can put the warning on the Status Report, which is a central place for checking for problems.

I agree that the JS and hook_requirements aren't blockers.

The content type is coming up correctly, but brute forcing with header() isn't the best option. For example, If you look at the source for theme_aggregator_page_opml(), you will see that it does

drupal_add_http_header('Content-Type', 'text/xml; charset=utf-8');

You may want to address this.

Thanks for your contribution, webdorado!

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.