[D7] Hypertable

Automated review

The automated review didn't show any problems, great job!

Manual review

The code looks very good. I have two remarks in hypertable.module. One about the naming of a global variable:

  if (!isset($GLOBALS['THRIFT_ROOT'])) {
    $GLOBALS['THRIFT_ROOT'] = HYPERTABLE_THRIFT_ROOT;
  }

From the Drupal Coding standards:

If you need to define global variables, their name should start with a single underscore followed by the module/theme name and another underscore.

The second remark is about the includes you have:

  include_once 'HypertableHQL.php';
  include_once 'HypertableDriver.php';

According to the Drupal Coding Standards the include should start with a ".".

Greetings!

We are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and put yourself on the PAReview: review bonus high priority list. Then I'll take a look at your project right away :-)

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

Hi StevenWill,

Nice code, however I have some things for you. :)

Manual Review:

hypertable.module

16 | In function hypertable_init you are using include_once:

1. You may want to use module_load_include in drupal.
2. You already have class includes in hypertable.info, so you do not need to include them again, especially in hook_init.
3. Move files contain classes in separate folder "includes"

hypertable_nodes.module

1. 24 | function hypertable_nodes_entity_delete - code applies only to nodes, so you may want to use hook_node_delete
2. 38 | function hypertable_nodes_entity_insert - code applies only to nodes, so you may want to use hook_node_insert
3. 48 | function hypertable_nodes_entity_update - code applies only to nodes, so you may want to use hook_node_update
4. 62 |
   

     // Unset unwanted parts of the entity.
     unset($entity->rdf_mapping);
     unset($entity->original);
     unset($entity->additional_settings__active_tab);
     unset($entity->submit);
     unset($entity->preview);
     unset($entity->delete);
     unset($entity->form_build_id);
     unset($entity->form_token);
     unset($entity->form_id);
     unset($entity->op);
     unset($entity->validated);
     unset($entity->is_new);
            

   Use one call of unset function:

     // Unset unwanted parts of the entity.
     unset($entity->rdf_mapping,
              $entity->original,
              $entity->additional_settings__active_tab,
              $entity->submit,
              $entity->preview,
              ....,
             $entity->is_new);
   

5. 111 | Unused variable $start
6. 117 | Should date '2012-12-05' be static?
7. 111 | Unused variable $start
8. 127 | 'Anonymous' may be renamed by developer in drupal settings, so use variable_get of that setting.
9. 128 | Why you use string concatenation in array auto-increment '$items[] .=' ?
10. 142 | You may want to avoid extra variable just for couple uses '$total = $result->total'
11. 145 | You may want to avoid extra variable just for one use
    

       $output .= theme('pager', array('quantity' => $total));
    
      return $output;

hypertable_users.install

1. 25 | This db query needs to be reformatted according to standars, or better use db_select. $sql = db_query("SELECT * from {users} where uid = '1'");

hypertable_users.module

1. 59 | Not sure why are you defining extra variable to set the same options:
   

    function hypertable_users_entity_massage($entity) {
     // Create mapping of fields for hypertable.
     $user = new stdClass();
     $user->uid = $entity->uid;
     $user->name = $entity->name;
     $user->pass = $entity->pass;
     $user->mail = $entity->mail;
     $user->theme = $entity->theme;
     $user->signature = $entity->signature;
     $user->signature_format = isset($entity->signature_format) ? $entity->signature_format : '';
     $user->created = $entity->created;
     $user->access = $entity->access;
     $user->login = $entity->login;
     $user->status = $entity->status;
     $user->timezone = isset($entity->timezone) ? $entity->timezone : '';
     $user->language = $entity->language;
     $user->picture = $entity->picture;
     $user->init = $entity->init;
     $user->data = isset($entity->data) ? json_encode($entity->data) : '';

2. Others are the same as in hypertable_nodes.module

Good luck!

Instead of setting your variables in hypertable_install() you can use those default values at the top of your module file where you have all the defines. For ex,
define("HYPERTABLE_PORT", variable_get('hypertable_port', 38080)); should work exactly the same.

You shouldn't load your libraries in hypertable_init() - this will cause it to be loading on every Drupal page request, instead just load them where you need them. In this case I think that would be inside your hypertable_nodes example module.

This seems like a great addition! I'm not too familiar with Hypertable - but could this also be done in the DB abstraction layer? in includes/database we have tools for converting a query to a specific DB implementation like pgsql or sqlite. https://drupal.org/project/oracle does this by providing an "oracle" folder with all the necessary includes that can be dropped into includes/database/ as well as a module to help make use of it (mostly admin and settings pages).

No major blockers though, looks like a useful module.

----
Top Shelf Modules - Enterprise modules from the community for the community.

You have a typo in hypertable_menu() "ypertable". And in hypertable_nodes_list_nodes() "anonymouse".

The Libraries API module is a recommended method for adding 3rd party dependencies. I'd very much like to see hook_init() cleaned up so you're not loading all that on every Drupal page request.

In hypertable_nodes_menu() is there a security problem in having 'access callback' => TRUE? I think this would allow any user to see the site's nodes regardless of permission settings.

----
Top Shelf Modules - Crafted, Curated, Contributed.

Multiple Applications

It appears that there have been multiple project applications opened under your username:

Project 1: https://drupal.org/node/1992544

Project 2: https://drupal.org/node/1944834

As successful completion of the project application process results in the applicant being granted the 'Create Full Projects' permission, there is no need to take multiple applications through the process. Once the first application has been successfully approved, then the applicant can promote other projects without review. Because of this, posting multiple applications is not necessary, and results in additional workload for reviewers ... which in turn results in longer wait times for everyone in the queue. With this in mind, your secondary applications have been marked as 'closed(duplicate)', with only one application left open (chosen at random).

If you prefer that we proceed through this review process with a different application than the one which was left open, then feel free to close the 'open' application as a duplicate, and re-open one of the project applications which had been closed.

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