Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Comment #1
vineet.osscube CreditAttribution: vineet.osscube commentedHi,
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.
Comment #2
fr3shw3b CreditAttribution: fr3shw3b commentedHi,
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.
Comment #3
asgorobets CreditAttribution: asgorobets commentedHi,
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.
Comment #4
vaibhavjainHello 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.
Comment #5
monymirzaYou are still missing Coding standards
You can find them all here:
http://ventral.org/pareview/httpgitdrupalorgsandboxasgorobets1869518git
Comment #6
asgorobets CreditAttribution: asgorobets commentedHi,
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"
Comment #7
asgorobets CreditAttribution: asgorobets commentedAdded review bonus tag.
Comment #8
ygerasimov CreditAttribution: ygerasimov commentedI 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
Better to change to
Line 127
Can be simplified to
Line 146
Please rename $menu to $menu_path and change corresponding lines above where you initiate this variable.
Line 158
Is this code required. These are required arguments of the function. Maybe
empty()
check was in mind.Comment #9
sonnym CreditAttribution: sonnym commentedFirst 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 implementhook_form
(see: hook_form). This method is actually the callback todrupal_get_form
as defined inmenu_slice_hook_menu
.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!
Comment #10
asgorobets CreditAttribution: asgorobets commentedHi 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
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!
Comment #11
ygerasimov CreditAttribution: ygerasimov commentedShould be good to go now.
Comment #12
anwar_maxEverything is looking good to me.
Comment #13
asgorobets CreditAttribution: asgorobets commentedProbably tags added in #12 should be separated with comma.
Comment #14
klausihttp://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".
Comment #15
malc0mn CreditAttribution: malc0mn commented@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.
Comment #16
asgorobets CreditAttribution: asgorobets commentedThe 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.
Comment #17.0
(not verified) CreditAttribution: commentedAdded links to reviewed applications.