What can I do with this module?

  • Create menus from subtrees of existing menus.
  • Clone menu subtrees to an existing menu.

When should I use this module?

Menu Slice is very similar to Menu Block module.

  • Use Menu Block when you want to display a subtree of your menu in a block.
  • Use Menu Slice when you want to separate a subtree into a different menu. You will like it in cases when you have a big navigation menu with thousands of links and you need to create an OG Menu for your organic group with just one subtree of the big menu.

Demo:

http://www.youtube.com/watch?v=lA7ufB6eD3E

Project page:

http://drupal.org/sandbox/asgorobets/1869518

Git

git clone --branch 7.x-1.x http://git.drupal.org/sandbox/asgorobets/1869518.git menu_slice

Drupal 7 version only.

Reviews of other projects:
http://drupal.org/node/1867346#comment-6880854
http://drupal.org/node/1803114#comment-6879878
http://drupal.org/node/1846248#comment-6880164

Comments

vineet.osscube’s picture

Hi,

first of all there are quite a few issues to sort out such as indentation, whitespace.

You can find them all here:
http://ventral.org/pareview/httpgitdrupalorgsandboxasgorobets1869518git

Here you can check source code whether it meets drupal coding standards or not, and advise you what to change in your code. You can repeat review after your commits, and can fix those errors.

Manual Review:

1) Please add README.TXT file which tell about module's description, configuration, help, etc.
2) Use t() function in line no: 110, 113 in your module file.
3) Do not use t() in menu_slice_menu() as it handle by drupal itself.

fr3shw3b’s picture

Status: Needs review » Needs work

Hi,

Like the concept.

Needs work for the minor issues raised by osscube for automated review and to add the README.txt file.

Manual Review:

1. In menu_slice.module you need to add parameter information to the doc blocks
of the custom function _menu_slice_get_options().

2. On line 57 in the module file rename descriotion to description.

asgorobets’s picture

Status: Needs work » Needs review

Hi,

Thank you osscube and fr3shw3b for your quick response and useful reviews.

I made changes you mentioned and the ones related to coding standards and doxygen.
Added README file and also removed master branch.

Thank you for your feedback.

vaibhavjain’s picture

Hello asgorobets,

Just a small advice.
You can use - $form['#redirect'] instead of drupal_goto used in the line 146
This will allow form alter to set or update value.

monymirza’s picture

Status: Needs review » Needs work

You are still missing Coding standards

You can find them all here:
http://ventral.org/pareview/httpgitdrupalorgsandboxasgorobets1869518git

asgorobets’s picture

Status: Needs work » Needs review

Hi,

Thanks vaibhavjain for your advise. I moved my redirect from another function that wasn't a form and missed that point.
Changed that to $form_state['redirect'].

monymirza:
Thanks for pointing this out.
Actually i'm not sure if README.txt line length is actually a major issue since there are no strict standards for README.txt files.
I've fixed line length and did some minor changes to README.txt anyway to improve readability.

Since minor issues were fixed, changing back to "needs review"

asgorobets’s picture

Issue tags: +PAreview: review bonus

Added review bonus tag.

ygerasimov’s picture

I think module is ready to go. Here is some review on the code. More like nitpicking. Maybe you will find some of the advises useful.

Line 82
module_load_include('inc', 'menu', 'menu.admin');

Better to replace with code using $form_state['build_info']['files'][] = drupal_get_path('module', 'menu') . '/menu.admin.inc';

Line 113.

Code

if ($form_state['values']['new']) {
  if (empty($form_state['values']['menu_title'])) {
    form_set_error('menu_title', t("Menu title shouldn't be empty."));
  }
  if (empty($form_state['values']['menu_name'])) {
    form_set_error('menu_name', t("Menu machine-name is required."));
  }
}

Better to change to

