[D7] Thundersearch

Hi,

Check and fix this errors http://pareview.sh/pareview/httpgitdrupalorgsandboximgio2164731git

Hello imgio,
Thanks for the work on the module. Such work adds to the making the Drupal ecosystem more robust and a compelling option as a platform.

I would like to suggest something: your .module file is quite large (238 KB) and it is because all your functions are in it. This large size could have a performance cost as the entire .module file is loaded whenever a page is requested.

I think it would be a good idea to have only your hook functions in the .module all other functions can be placed in an .inc file.

This same idea is also applicable to your menu hook: instead of placing the menu callback functions in the .module file you can place them in the .inc and reference them via the 'file' element.

For example, instead of

$items['thundersearch'] = array(
    'title' => 'Thunder Search',
    'page callback' => 'drupal_get_form',
    'page arguments' => array('thundersearch_form'),
    'access arguments' => array('thundersearch content'),
);

You can have:

$items['thundersearch'] = array(
    'title' => 'Thunder Search',
    'page callback' => 'drupal_get_form',
    'page arguments' => array('thundersearch_form'),
    'access arguments' => array('thundersearch content'),
    'file' => 'thundersearch.inc',
);

In this example, the function thundersearch_form() will not be in thundersearch.module instead it will be in thundersearch.inc

You may consult https://api.drupal.org/api/drupal/modules!system!system.api.php/function/hook_menu/7 for further details

All the best,
Feyisayo

Review:

• Please add comment lines in between code lines.
• A saw by accident that Belgum should be written as Belgium and Netherland => Netherlands. (Line 659 and 660 of thundersearch.module)
• thundersearch.module line 548: Implements thundersearch_getMap() => Custom functions should have an explaining comment block containing @param and @return information. The same is true for other custom function functions. The 'Implements ...' comment is used for existing hooks.
• thundersearch.module line 223: Try to prepend the module name to the variable name 'server_url', so there are no conflicts with other modules.

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 imigio,

I notice in the 'thundersearch.module' file where you implement hook_menu you define a new page past /admin/config/.
Whenever creating a admin config page it is best to include the config page in your .info file; so for example, in this case i recommend adding this to your .info file:

configure = admin/config/search/thundersearch

When a config page is defined in the .info file it allows that config page to be accessed straight from the modules list in the admin drupal interface.

It is also recommended to put anything with hook_admin_* in a module_name.admin.inc file. So to fix this all you would need to do is:

  $items['admin/config/search/thundersearch'] = array(
    'title' => 'Thundersearch settings',
    'description' => 'Configure relevance settings for thundersearch.',
    'page callback' => 'drupal_get_form',
    'page arguments' => array('thundersearch_admin_settings'),
    'access arguments' => array('administer search'),
    'file' => 'thundersearch.admin.inc', // Add this line
  );

Then move all the hook_admin_* functions into this file and as long as it is first being referenced in the .module file all your code will still be accessible.

Hi,

It is recommended to place all images under images folder. In your module images (us.png etc ) are under root folder.

