[D6] Delete menu options

Automated review:
http://ventral.org/pareview/httpgitdrupalorgsandboxmjgmaas1886610git

You should also take a look at the tips for a great project page.

Hi,

I did a manual review of your code, here's the result:

• README.TXT

This file is less detailed than the project page, maybe add at least the same amount of information

• delete_menu_options.info

You should add a dependency to the "menu" Drupal core module that can be deactivated. If Menu is deactivated, the functionalities of your module are not accessible.

• delete_menu_options.install

l13: see drupal function st() http://api.drupal.org/api/drupal/includes!install.inc/function/st/6

• delete_menu_options.module

l14: Comments should use proper grammar, add ending dot (your other comments are fine so I think it's a copy/paste mistake)
l22: Mispelled words in sentence, should be "This hook adds 2 checkboxes which the user may select to delete child menu items and/or the referenced node(s)."
l32: The sentence is not very clear, I'd write "Delete nodes referenced by the menu items that will be deleted (orphaned nodes only)."
l84: children and not child
l108: Sentence must finish with a dot.

These issues are very minor, your code is otherwise very nice.

Strange, the Drupal documentation on t() is clear about the use of st() or get_t()...
I would like to have the point of view of someone else on this.
Anyway this is a very minor issue in my opinion.

Just a tiny issue in delete_menu_options.module, but that's because I had to find something:
l22: there is a " character at the end of the sentence, copy paste error :)

All items on the project application checklist looks ok to me, just wait for a reviewer of a "higher rank" to also review your application.

Note that the application process is much more quicker if you take some time to review other applications: Review Bonus

Hello mjgmaas,

Thank you for great module.

1. Please add git link corresponding to non-maintainer like "git clone http://git.drupal.org/sandbox/mjgmaas/1886610.git delete_menu_options"
2. You must use placeholders %d for integer values and %s for string values (in db_query mlid = %s). For example in file: delete_menu_options.module at line: 59
db_query("SELECT mlid, link_path FROM {menu_links} WHERE mlid = %s", $item['mlid'])
Also at line: 95, 131, 141

Cheers!

Closing due to lack of activity. Feel free to reopen if you are still working on this application.

I'm a robot and this is an automated message from Project Applications Scraper.

Adding version to title so D6 reviewers can more easily find it.

Your project page and README have a heading for GitHub but the code is hosted on git.drupal.org instead.

I'm not sure about the get_t() vs st() the docs appear to allow either, so it seems like a very minor issue from ventral.org. Hopefully klausi or patrickd will see this issue and can comment on that test.

If you don't need to pass any arguments though, no need for the empty array. Just drupal_set_message(st("The module 'Delete menu options' has been installed successfully")); is sufficient.

I've never seen a filter_filter() call like that in hook_help() - that's interesting and possibly awesome.

In delete_menu_options_form_alter() use single-quotes for all your strings if possible, see https://drupal.org/coding-standards#quotes. Also, if you just want to affect a single form, consider using hook_form_FORM_ID_alter() instead, which in this case would become delete_menu_options_form_menu_item_delete_form_alter().

Why does _delete_menu_options_get_menu_children() have a recursion limit of 10? Wouldn't you want to delete all children regardless of the nesting level?

I can see where the node delete is useful, but it could also surprise the user. Since you already know which menu item is about to be deleted, perhaps a nice message on that same page listing nodes which would be affected would help. Then the user knows in advance which nodes will be removed.

Those are all minor issues. I do have one security concern though, I don't see any access checks to ensure that the user has permission to delete those nodes. Those are 2 separate permissions and I don't think you should assume that the user has both. Can you add a node_access() check around that? Unfortunately, I think node_access() requires a fully loaded node object.

if (is_numeric($nid)) {
  $node = node_load($nid);
  if (node_access('delete', $node)) {
    node_delete($nid);
  }
}

Setting to needs work for security question only, otherwise this is RTBC from me.

----
Top Shelf Modules - Enterprise modules from the community for the community.

Thanks for those fixes, looks ok to me. This review was based on the D6 version.

----
Top Shelf Modules - Crafted, Curated, Contributed.

I understand that it makes sense to require the "administer nodes" permission to delete all orphaned referenced nodes of a menu entry, but why do you require this permission for deleting all child menu entries? (Not sure whether there's a better permission for this in d6 though)

Anyways, code looks good and has a good amount of inline comments ;)

Thanks for your contribution!

I updated your account so you can 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 stay 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.