The BIRT module fills a necessary hole in the Drupal environment by providing a way to publish solid business intelligence reports within a Drupal website. The BIRT module is already within several live websites, for Drupal 5 and Drupal 6. As written on Eclipse.org, BIRT is "an Eclipse-based open source reporting system for web applications". It possibly has the most free features of any BI application.

While there is a few settings that need to be made on the BIRT engine side, on the Drupal site there are no external libraries needed.

We have included several example reports that can get users -- who have some familiarity with BIRT -- up and running quickly with the BIRT module. We have provided several videos to get users up and running with out module as well as links to videos on how to get up and running with BIRT in general.

A typical user would have the BIRT report designer within their Eclipse in order to build reports. They would have the BIRT engine in order to serve up reports. They would have Drupal in order to be the content management system of the reports, taking advantage of Drupal's strong permissions system.

The Drupal API's were used extensively. And I have put the BIRT module through Coder and fixed all the issues, including the minor ones. The only issues that remained were false positives: The short Java file that comes with this module is to store and communicate the BIRT report settings to the BIRT engine, but Coder evaluated the code as if it were PHP. Proper Drupal security functions were used in making this module (the t() function with output, variable substitution in queries, table bracketing in queries, for example), and security will be taken serious with follow up versions of this module.
************EDIT: Please look specifically at approving the Drupal 7 version. Thank you.

Link to sandbox: http://drupal.org/sandbox/tonioman/1605168
Link to git repo: git.drupal.org:sandbox/tonioman/1605168.git

Reviews of other module applications:
http://drupal.org/node/1479034#comment-6591884
http://drupal.org/node/1799148#comment-6591750
http://drupal.org/node/1802578#comment-6591800
http://drupal.org/node/1597704#comment-6591806 (this last one was more of a housekeeping task)

CommentFileSizeAuthor
#13 drupalcs-result.txt929 bytesklausi

Comments

tglynn’s picture

tglynn commented
Status: Active » Needs review

Marking as needs review.

tglynn’s picture

tglynn commented
Issue summary: View changes

Mentioned previous issue in the issue queue.

dev001’s picture

dev001 commented
Status: Needs review » Needs work

par review - lots of issues. you can see it here:http://ventral.org/pareview/httpgitdrupalorgsandboxtonioman1605168git

Manual review:
1.why you start a session .i think you have no need to start a session.
2. http://" . $_SERVER["HTTP_HOST"] use $base_url or any drupal function for get url.

wabranty’s picture

wabranty commented
Status: Needs work » Active

@login radius: Thank you for your comments. We have taken what was essentially an abandoned module for Drupal 5 and are working hard to make it viable Drupal 6 and Drupal 7. Our own testing has shown the same Java problems you pointed out and thus we are working to tighten up the commands to the PHP/Java Bridge and BIRT.

tglynn’s picture

tglynn commented
Issue summary: View changes

Added project and git links.

tglynn’s picture

tglynn commented
Status: Active » Needs review

Can you please review this module again? Please note that the Drupal 7 version is what I would like you to review. Over the past two months I have entirely rewritten this module.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented

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 I'll take a look at your project right away :-)

killerpoke’s picture

killerpoke commented
Assigned: Unassigned » killerpoke
killerpoke’s picture

killerpoke commented
Status: Needs review » Needs work

Hey tonioman!

