fuzzy76 [fuzzy76]

Hello, and thanks for applying for a CVS account. I am adding the review tags, and some volunteers will review your code, pointing out what needs to be changed.

As per http://drupal.org/cvs-application/requirements, the motivation message should be expanded to contain more details about the features of the proposed module/theme; for modules it should include also a comparison with the existing solutions, while for themes a screenshot is also required.

Thanks for your reply.

Having trouble getting this working on Drupal 6.1.4 on XAMPP for Windows Version 1.7.1.

I think there is something wrong with the way I am writing the path. I tried using double \ ("\\" for directory levels) as well and had the same error.

Certify Module Config:

Enter the path to the location where to store certificates.:
C:\xampp\htdocs\certificates
Path to pdftk.:
C:\xampp\htdocs\CMS\helperapps\pdftk.exe

Error log follows:

Execution of pdftk failed - not installed or wrong path?
Command line:
C:\xampp\htdocs\CMS\helperapps\pdftk.exe C:\xampp\htdocs\CMS\modules\certify\default_certificate.pdf fill_form C:\xampp\htdocs\certificates\cert_35_.fdf output C:\xampp\htdocs\certificates\cert_35_.pdf flatten 2>&1
Output:
Unhandled Java Exception: java.io.CharConversionException at 0x0056175e (Unknown Source) at 0x00561c52 (Unknown Source) at 0x00561d03 (Unknown Source) at 0x0057692d (Unknown Source) at 0x0055f64d (Unknown Source) at 0x006bc0ad (Unknown Source) at 0x006994e0 (Unknown Source) at 0x00567a96 (Unknown Source) at 0x0056e8f2 (Unknown Source) at 0x0056e92c (Unknown Source) at 0x00445fa6 (Unknown Source) at 0x0048b15e (Unknown Source) at 0x0048b5b8 (Unknown Source) at 0x0048c238 (Unknown Source) at 0x0046c81f (Unknown Source) at 0x00469301 (Unknown Source) at 0x00466810 (Unknown Source)

I am using the windows executable from http://www.pdfhacks.com/pdftk/pdftk-1.41.exe.zip

I also tested it standalone (outside Drupal/Quiz) in the command line and got the same error

Unhandled Java Exception
java.io.CharConversionException

I saw that this had similar issues in Ubuntu 9.10 until patched, the GCJ environment was mentioned as a culprit there also.

Not sure where I can get another windows port for the current version.

Tried historical release PDFTK 1.12 Windows EXE from http://www.accesspdf.com/pdftk/#history, got the following when testing the EXE as a standalone app:

Error: Failed to open FDF (data) file:
xxx.fdf
No output created

The purpose of a CVS application is not to debug the code, but to verify if the code doesn't have security issues, follows the coding standards, and uses the correct Drupal functions (and API).

While debugging information could be interesting for fuzzy76, a CVS application is not the issue queue for the module.

• The points reported in this review are not in order or 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.   $t = get_t();
     $requirements = array();
     $quizneeded = array('title' => $t('Certify require Quiz v4'), 'description' => $t('Quiz v4 or higher is required'), 'severity' => REQUIREMENT_ERROR);
   
     if ($phase=="runtime" || $phase=="install") {
   
       // Get current list of modules
       $files = drupal_system_listing('\.module$', 'modules', 'name', 0);
   
       if (!isset($files['quiz'])) { // Do we have a quiz module at all? This shouldn't be an issue since the .info specifies it as a dependency
         $requirements['quizneeded'] = $quizneeded;
       }
       else {
   
   

   The dependency from a module is already checked from Drupal; there is no need to write extra code for that.

2. See http://drupal.org/coding-standards to understand how a module should be written. In particular, see how the code should be formatted; how Drupal variables, global variables, constants, and functions defined from the module should be named.

3.     'site_name'         => variable_get('site_name', ''),
   
   

   The default value for that Drupal variable is 'Drupal'.

4.     header('Content-Description: File Transfer');
       header('Content-Type: application/pdf');
       header('Content-Disposition: attachment; filename="' . basename($filebase . 'pdf') . '"');
       header('Content-Transfer-Encoding: binary');
       header('Expires: 0');
       header('Cache-Control: must-revalidate, post-check=0, pre-check=0');
       header('Pragma: public');
       header('Content-Length: ' . filesize($filebase . 'pdf'));
   
   

   The function to use is drupal_set_header().

5.     watchdog("fail", "Execution of pdftk failed - not installed or wrong path?<br />Command line:<br />%exe<br />Output:<br />%output", array('%exe' => $exec, '%output' => $output), WATCHDOG_ERROR);
       drupal_set_message(t("The certificate generation failed. Please contact a system administrator for assistance."), 'error');
       drupal_goto('/certify');
   
   

   The first parameter of watchdog() is a string that identifies the module that output the message error; 'fail' doesn't identify the module that output the error message.

6.   //  TODO_1: Utvid funksjonalitete under til å ta høyde for følgende scenario:
     //  Sett at vi har et scenario der det finnes et sertifikat som inneholder to tester og en bruker har tatt en av testene.
     //  For å kunne regne ut riktig prosentscore for brukeren trengs det info også om de quiz'ene som brukeren ikke har besvart.
     //  quiz_get_score_data funksjonen returnerer i dag info om quiz som brukeren har tatt.
     //  Kunne denne funksjonen også sendt de quiz'ene som ikke er besvart, men på en eller annen måte merket de?
     //
     //  Slik koden foreligger nå vil det bli en feil i utregning av progress for et sertifikat fordi quiz'er som brukeren ikke
     //  har besvart ikke tas med i beregningen.
     //  En effekt av dette er at man får lov å laste ned et sertifikat som innholder to tester når man har bestått bare en av dem.
   
   

   Comments, and variable names should be in English.
   The presence of a TODO comment makes me think the code is not complete; one of the requirements is that the code should be as much complete as it can.

7.           $cert = new stdClass();
             $cert_node = node_load($data->nid);
             $cert->title = $cert_node->title;
             $cert->body = $cert_node->body;
             $cert->nid = $data->nid;
             $cert->certificate_template = $data->certificate_template;
             $cert->quizzes[] = $quiz;
             $cert->downloadable = TRUE;
             $certificates[$data->nid] = $cert;
   
   

   Is the code checking if the user has access to the node, before to show its data?

8.       $output = filter_filter('process', 2, NULL, file_get_contents( dirname(__FILE__) . "/README.txt") );
   
   

   There is a Drupal function to get the name of the directory where a module is installed.

Thank you for your contribution! I am going to update your account.

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.