Report Builder

manual review:

• project page is a bit short, see http://drupal.org/node/997024
• "$report['data'] = unserialize($report['data']);": you should define the db field as 'serialize' in hook_schema(), then you don't need to do that yourself.
• "'#title' => t($report['title']),": title is not sanitized here, what if I enter a javascript snippet es title? Is this XSS vulnerable then?
• "$title = $form_state['input']['title'];": why don't you access $form_state['values'] here?
• report_builder_get_title(): this function is not used anywhere? Also it does not do really something useful?
• "ctools_modal_text_button('Change Filters'..." all user facing text should run through t() for translation, or does ctools handle that for you?
• "'#title' => 'Report title',": same here, use t().
• "<em>@title</em>": use the "%" placeholder, it will generate em tags for you.
• "'<h2 id="report-builder-title">' . $report['title'] . '</h2>'": again, the title should be sanitized here.
• "'#value' => 'Update',": again, use t() here. Please check all your strings.
• "drupal_deliver_html_page(render($page));": why do you need that? Shouldn't be returning the $page renderable array be enough, and Drupal takes care of the rendering itself?

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Looking at the previous issues

• "$title = $form_state['input']['title'];": why don't you access $form_state['values'] here?
  
  the difference between the two is that $form_state['input'] is the raw user input and it shouldn't be used unless you understand possible security implications. Almost all the FAPI functions use $form_state['values'] for their processing. If $form_state['values'] doesn't work for you for whatever reason I think it's OK to use $form_state['input']
• report_builder_get_title() is a callback for the report view page. The returned value $report['title'] should be sanitized because it goes directly into the page, I think.

mkadin, it can happen, I have seen it before because FAPI processes these values. If using values doesn't work for you then use input, make sure that there are no security implications and add a note explaining why it has to be so.

Review of the 7.x-1.x branch:

• Drupal Code 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. Get a review bonus and we will come back to your application sooner.

manual review:

• "check_plain(t('Report has been saved'))": you don't need check_plain() here as no user provided input is involved.
• "check_plain(t('Report %title has been deleted.', array('%title' => check_plain($report['title']))))": you don't need the 2 check_plain() here as the "%" placeholder will already sanitize the title for you. Double escaping is bad. Also in other places.
• report_builder_list_reports(): do not concatenate translated strings, always use placeholders in one string, i.e. for links.
• "'info' => check_plain('Report Builder Chart: ' . $report->title),": all user facing text should run through t() for translation, and you should use a placeholder for the title.

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

manual review:

• "'#default_value' => (($new) ? 500 : check_plain($report['data']['chart_data']['width'])),": The form API sanitizes #default_value for you, so no need for check_plain() here. See http://drupal.org/node/28984

Otherwise this looks RTBC to me. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

So, let's get picky ;)

1. .install, _schema(): Make sure all descriptions begin with a capital letter and end with a dot.
2. drupal_write_record('report_builder', &$report); Oo! Call-time pass-by-reference has been deprecated! Be careful with copy and paste.
3. while ($i < sizeof($values)) {don't do such, as it's a while loop it'll re-execute it's condition on every loop, so the sizeof will be executed over and over again
4. The <> operator is normally only used in sql queries, it's unusual to see it in code, maybe you replace it with !=?
5. Your functions are pretty well commented, what I'm missing are more inline-comment, you could definitely use more of them ;)

Beside that I had several warnings and notices during testing I couldn't really reproduce (tested on a clean local install), so consider creating an early alpha release first and do some more intensive testing with different configuration and on different environments.
As this is a quite big module there's probably more we've missed but I'm pretty sure there just minor issues and I think your definitely experienced enough to maintain your projects seriously.

So, thanks for your contribution and welcome to the community of project contributors on drupal.org!

I've granted you the git vetted user role which will let you promote this to a full project and also create new projects as either sandbox or "full" projects depending on which you feel is best.

Thanks, 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.

As you continue to work on your module, keep in mind: Commit messages - providing history and credit and Release naming conventions.

Thanks to the dedicated reviewer(s) as well.