[D7] Bear Responsive Menu

There are some errors reported by automated review tools, did you already check them? See http://ventral.org/pareview/httpgitdrupalorgsandboxalioso_oopa2034049git

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 @alioso,

I have the following comments after the review:

1. Check the message generate from automated robot above. You have to fix some coding standards in your code.

2. Defined var without using it:

$name = 'responsive_menu';
....

Change to this or remove it:

$name = 'responsive_menu';
....
drupal_add_js(array($name=> $settings), 
array('type' => 'setting', 'scope' => JS_DEFAULT));

3. Assing drupal_get_path('module', 'responsive_menu') to a variable and after that use this variable to the drupal_add_js functions.

4. Remove module_load_include('inc', 'responsive_menu', 'responsive_menu_common'); from admin.inc file.
The .module file is always included, so, no need of this line. In fact if you are planning only to have defines in common.inc file it is save to add it to .module file and not to move it to a separate file. It will be more easy for reading your code and less overhead - no including of new files.

Hello @alioso

my manual review:

.module line 8

When you include a file of your own module you can use require_ince instead module_load_include(). You must use module_load_include to load files of another module because you don't know the route of the file and module_load_include will find it. If you include a file of your own module you can use require_once:

require_once realpath(__DIR__) . '/responsive_menu_common.inc';

.module line 16

$output .= t(RNJS_MAIN_JS . ' plugin for a nice responsive Drupal menu.');

The $output is not declared before so:

$output = t(RNJS_MAIN_JS . ' plugin for a nice responsive Drupal menu.');

And use placeholders to include the "dynamic" text at t()

.module line 17

I think that you must use return $output; instead break;

.module line 57

You implements hook_library() not add_js()

.install hook_uninstall()

You can use variable_del() instead db_delete() to remove the variables of your module.

Hello.

I have to correct one of the points of my review. Sorry. In the review process of a module of mine told me that instead utilizase module_load_include required, so I adopted that point as a correction to make.

My module has passed the review process, but the person who enabled the publication of the project (cweagans) commented as follows on the subject:

For this bit:

 389 /**
 390  * include the block functions file
 391  */
 392 require_once realpath(__DIR__) . '/pay_with_a_tweet.blocks.inc';
 393 
 394 /**
 395  * include the tokens functions file
 396  */
 397 require_once realpath(__DIR__) . '/pay_with_a_tweet.tokens.inc';

1) To load module include files, you should use module_load_include; and
2) If you're including this unconditionally, there's no reason to split them out into another file. It is, in fact, a bit slower, since you're adding a couple of disk reads/file open operations into the mix. You could probably just copy the contents of those files into your .module file and you'd be fine.

I understand that, in this case, it is better to follow the recommendation of a person who can enable publishing projects.

Assigning to myself for review.

I recommend using a version specific branch instead of a master branch. Master branch does not tell us which version of Drupal is required. See here for more details.

The .info file says the name of the module is 'Responsive Menu', but there is a project named Responsive Menus. The name on the application and the project page is 'Bear Responsive Menu'. Some consistency would be nice.

I'm not sure if meanmenu.css and jquery.meanmenu.2.0.min.js would be considered 3rd party or not. They may be found here.

I also would not use a query to retrieve a list of variables to delete in hook_uninstall. I would use an explicit list of variable names instead.

Could the admin field 'responsive_menu_position' be a select instead of a textfield?

In hook_enable, you may want to use the l() function or the url() function instead of formatting the link tag explicitly.

The admin settings allow the user to enter text, but check_plain is not used. These settings are passed directly to the javascript in Drupal.settings. Could this be a vulnerability? See here for more information about writing secure code.

Repository contains a file named README.md. Why is this file needed?

Performed review based on checklist found here.

1. Basic application checks
1.1 Application contains a link to the project page. The application also contains a repository, but not a drupal repository. The project page contains a working git clone command for a drupal repository. I suggest copying the git clone command from the project page to the application to avoid confusion.
1.2 Duplicates of this functionality could not be found. I could not find another project that already implements the responsive menus without media queries and having expandable children.
1.3 User does not have duplicate applications.

2. Basic repository checks
2.1 Repository contains code.
2.2 Repository does NOT contain a version specific branch. Please migrate the master branch into the appropriate version branch.
2.3 Repository contains at least 120 lines of code and 5 functions.

3. Security Review
3.1 Admin settings allow the user to enter text, but I do not see any use of check_plain. See here for more information about writing secure code.

4. Licensing checks
4.1 Repository does not contain a LICENSE.txt file.
4.2 Repository MAY contain 3rd party code. I'm not sure if meanmenu.css or jquery.meanmenu.2.0.min.js would be considered 3rd party or not. Perhaps documentation should clarify that these files should be included as a library or something instead of including them in the repository. I'm not sure.

5. Documentation checks
5.1 Project page contains a minimum of detailed information. The screenshot is not very helpful. The page should mention the installation instructions in INSTALL.txt and the requirements for the module. Perhaps the page could contain the same information as the README.txt file in the repository. See here for tips on creating a great project page.
5.2 Repository contains a README.txt file, but it could contain more details. Link to the project page? People should be able to determine whether or not this project will be helpful to them by reading the README.txt file. Also, the repository contains a file named README.md. This file does not appear to be necessary. Why is this file needed?
5.3 Code contains helpful inline comments.

6. Coding standards and style
6.1 I ran an automated review at ventral.org http://ventral.org/pareview. The review returned errors but most of them pertain to code structure (space before and after '=', unused variables, etc...) but nothing serious. The automated review also mentions the master branch issue.

7. API and best practices review.
7.1 Drupal APIs are used correctly for the most part. The hook_uninstall() function still uses db_delete instead of variable_del. An explicit list of variables to delete should also be used instead of a query. Also, l() or url() should be used instead of formatting the link tags explicitly.

Closing due to lack of activity. Feel free to reopen if you are still working on this application (see also the project application workflow).

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