[D7] Biological Taxonomy

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,
Some points before i review this further:
1) You have used get_t() in hook_menu generaly this is used in hook_schema , hook_requirement or hook_update as per the database access standard.Please make sure you require it.

2)Read you project page still it is recommended to use hook_help to make users understand your module.
3)A documentation of variables used in below function would be good:
indices.inc

function _biological_taxonomy_index_add($table_name, $index_name, array $columns, &$context) {

Hello,
I'm revising your module but I cannot manage to do it work on a clean drupal installation as I get it from GIT.

It seems there are a couple of mistake:

file "biological_taxonomy.module", line 19
'page callback' is invalid because you do not specify the 'file' the callback function is in. I think you should ad this code line
'file' => 'system.admin.inc',
In the $items['admin/config/biological_taxonomy'] array definition.

file "Includes/batch/indices.inc" line 45
A function named '_bt_index_add' is not defined and cannot be declared as a callback_batch_operation() implementation. I think the line should be:
'_biological_taxonomy_index_add',

Hi JacobSanford,
* 1. There are certain unnecessary variables in *.install file. $itis_download_url, $itis_donwload_files, $itis_target_html which have no such specific purpose.You have only used them once, so these can directly be replaced in-place.
+ 2. Do not use hardcoded path as you have done in $itis_target_html = '/sites/all/libraries/itis/';. Users should not be bound to keep the files in sites/all only. Use libraries_get_path() instead. Same as the case with this variable_set('biological_taxonomy_sqlite_path', DRUPAL_ROOT . '/sites/all/libraries/itis/ITIS.sqlite'); Also change it accordingly in other files as well.
* 3. Extra blank line(line 53) in *.module file

'files' => array(
      'js' => array(
        'ITIS.sqlite',
      ),
    ),

  );

* 4. In your project page as well as README.md, please include the line that the users require PDO SQLite (3.x or above) for the module to run correctly. As the normal PHP installation does not have this requirement pre-installed.
+ 5. It is highly discouraged to use direct database instructions as you have done in db.inc.
Sample snippet from your code $get_kingdoms_query = "SELECT tsn, complete_name FROM taxonomic_units WHERE parent_tsn=0 AND (n_usage='accepted' OR n_usage='valid')";
Use db_query() instead.
+ 6. There has been multiple usage of the code drupal_get_path('module', 'biological_taxonomy') in indices.inc
Save it in a constance instead and use it all places. For eg: define(BIOLOGICAL_TAXONOMY, drupal_get_path('module', 'biological_taxonomy'));

* marked are petty problems, but + marked are fairly good issues with your project. Have a look at it and sort things out.

Heya,

I notice you have a private function (_bt_4dd) - it might be a good idea to prefix this with the module name to avoid clashes.

There is some coding standard errors to fix :
Review of the 7.x-1.x branch (commit 4676088):

• ./includes/batch/taxons.inc: all functions should be prefixed with your module/theme name to avoid name clashes. See https://www.drupal.org/node/318#naming
  

  function _bt_4dd($tsn, &$context) {
  

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.

Source: http://pareview.sh/ - PAReview.sh online service

The author has already said why the function has this name. Please do not post automated reviews blindly :-(

Ignore the function _bt_4dd. Please fix the remaining

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

• ./includes/batch/taxons.inc: all functions should be prefixed with your module/theme name to avoid name clashes. See https://www.drupal.org/node/318#naming
  

  function _bt_4dd($tsn, &$context) {
  

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.

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.

FILE: /root/repos/pareviewsh/pareview_temp/includes/batch/indices.inc
--------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
--------------------------------------------------------------------------
   1 | ERROR | [x] The PHP open tag must be followed by exactly one blank
     |       |     line
 101 | ERROR | [x] Expected 1 blank line after function; 2 found
--------------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------


FILE: /root/repos/pareviewsh/pareview_temp/includes/batch/taxons.inc
--------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------
 1 | ERROR | [x] The PHP open tag must be followed by exactly one blank
   |       |     line
--------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------


FILE: /root/repos/pareviewsh/pareview_temp/includes/itis/db.inc
--------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------
 1 | ERROR | [x] The PHP open tag must be followed by exactly one blank
   |       |     line
--------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------


FILE: /root/repos/pareviewsh/pareview_temp/tests/import_page.test
--------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
--------------------------------------------------------------------------
  1 | ERROR | [x] The PHP open tag must be followed by exactly one blank
    |       |     line
 14 | ERROR | [x] Expected 1 blank line before function; 0 found
--------------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------


FILE: /root/repos/pareviewsh/pareview_temp/biological_taxonomy.install
--------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------
 1 | ERROR | [x] The PHP open tag must be followed by exactly one blank
   |       |     line
--------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------

Time: 209ms; Memory: 8Mb

If the project review guide is actually read, some of you guys will find that these issues are non-blocking issues.

The code standard is not perfect but as stated in the guide, if the project maintainer has demonstrated use of the coding standards however there are a couple of issues, this is not a reason to block the module from promotion.

I have tested this module and everything is functional so I feel that if this should now be reviewed and tested by the community.

Hi JacobSanford,

My Findings are :

1: Never concatenate data directly into SQL queries
$get_kingdom_id_query = "SELECT tsn FROM taxonomic_units WHERE kingdom_id = '$kingdom_tinyint_id' AND rank_id<='$bottom_limit_selected' $filter_valid_accepted ORDER BY rank_id ASC";

AND
$get_kingdom_id_query = "SELECT kingdom_id FROM kingdoms WHERE kingdom_name='$kingdom_name'";

$get_term_query = "SELECT tsn, complete_name, parent_tsn, rank_id FROM taxonomic_units WHERE tsn='$tsn'";

See : https://www.drupal.org/docs/7/security/writing-secure-code/overview

2: Please keep tablename in curly brackets , Otherwise it will give error when table_prefix is used in system
example
$get_kingdoms_query = "SELECT count(*) FROM kingdoms";

3: All user facing text should be passed through t().
return '<p>There is a problem with the ITIS database file. See the '

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.