[D7] Administration menu exclude

Project page
Your project page is very sparse. Please take a moment to make your project page follow tips for a great project page.
So please provide Git details on project page to review your code.

Your git command is in an incorrect format.

It should be something like this:

git clone --branch 7.x-1.x http://git.drupal.org/sandbox/sguardiola/2299429.git 

Automated report from pareview.sh

FILE: /var/www/drupal-7-pareview/pareview_temp/admin_menu_exclude.module
--------------------------------------------------------------------------------
FOUND 11 ERRORS AND 2 WARNINGS AFFECTING 9 LINES
--------------------------------------------------------------------------------
2 | ERROR | [x] Additional whitespace found at start of file
25 | WARNING | [ ] A comma should follow the last multiline array item. Found:
| | )
26 | WARNING | [ ] A comma should follow the last multiline array item. Found:
| | )
32 | ERROR | [ ] Doc comment short description must end with a full stop
33 | ERROR | [ ] There must be exactly one blank line before the tags in a
| | doc comment
33 | ERROR | [ ] Missing parameter comment
33 | ERROR | [ ] Missing parameter type
34 | ERROR | [ ] Missing parameter comment
34 | ERROR | [ ] Missing parameter type
52 | ERROR | [x] TRUE, FALSE and NULL must be uppercase; expected "FALSE"
| | but found "false"
53 | ERROR | [x] Space found before comma in function call
53 | ERROR | [x] TRUE, FALSE and NULL must be uppercase; expected "FALSE"
| | but found "false"
65 | ERROR | [ ] Files must end in a single new line character
--------------------------------------------------------------------------------
PHPCBF CAN FIX THE 4 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------

In admin_form_exclude.module:

I am not sure what to make of including a form element directly within another form element, as you do in