> Update git clone on sandbox-site to real branch
> Automatic check fails by about 200 errors (see http://ventral.org/pareview/httpgitdrupalorgsandboxtonioman1605168git)
> Please create a README.txt that follows the guidelines for in-project documentation (your's is kind of empty).

birt.install
--------------

> line 85/86: you should maybe use (taxonomy_vocabulary_machine_name_load === FALSE) for checking this, because it's the proper way of doing this (At least I think so)
> I don't think it's save to check against the taxonomy-term, you should use the machine_name.

> line 89: You should create a new stdClass, and not casting an array to an object
> line 95. You should use the "taxonomy_vocabulary_machine_name_load" to get the taxonomy-object (and the vid) and not doing a SQL-Select by yourself
> line 112-115: when you save the term-object, it's id-value gets set. You don't need the Select-Statement to get it (see: http://api.drupal.org/api/drupal/modules%21taxonomy%21taxonomy.module/fu...)
> line 143-148: I'm not sure if it is a good idea to just delete all nodes, without warning the user

birt.module
--------------

-

I think the only two things blocking this module right now are the formatting-erros (and the README.txt) and the correct use of the taxonomy API

killerpoke’s picture

killerpoke commented
Assigned: killerpoke » Unassigned
tglynn’s picture

tglynn commented
Status: Needs work » Needs review

Thanks for the review killerpoke. I have made your recommended changes as well as several other improvements to the module. Here are my responses to each of your notes:

Branch: Changed to real branch

README file: Created in main branch and not in master branch

Formatting errors:
----- All the formatting errors were cleared except for a handful: In the par review, all that’s left is pretty much are small complaints about there being camelcase functions, but those are from the API that I’m using so my module hopefully won’t be judged on it.

> line 85/86: you should maybe use (taxonomy_vocabulary_machine_name_load === FALSE) for checking this, because it's the proper way of doing this (At least I think so) -
----- This change has been successfully made.
> I don't think it's save to check against the taxonomy-term, you should use the machine_name.
----- This module is checking machine_name.

> line 89: You should create a new stdClass, and not casting an array to an object
----- “taxonomy_vocabulary_save((object) array(“ is considered good practice as seen by many solid developers online.
> line 95. You should use the "taxonomy_vocabulary_machine_name_load" to get the taxonomy-object (and the vid) and not doing a SQL-Select by yourself
----- This change has been successfully made.
> line 112-115: when you save the term-object, it's id-value gets set. You don't need the Select-Statement to get it (see: http://api.drupal.org/api/drupal/modules%21taxonomy%21taxonomy.module/fu...)
----- This change has been successfully made.
> line 143-148: I'm not sure if it is a good idea to just delete all nodes, without warning the user
----- Because the module must be disabled before it gets uninstalled, the module code will not be active when the user goes to uninstall it. In other words, the module cannot produce a message before uninstall. I'll continue to look into it as it is something I definitely agree with you would be very very helpful in preventing problems in certain cases.

jchand’s picture

jchand commented
Status: Needs review » Fixed

This Module has made huge improvements and is in a much better shape then when it was first posted for review back in June 28, 2012 . Better use of Drupal API's and proper and semantic use of structure, and i noticed that most of the issues described above have also been taken care of. Based on this, i think this module should be under Status : Fixed.

tglynn’s picture

tglynn commented
Status: Fixed » Reviewed & tested by the community

Switching status based on jchand's response.

tglynn’s picture

tglynn commented
Issue summary: View changes

Wanted to emphasize that the Drupal 7 version is the one to focus on.

tglynn’s picture

tglynn commented
Issue summary: View changes

edits

tglynn’s picture

tglynn commented
Issue tags: +PAreview: review bonus

Did four manual reviews. One of them was small; that's why there's four instead of three.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: -PAreview: review bonus
StatusFileSize
new drupalcs-result.txt929 bytes

There is still a master branch, make sure to set the correct default branch: http://drupal.org/node/1659588 . Then remove the master branch, see also step 6 and 7 in http://drupal.org/node/1127732
Review of the 7.x-1.x branch:

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.

manual review:

  1. "variable_get('birt_server_address', '')": all variables that your module defines must be removed in hook_uninstall().
  2. "variable_get('javabridge_directory_address', '')": all variables that your module uses must be prefixed with your module's name to avoid name collisions.
  3. "'access arguments' => array('Administer BIRT'),": that permission is not defined with hook_permission()? And permissions should be all lower case.
  4. "include_once "http://" . variable_get('javabridge_directory_address') . "/java/Java.inc";": that could be a problem for server setups that do not allow remote inclusion of files.
  5. birt_preprocess_node(): do not simply echo exceptions/error messages, that might disclose details about your server setup. Use watchdog() instead to log errors.
  6. birt_init(): why do you need that in hook_init()? This hook is expensive and is fired on every single page request. Why can't you cleanup before you execute stuff in birt_preprocess_node()?
  7. node--birt.tpl.php: "Report Should Appear Here": all user facing text should run through t() for translation.
  8. birt_preprocess_node(): "$vars['report'] == $report;": comparison? Did you mean "=" instead of "=="? Does this module currently even work? The report is not added to the theming variables?
  9. node--birt.tpl.php: $report_url is not set anywhere? Same for $report_height?

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

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented

Based solely on Klausi's review: #3 means that this module can only be administered by uid 1, but if it weren't for that I would say that #4 is a security issue (it's essentially a remote code execution issue for anyone who can administer the module). I think the best solution to #4 is that their should be README.txt instructions on how to download the java/java.inc file instead of including it over http. I wonder if opcode cache's handle files included over http like that?

I suggest that when the hook_permission is added that "administer birt" permission is configured as "restrict access" => TRUE to make clear the importance of the the permission.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs work » Closed (won't fix)

Closing due to lack of activity. Feel free to reopen if you are still working on this application.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Issue summary: View changes

Completed manual reviews