• Maybe it's not the best way to use error_reporting(E_ALL & ~E_NOTICE); in .module file
• Use module_load_include instead of include_once please
• Just question (I don't know). DRUPAL_NO_CACHE (searchform, searchmap and ussearchmap) is used in all three blocks. Is it necessary to use it in searchform?
  

        switch ($delta) {
        case 'searchform':
          $block['content'] = drupal_get_form('thundersearch_form', $_SESSION['squery']);
          break;
      

• I suggest you to use templates .tpl.php instead of functions thundersearch_getMap and thundersearch_get_us_map

Best regards

Removing review bonus tag, you have not done all manual reviews, you just repeated the output of an automated review tool. Make sure to read through the source code of the other projects, as requested on the review bonus page.

Some small issues reported in coder , please fix them. Rest all looks fine.

thundersearch.inc

severity: normalreview: style_string_spacingLine 166: String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms [style_string_spacing]
$img_path = $url . '/' . drupal_get_path('module', 'thundersearch');

Line 276: String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms [style_string_spacing]
$pagign_tbl .= '<TD WIDTH="100" ALIGN="RIGHT" style="text-align:right;"><a href="' . thundersearch_parseURL($tdata['table']['tr']['td'][2]['a']['href']) . '"><IMG SRC="' . $urls['img_path'] . '/next.gif" HSPACE="0" VSPACE="0" BORDER="0"></a></TD>';

Line 289: Remove PHP debugging function calls. [production_php]
echo "<pre>";print_r($arr);echo "</pre>";

I am going to the module's admin setting and I get PHP error message

Warning: require_once(/var/www/thundersearch/sites/all/modules/thundersearch/thundersearch.admin.inc): failed to open stream: No such file or directory in menu_execute_active_handler() (line 515 of /var/www/thundersearch/includes/menu.inc).

Looks like you need to update hook_menu()

@imgio, first of all thankyou for your contribution.

IMHO, you should review some module and get review bonus tag.

Manual Review:

I did not reviewed your whole module code, just check configuration form and its code (thundersearch.admin.inc).

1. As per Drupal naming convention, you are missing $form_state variable

function thundersearch_admin_settings($form) {

It should be:

function thundersearch_admin_settings($form, &$form_state) {

2. It looks silly, you are using system_settings_form() and also defining submit function with this.

$form['#validate'][] = 'thundersearch_admin_settings_validate';
$form['#submit'][] = 'thundersearch_admin_settings_submit';
return system_settings_form($form);

I think it should be like this if you are managing something different in your submit function that system_settings_form() don't do.

$form['#validate'][] = 'thundersearch_admin_settings_validate';
$form['#submit'][] = 'thundersearch_admin_settings_submit';
return $form;

3. From your validation function, it seems you are just validating whether field value is empty or not. Even that is also not correct, if you just enter space in field, it get saved because no trim function is used in following code.

if ($form_state['values']['thundersearch_pr'] == "") {

Then why are you not adding just '#required' => TRUE, to get it done.

4. Your submit function looks wired, not sure what you are trying to do with this extra code:

function thundersearch_admin_settings_submit($form, &$form_state) {
  if (variable_get('thundersearch_pr') != $form_state['values']['thundersearch_pr']) {
    drupal_set_message(t('PR value, the Search Index name, has been stored.'));
    variable_set('thundersearch_pr', $form_state['values']['thundersearch_pr']);
  }
  else {
    drupal_set_message(t('PR value, the Search Index name, has been updated.'));
  }

  if (variable_get('thundersearch_server_url') != $form_state['values']['thundersearch_server_url']) {
    drupal_set_message(t('The Server URL has been stored.'));
    variable_set('thundersearch_server_url', $form_state['values']['thundersearch_server_url']);
  }
  else {
    drupal_set_message(t('The Server URL has been updated.'));
  }
}

You yourself make this form complicated, it can be easily achieved using system_settings_form() and there is no need to define validation and submit function. Due to some bug in your submit function, if you try to save some dummy value in URL of Thunderstone server field, it reverted with default value (http://thundersearch.eafoundation.com) after save every time.

Before Submit:



After Submit:

Instead of writing this whole code, I would simply create this configuration form in following way:

/**
 * @file
 * Includes admin functions for thundersearch.
 */

/**
 * Implements hook_admin_settings().
 */
function thundersearch_admin_settings($form) {
  // Collect some stats!
  $form['thundersearch_pr'] = array(
    '#type' => 'textfield',
    '#title' => t('Search Index Name, which is the PR value in URL string for Thundersearch'),
    '#default_value' => variable_get('thundersearch_pr'),
    '#description' => t('Enter the Thunderstone appliance search index name here. IMPORTANT: Upper/lower case is significant.'),
    '#required' => TRUE,
  );

  // Gio's addition.
  $form['thundersearch_server_url'] = array(
    '#type' => 'textfield',
    '#title' => t('URL of Thunderstone server'),
    '#default_value' => variable_get('thundersearch_server_url', "http://thundersearch.eafoundation.com"),
    '#description' => t('Enter the URL of your Thunderstone search engine.'),
    '#required' => TRUE,  
  );

  return system_settings_form($form);
}

Also all reviewers 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).

@imgio, thanks again for your hard work!

Ohh, forget to change this to Need Work.

Automated Review

No issues reported by PAReview. Good job!

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and/or fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements. While I think it is fine, could you comment here about the images you have used for the maps and navigation.

3rd party assets/code

Yes: Follows the guidelines for 3rd party assets/code.

README.txt/README.md

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

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes: Meets the security requirements.

Coding style & Drupal API usage

1. (+) Looks like an useful module. However, I wasn't able to get it to work because of the lack of the documentation (or link to documentation) on what Thunderstone service is about. Maybe you should have a help link on the configuration form and link it to the Thunderstone documentation page. I was not able to find the documentation easily and I didn't spend much time searching for it. Sample code where the link could be added to the documentation form:
   

     $form['thundersearch_server_url'] = array(
       '#type' => 'textfield',
       '#title' => t('URL of Thunderstone server'),
       '#default_value' => variable_get('thundersearch_server_url'),
       '#description' => t('Enter the URL of your Thunderstone search engine. See !url for the official documentation.', array('!url' => l(t('this link') => 'http://thunderstone-documentation-link'))),
       '#required' => TRUE,
     );
   

2. (*) Get the following notices which should be taken care of before creating the release. To reproduce, click on any state in the US map, or the world map:
   

   Notice: Undefined variable: block in thundersearch_block_view() (line 121 of /Users/apple/www/contrib/sites/all/modules/sandbox/thundersearch/thundersearch.module).
   Notice: Undefined variable: block in thundersearch_block_view() (line 121 of /Users/apple/www/contrib/sites/all/modules/sandbox/thundersearch/thundersearch.module).
   

3. In the function thundersearch_form(), consider adding JS using the #attached attribute of the FAPI. Just a recommendation.
4. (*)The comment style of the function you are using is the same throughout the module:
   

   /**
    * Function function_name().
    */
   

   It is recommended to write small description of what the function actually does, unless it is a hook implementation.

5. (*) I added in a term in a term in the Thundersearch Terms vocabulary, It caused empty values on the search form select list. See screenshot.
   
6. I see that the helper functions are in mixed case. E.g. thundersearch_getKeyword, thundersearch_getJump, thundersearch_getOrderBy, etc. I think for consistency you should use the function name separated with underscores for the function name in Drupal 7, unless it belongs to a class. Not sure if this is a blocker though.

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.

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This review uses the Project Application Review Template.

This application has been in review for a long time in "Needs Review" state. All the reviews that you get are from the free time and effort put in by volunteers. I'd recommend you to participate in Review Bonus program, so that the GIT admins could have a look at it right away!

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

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