Likno JavaScript/CSS Menus

Hello, and thanks for applying for a CVS account. I am adding the review tags, and some volunteers will review the code, pointing out what it needs to be changed.

This looks VERY interesting. Will it be free to use or?

• This is a partial review.
• The points reported in this review are not in order of importance / relevance.
• Most of the times I report the code that present an issue. In such cases, the same error can be present in other parts of the code; the fact I don't report the same issue more than once doesn't mean the same issue is not present in different places.
• Not all the reported points are application blockers; some of the points I report are simple suggestions to who applies for a CVS account. For a list of what is considered a blocker for the application approval, see CVS applications review, what to expect. Keep in mind the list is still under construction, and can be changed to adapt it to what has been found out during code review, or to make the list clearer to who applies for a CVS account.

1. License files cannot be committed in Drupal.org repository. Projects committed in Drupal.org repository have the same license used by Drupal.
2. Files available from third-party sites should not be committed in Drupal.org repository. Files licensed under a license different from GPL License (even if it is a compatible license) should not be committed in Drupal.org repositories.
3. The version line needs to be removed from the .info file.

4. function likno_menus_uninstall() {
       db_query("DELETE FROM variable WHERE SUBSTR(name,1,4) = 'AWM_'");
   }
   
   

   Deleting Drupal variables using a query that matches any Drupal variable with a name that starts with the module name would remove also the Drupal variables of other modules.

5. See http://drupal.org/coding-standards to understand how a module should be written. In particular, see hoe Drupal variables, global variables, constants, and functions defined from the module should be named; how the code should be formatted.

6.       '#description' => t('The menu you want to be displayed using Superfish. Defined at '. l('admin/settings/likno_menus', 'admin/settings/likno_menus') .' under <i>'.variable_get('AWM_menu_name_'. $awm_delta_this, 'menu'. $awm_delta_this).'</i>.'),
         '#value' => '<input value="'.$awm_which_menu.'" maxlength="128" name="superfish_name_1" id="edit-superfish-name-1" size="60" value="Superfish 1" class="form-text" type="text" readonly>',
   
   

   l() should not be used together with t(). See the documentation for t(), where this code is reported to be wrong:

   $output .= t('<p>Go to the @contact-page.</p>', array('@contact-page' => l(t('contact page'), 'contact')));
   

   The correct code to use is the following:

   $output .= '<p>'. t('Go to the <a href="@contact-page">contact page</a>.', array('@contact-page' => url('contact'))) .'</p>';
   

   Form elements must be generated through the form API.

2. the only "files available from third-party sites" submitted were some necessary .js and .css files from the superfish module, that need to be loaded only if the superfish module is not available; ALL files are licensed under the GPL license.

What I reported is still valid.
There are at least two reasons for which files available from third-party sites should not committed in drupal.org repository; the first I can think of is to avoid to have multiple copies of those files in drupal.org repository, and in Drupal-powered sites.

Maybe you can ask the user to install those files in sites/libraries

In this way, any module that require that files can search for them in that location.

Then you can make some checks in the code to know if the files are there, and if they aren't you can show an error message to the user requesting the installation of the files in that folder.

What juankvillegas reports is correct; to be exact, the module should implement hook_requirements() and verify the required files are present.

Hi. Please read all the following and the links provided as this is very important information about your CVS Application:

Drupal.org has moved from CVS to Git! This is a very significant change for the Drupal community and for your application. Please read the following documentation on how this affects and benefits you and the application process:
Migrating from CVS Applications to (Git) Full Project Applications

• The status of this application will be put to "postponed" and by following the instructions in the above link, you will be able to reopen it.
• Or if your application has been "needs work" for more than 5 weeks, your application will be marked as "closed (won't fix)". You can still reopen it, by reading the instructions above.

Happened upon this issue by accident, moving this over the correct place, the CVS Application queue is no longer active.

Changing priority.

Also, this module application is here since september 6th of last year.

After clarification with tim.plunkett, changing priority according to the new priority guidelines.

I got the following error when trying to extract the required zip file:

Archive: /drupal6/sites/all/modules/likno_javascript_css_menus/superfish_js_css_for_likno.zip
[/drupal6/sites/all/modules/likno_javascript_css_menus/superfish_js_css_for_likno.zip]
  End-of-central-directory signature not found.  Either this file is not
  a zipfile, or it constitutes one disk of a multi-part archive.  In the
  latter case the central directory and zipfile comment will be found on
  the last disk(s) of this archive.
zipinfo:  cannot find zipfile directory in one of /drupal6/sites/all/modules/likno_javascript_css_menus/superfish_js_css_for_likno.zip or
         /drupal6/sites/all/modules/likno_javascript_css_menus/superfish_js_css_for_likno.zip.zip, and cannot find /drupal6/sites/all/modules/likno_javascript_css_menus/superfish_js_css_for_likno.zip.ZIP, period.

I'm on Ubuntu and used whatever the default zip option is on the contextual menu.

Maybe provide a tar.gz for those not on windows?

mrf the code is in the git repository here
http://drupal.org/sandbox/aaskee/1093394

I started to look through the code and was writing a coding standards patch. I got halfway through the .install and then I opened .admin.inc, and just stopped.

There are huge chunks of untranslated strings, and HTML hardcoded everywhere. Please look over http://drupal.org/coding-standards and run your code through http://drupal.org/project/coder.

Also, why is there so much styling done in JS? Why is it not in a CSS file?

Once the code is more readable, I can give it a real review.

The applicant has been contacted to ask if the application is abandoned.

The application has been closed. If you would like to reopen it, you are free to do so.
See http://drupal.org/node/894256