$form['exclude'] = array(
    '#type' => 'fieldset',
    '#title' => t('Exclude'),
    '#group' => 'advanced',
    'paths' => array( <====
      '#type' => 'textarea',
      '#title' => t('Exclude paths from loading administration menu'),
      '#default_value' => variable_get('admin_menu_exclude_paths', ''),
      '#description' =>
        t('Add a line for each url you want to exclude.
        You can use wildcards at the end of the url. Example: user/*')
    )
  );

I don't recall ever seeing it before, though I do not know for certain it is 'wrong'.

admin_menu_exclude_init() does not implement hook_boot().

Also note the general rule about 120+ lines of code and 5 functions.

It doesn't matter much to me, and I don't know for a fact that it's wrong, but I would do this:

$form['exclude'] = array(
    '#type' => 'fieldset',
    '#title' => t('Exclude'),
    '#group' => 'advanced',
);
$form['exclude']['paths'] = array(
   #type' => 'textarea',
      '#title' => t('Exclude paths from loading administration menu'),
      '#default_value' => variable_get('admin_menu_exclude_paths', ''),
      '#description' =>
        t('Add a line for each url you want to exclude.  You can use wildcards at the end of the url. Example: user/*'),
);

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

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

Hi Sergio,

I have reviewed the module and I did not find any problem so far. It is useful when using Administration menu. Maybe in the future you could add some token functionality for complex menus.

Cheers.

Hello guardiola86,

I reviewed your module,

Module is not working for me, may be i configured it wrong as your project's readme file i follow everything but i don't know what i am doing wrong

Here is one point regarding with your module code

1. Here is your code

      if (
        strpos($path, '*') !== FALSE &&
        strpos($current_path, str_replace('*', '', $path)) !== FALSE
      ) {
        $hide = TRUE;
      }
      elseif ($path == $current_path) {
        $hide = TRUE;
      }

You can use break statement to stop foreach loop so that if $hide variable get its value there is no need to iterative the code further

for example

      if (
        strpos($path, '*') !== FALSE &&
        strpos($current_path, str_replace('*', '', $path)) !== FALSE
      ) {
        $hide = TRUE;
        break;
      }
      elseif ($path == $current_path) {
        $hide = TRUE;
        break;
      }

2. You are using variable_set('admin_menu_exclude_paths', $form_state['values']['paths']);
in your submit function so you should write a code to delete this variable from database when module is uninstall.

Your git command is not in the correct format.
It should be like this : git clone --branch 7.x-1.x http://git.drupal.org/sandbox/sguardiola/2299429.git admin_menu_exclude

Good module. Most of what I have are small suggestions. I think hook_help() should be implemented before this gets approved.

PAReview

There were no code issues from pareview.sh but it did say you did not have a default branch set.

http://pareview.sh/pareview/httpgitdrupalorgsandboxsguardiola2299429git

Documentation

• The project page is very sparse. I would suggest at least copying over what you have from the first post of this page to your project page.
• Readme.txt does not follow the readme template but at first glance it seems to have enough there guide a first time user on how to get started.
• There is no help page. You should Implement hook_help() as per https://www.drupal.org/node/161085#hook_help

Repository

There is a version specific branch. You may want to consider looking at how commit messages are formatted in Drupal so you can get into that habit.

Other Suggestions

• You may want to look at the function drupal_match_path which will make your hook_init implementation a little easier.

@guardiola86, thankyou for your work.

Automated Review

Best practice issues identified by pareview.sh / drupalcs / coder. None.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

(*) Maybe: Does not cause module duplication and fragmentation. Can you follow the steps in the link and confirm that the functionality contained in this module should stay separate from Administration menu? Could you open an issue to Administration menu and confirm that they don't have plans to incorporate similar functionality any time soon?.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements

3rd party code

Yes: Follows the guidelines for 3rd party code.

README.txt/README.md

Yes: Follows the guidelines for in-project documentation and the README Template.

Code long/complex enough for review

No: Follows the guidelines for project length and complexity. Adding PAReview: Single project promote tag.

Secure code

Yes. If "no", list security issues identified.

Coding style & Drupal API usage

1. (+) admin_menu_exclude_form_admin_menu_theme_settings_alter: In this form why are you not using system_settings_form($form) instead of calling your custom submit function, due to this extra .install file also created. Better to use this like following code.
   

   function admin_menu_exclude_form_admin_menu_theme_settings_alter(&$form, &$form_state, $form_id) {
     $form['exclude'] = array(
       '#type' => 'fieldset',
       '#title' => t('Exclude'),
       '#group' => 'advanced',
     );
     $form['exclude']['admin_menu_exclude_paths'] = array(
       '#type' => 'textarea',
       '#title' => t('Exclude paths from loading administration menu'),
       '#default_value' => variable_get('admin_menu_exclude_paths', ''),
       '#description' => t('Add a line for each url you want to exclude.  You can use wildcards at the end of the url. Example: user/*'),
     );
     
     return system_settings_form($form);
   }
   

2. I tested your module functionality for one node where nid is 4 and url alias is test-node. If I add test-node under Exclude paths then it works as intended but If I add node/4 then it doesn't work, can't we implement this for both cases as other drupal modules also work in similar way.
3. I agree with geekygnr, it would be better to use drupal_match_path in admin_menu_exclude_init() function.

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

Thanks again for your contribution!

My blocking issues from #34 have been addressed.

I am not seeing any additional blocking issues, so I am setting RTBC. As I am not a git admin, so it needs a second set of eyes from a review admin.

Thank you for your patience and hard work guardiola86!

Hi guardiola86,

Having an admin review it to get the project promoted can take some time. I saw that you have two manual reviews done. One more and you can add the PAReview: review bonus tag. Two days after I added that tag my module was looked at.

Automated Review

Review of the 7.x-1.x branch (commit 1a94883):

• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

Manual Review

admin_menu_exclude_help() has a translatable string broken across lines, a single t() would be good enough here.

Else looks good to me, not seeing any blocking issues.

There is no objection for more than 3 weeks, so i think it is fine to approve now.

Thanks for your contribution, guardiola86!

I promoted this project for you: https://www.drupal.org/project/admin_menu_exclude

Now this experimental project has been promoted, you'll need to update the URL of your remote repository or reclone it.

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.