if (!isset($form_state['values']['new']) {
  return;
}
if (empty($form_state['values']['menu_title'])) {
  form_set_error('menu_title', t("Menu title shouldn't be empty."));
}
if (empty($form_state['values']['menu_name'])) {
  form_set_error('menu_name', t("Menu machine-name is required."));
}

Line 127

$parent_parts = explode(':', $form_state['values']['parent_link']);
$parent_mlid = $parent_parts[1];

Can be simplified to

list(, $parent_mlid) = explode(':', $form_state['values']['parent_link']);

Line 146

$form_state['redirect'] = 'admin/structure/menu/manage/' . $menu . '/edit';

Please rename $menu to $menu_path and change corresponding lines above where you initiate this variable.

Line 158

if (!isset($menu) || !isset($parent)) {
  return;
}

Is this code required. These are required arguments of the function. Maybe empty() check was in mind.

sonnym’s picture

First off, I must thank you for providing a demo video with this project submission; this really provides a nice overview of the interface your module provides.

I have noticed a few issues with the module I would like to point out, but it looks like everything is actually working as expected.

  • menu_slice_form does not appear to actually implement hook_form (see: hook_form). This method is actually the callback to drupal_get_form as defined in menu_slice_hook_menu.
  • Given that menu_slice_form is actually the callback function, $form_state should be passed by reference, and $form should already be an instantiated array, so there is no need to instantiate it manually.

In the case where you are adding a menu tree to an existing menu, you might want to redirect to a page other than the Edit page for that menu (I found that bit of the video somewhat confusing). So, in menu_slice_form_submit, you could move the redirect into the conditional a few lines above, and go to the links page when augmenting an existing menu. This is just a suggestion, and definitely not necessary!

The logic in menu_slice_perform_slice seems reasonable; I am unaware of any core API functions that provide this functionality, so I think you had no choice but to create it.

Overall, I think this is a nice, clean, and small module that provides some interesting functionality. Best of luck with the review!

asgorobets’s picture

Hi ygerasimov,

Thank you for your useful review during your spare time!! I really appreciate!

It was new for me that form_state['build_info']['files'] can be used, after checking out some contrib modules I decided to use form_load_include to handle module_load_include in form.

I also followed other recommendations and changed $menu to $menu_name, since it's more of machine-name than path. Yes, it's used in redirect, but it's also used as a parameter to menu_slice_perform_slice function.

Regarding

if (!isset($menu) || !isset($parent)) {
  return;
}

It was unnecessary and has been removed.


Hi sonnym,
I was very happy with your comment. First of all I'd like to say that a demo video was something I missed a lot on other project applications to understand how their modules actually work. It was indeed to simplify reviewers life.

I was shocked when find out that hook_form I was using all my Drupal career is not actually a hook in my case. Now I understand why none of contrib modules write in the block comment that they are implementing hook_form and now I know the diference between hook_form and form callback.
It was very useful.
I also wanted to move the redirect to somewhere more handy like List links page. It was just a matter of time. I did redirect to List links when dealing with existing menu, and redirected to Edit Menu when new menu was created, so users can edit menu Description or assign it to organic group or whatever settings they have in this form.
Thank you very much sonnym for your effort on review!

Cheers!

ygerasimov’s picture

Status: Needs review » Reviewed & tested by the community

Should be good to go now.

anwar_max’s picture

Issue tags: -PAreview: review bonus

Everything is looking good to me.

asgorobets’s picture

Issue tags: +PAreview: review bonus, +PAReview: admin mentoring

Probably tags added in #12 should be separated with comma.

klausi’s picture

Status: Reviewed & tested by the community » Postponed (maintainer needs more info)

http://drupal.org/project/menu_clone

This sounds like a feature that should live in the existing menu_clone project. Module duplication and fragmentation is a huge problem on drupal.org and we prefer collaboration over competition. Please open an issue in the menu_clone issue queue to discuss what you need. You should also get in contact with the maintainer(s) to offer your help to move the project forward. If you cannot reach the maintainer(s) please follow the abandoned project process.

If that fails for whatever reason please get back to us and set this back to "needs review".

malc0mn’s picture

@asgorobets: I made you co-maitainer and sent you a message through your profile's contact form. This should definitely be part of menu_clone!

Great work (clean code, standards followed etc.) and what's even more important: open to improvements! Keep up that level of work!

Cheers,

mlc.

asgorobets’s picture

Status: Postponed (maintainer needs more info) » Fixed

The Menu Slice module was integrated into Menu Clone module per klausi's suggestion.
It looks like i'll need another module to go throw review process again.
Changing status to: Fixed.

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

Added links to reviewed applications.