colincalnan [colincalnan]

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.

Colin -- Talked to NikG about this today. CVS applications are kind of swamped these days, so it may take a while.

Code formatting looks great (Coder found no issues), commenting is really thorough, and sanitizing functions seem to be used everywhere they're useful for security. The only issue I see is the use of hook_init for CSS and JS includes that don't seem to be needed on every page. Ideally those would only be added where they're actually needed. But that's not really an issue that needs to be fixed prior to CVS.

• 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. The version line needs to be removed from the .info file.
2. Hook implementation comments should be like the following one:

   /**
    * Implements hook_menu().
    */
   

3.     drupal_set_message('Failed to get Metadata for ' . $ogdi->dataset_id, 'warning');
   
   

   Use a t()-placeholder.

4. The module requires PHP5, but it doesn't declare its dependency from that version of PHP.

5.     $ch = curl_init($this->datasource . $querystring);
       
       curl_setopt($ch, CURLOPT_RETURNTRANSFER, 1);
       curl_setopt($ch, CURLOPT_HEADER, 0);
       curl_setopt($ch, CURLOPT_TIMEOUT, 30);
       $result = curl_exec($ch);
       $info = curl_getinfo($ch);
       curl_close($ch);
   
   

   Why isn't the code using drupal_http_request()?

6. PHP files should not end with ?>, as reported in the coding standards.

7.     '#description'  =>  t('The value for the label of the Filter box on the dataset page'),
   
   

   Description strings, as other strings used in the user interface, should end with a period.

Hi, wondering how the progress of this module is. Would propose to add this module to the list of modules for governmental services: http://groups.drupal.org/node/99004

looking forward

Code looks great now.

FWIW, Adding JS and CSS unconditionally is better when css/js aggregation is enabled. I don't think we want to ding applicants on that.

Thank you for your contribution!
I am going to update your account so you can opt into security advisory coverage now.
These are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find more contributors chatting on the IRC #drupal-contribute channel. So, come hang out and stay involved.
Thank you, also, for your patience with the review process.
Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

I thank all the dedicated reviewers as well.

@colincalnan module is ok, cvs account is ready, so looking forward to the project page of this module :)

As I heard this still needs some review, so here are my recommendations:

/**
 * Implements hook_token_list()
 * and hook_token_values().
 *
...

Always write "Implements hook_..." to the appropriate place, so it can be easily search for.

/**
 * @file
 */

Don't just write @file so the coder module would shut up finally ;)

Every file has extremely verbose documentation (which is very great!), except ogdi-api.php, which seems to be the core of this thing.

Use DataTables module as a dependency, don't include the source in your module (so there's no conflict if other modules try to use DataTables).

$output .= '<div id="ogdi-wrapper">' . chr(10);

I'd rather write

$output .= "<div id=\"ogdi-wrapper\">\n";

Or if you don't like \"

$output .= '<div id="ogdi-wrapper">' . "\n";

Not a big deal.

Else the module looks great, I especially loved the huge amount of comments.

As I heard this still needs some review

I don't think it does. The account has been approved, so I believe this issue is done. There's still no project, but that's because colincalnan hasn't created it, not because the module needs more review.