[D7] Extra Tabs Menu

Hi Ajay and Ayesh, thanks both for the reviews.

ajayNimbolkar:

(*)I have follow the steps as given in redme.txt file and created menu as below
* Tab set 1 test 1 => node/552142
* Tab set 1 test 2 => node/552143
* Tab set 1 test 3 => node/552144
While visting "node/552142" page then other tab such as "test2" and "test3" not visible.
please guide if I did any thing wrong.

I'm curious, did you set the tabs up as branches in the menu tree (with "tab set 1 test 2" and "Tab set 1 test 3" nested under "Tab set 1 test 1"? I've attached a screenshot of how the tabs menu links should be ordered.

drupal_valid_path(), needed dynamic_allowed variable in line no 38, you have provided path but you need to set dynamic_allowed to "TRUE" because if user added path like "test/*/*" then drupal_valid_path() return false for that we need 2nd parameter.

It turns out that line is unnecessary, see my response to Ayesh's feedback below :-)

(*) 'title' => $menu_link['link_title'], line no 42 in extra_tabs_menu.module used t() function.

Sorry, I'm not seeing any spots where I'm running menu['link_title'] through t(), and line 42 is actually 'localized_options' => $menu_link['localized_options'], maybe you got the line numbers a bit confused?

Thanks for the review!

Ayesh:

May I ask why you needed drupal_valid_path() (in line 36) in first place? Isn't it the menu admin form's job to validate the paths?

Sure. First of all, it turns out that it was unnecessary, you're correct. I was originally using it because drupal_valid_path() handles validating menu paths as well as determining whether a user has access to the path. I was using it for the access check, not thinking that menu_tree_get_page_data() already filtered out links that the current user does not have access to. I've updated the codebase and removed that check, since it was unnecessary.

Thanks for the review!

@ajayNimbolkar:
Thanks for the quick response.

Please see below code where you have used 'title' => $menu_link['link_title'],

$data['tabs'][0]['output'][] = array(
'#theme' => 'menu_local_task',
'#link' => array(
'title' => $menu_link['link_title'],
'href' => $menu_link['link_path'],
'localized_options' => $menu_link['localized_options'],
),
);
So please used t function for title its used for translation.

I think I have to disagree here. The t() function is not meant to have user-entered text run through it, as stated in the section on translating variables in function's documentation. The function theme_menu_local_task takes care of running the link title through check_plain as well (if required), so in my opinion it's fine to use the raw value here.

I have set the order of menu as per the screen shot provided by you but also while visiting parent menu it was not showing child menu as "tab" in parent page.

Would it be possible for you to try to help with debugging this? Or finding the origin of the problem and noting it in this issue? I'd really appreciate it, since it's preventing the module from doing it's sole task, and since it doesn't sound like Ayesh ran into this, and I'm unable to replicate the issue as well.

Thanks!

Hi rgkimball, thanks for the review.

I think the major limitation of the current implementation is that you can only apply a set of links to a specific node/path, rather than a section of the site or a content type (as many use cases will call for).

I definitely see your point. This project is the result of a specific use case on a client project where that flexibility wasn't needed, but for this to be useful to the community at large it seems like it's flexibility should be expanded a bit. I've got some ideas for how we can still leverage menus to add these extra tabs, but not be tied to a specific menu and its menu links for controlling visibility.

Personally, I would like to see this as a context reaction to enable this type of configuration, but I am interested to hear what other people have to say on that point.

What probably makes the most sense is for the system that determines whether the tabs get altered or not to be pluggable, so the module can provide a basic configuration form, but can also leverage context reactions or possibly some other tool without having to get too crazy.

The only item that caught my attention was the constant defined at line 7 of your .module, using const instead of define(), per https://www.drupal.org/coding-standards#naming

Noted, it was something that slipped my mind when I wrote this. I don't imagine that I'll be using the constant in the future but if it does exist for some reason I'll switch it.

I'm going to drop this issue down to "needs work", and will be making some adjustments to how the additional tab visibility gets controlled and will bump it back to needs review when those changes are incorporated.

Thanks again for the feedback!

Hi all, thanks again for reviewing the module earlier. I've taken @rgkimball's feedback and made significant changes to how the module functions. I think the changes make things much more usable and flexible for site administrators and developers.

I've updated the project page, and the project application description with the changes so details can be seen there, but the gist of it is:

• The extra_tabs_menu module provides a settings form where a user can use block-style path visibility settings to determine which pages a set of tabs will display on, and the user can choose a menu parent whose links they'd like to have displayed on the configured pages
• A submodule has been added that adds a context reaction that allows an admin to choose a menu parent whose links will appear as tabs if the context conditions are met.
• A hook is added that allows developers to add their own mechanisms for determining when and a menu parent's links should be added as tabs.

If any of you have some time to check this out again (or new reviewers) I'd really appreciate it!

Thanks!

Thanks for the review @daria.a!

FILE: ...iew_temp/extra_tabs_menu_context/plugins/extra_tabs_menu_reaction.inc
---------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
---------------------------------------------------------------------------
14 | ERROR | Public method name
| | "ExtraTabsMenuContextReaction::options_form" is not in
| | lowerCamel format

There's not really anything that we can do about that. Since this is a context reaction extending context_reaction, that method name's already defined for us using snake_case rather than lowerCamel.

1. Move your admin page-related code to admin.inc file from your .module file.

Seems valid to me! The leaner that the .module can be the better. I've moved the admin form related code into an admin.inc file.

2. It's desirable to display a confirmation message (with drupal_set_message), that module's settings were successfully altered. That confirmation should be displayed right after user have clicked "Save" button.

Also totally valid. I've added a simple message saying that "Extra tabs menu configuration has been updated." when the user saves the form.

jiff, I udated status.

Thanks daria.a!

hi, one this is missing:
hook_install(). It is recommended to always implement. Please check an example here:

Thanks for the feedback ashish.verma85. I've Implemented hook_install, as well as hook_uninstall since there was a configuration variable that wasn't getting removed.