Issue #1392922: Correct Coder Review-reported code issues.

72e12f75 · Liam Morlund · Kevin Kaland · fb1b69be · 72e12f75 · 72e12f75
Commit 72e12f75 authored 13 years ago by Liam Morlund Committed by Kevin Kaland 13 years ago
--- a/fillpdf.admin.inc
+++ b/fillpdf.admin.inc
@@ -38,7 +38,7 @@ function fillpdf_settings($form, &$form_state) {
  $form['remote']['fillpdf_remote_service'] = array(
    '#type' => 'checkbox',
    '#title' => t('Use fillpdf-service.com'),
-    '#default_value' => variable_get('fillpdf_remote_service', true),
+    '#default_value' => variable_get('fillpdf_remote_service', TRUE),
  );
  $form['remote']['fillpdf_api_key'] = array(
    '#type' => 'textfield',
@@ -73,7 +73,7 @@ function fillpdf_settings($form, &$form_state) {
  $form['local']['fillpdf_local_service'] = array(
    '#type' => 'checkbox',
    '#title' => t('Use locally-installed PHP/JavaBridge'),
-    '#default_value' => (variable_get('fillpdf_local_service', true)),
+    '#default_value' => (variable_get('fillpdf_local_service', TRUE)),
  );
  if (!(file_exists(drupal_get_path('module', 'fillpdf') . '/lib/JavaBridge/java/Java.inc'))) {
    $form['local']['warning'] = array(
@@ -91,9 +91,9 @@ function fillpdf_settings($form, &$form_state) {
  $form['local_php']['fillpdf_local_php'] = array(
    '#type' => 'checkbox',
    '#title' => t('Use locally-installed pdftk'),
-    '#default_value' => (variable_get('fillpdf_local_php', true)),
+    '#default_value' => (variable_get('fillpdf_local_php', TRUE)),
  );
-  //TODO: Modify to add check for pdftk installed
+  // TODO: Modify to add check for pdftk installed

  $js = <<<JS
 Drupal.behaviors.fillpdfSettingsCheckboxes = {
@@ -156,7 +156,7 @@ function fillpdf_forms_admin($form, &$form_state) {
  );
  $form['submit'] = array(
    '#type' => 'submit',
-    '#value' => t('Submit'),
+    '#value' => t('Upload'),
    '#weight' => 15,
  );

@@ -174,7 +174,7 @@ function fillpdf_forms_admin_validate($form, &$form_state) {
    form_set_error('url', t('A PDF must be provided.'));
  }

-  //from includes/file.inc, line 634, but can't use that function because file not an object yet	
+  // from includes/file.inc, line 634, but can't use that function because file not an object yet
  if (!preg_match('/\.pdf$/i', $file)) {
    form_set_error('url', t('Only PDF files are allowed'));
  }
@@ -193,7 +193,7 @@ function fillpdf_forms_admin_submit($form, &$form_state) {
  // $validators not working, so I just checked manually in fillpdf_forms_validate()
  $validators = array('file_validate_extensions' => array('pdf'));
  if ($file = file_save_upload('upload_pdf', $validators, $dir, FILE_EXISTS_REPLACE)) {
-    drupal_set_message('<strong>' . $file->filename . '</strong> was successfully uploaded');
+    drupal_set_message(t('<strong>@filename</strong> was successfully uploaded.', array('@filename' => $file->filename)));
    $file->status = FILE_STATUS_PERMANENT;
    $file = file_save($file);
    // Does this file already exist in {fillpdf_forms}? If so, don't re-insert it.
@@ -215,8 +215,8 @@ function fillpdf_forms_admin_submit($form, &$form_state) {
    fillpdf_parse_pdf($fid);
  }
  else {
-    //commented out because even though error if file doesn't upload right, not error if they dont' upload a file (& this is still triggered)
-    drupal_set_message('Error saving file to ' . $dir, 'error');
+    // commented out because even though error if file doesn't upload right, not error if they dont' upload a file (& this is still triggered)
+    drupal_set_message(t('Error saving file to @dir', array('@dir' => $dir)), 'error');
  }

  $form_state['redirect'] = "admin/structure/fillpdf/$fid";
@@ -246,7 +246,7 @@ function fillpdf_form_edit($form, &$form_state, $fid) {
  $form['pdf_info'] = array(
    '#type' => 'fieldset',
    '#title' => 'PDF Form information',
-    '#collapsed' => true,
+    '#collapsed' => TRUE,
  );
  $form['pdf_info']['submitted_pdf'] = array(
    '#type' => 'item',
@@ -256,7 +256,7 @@ function fillpdf_form_edit($form, &$form_state, $fid) {
  $form['pdf_info']['sample_populate'] = array(
    '#type' => 'item',
    '#title' => 'Sample PDF',
-    '#description' => l("See which fields are which in this PDF", fillpdf_pdf_link($fid, null, null, true)) . '<br />' . 
+    '#description' => l(t('See which fields are which in this PDF.'), fillpdf_pdf_link($fid, NULL, NULL, TRUE)) . '<br />' .
    t('If you have set a custom path on this PDF, the sample will be saved there silently.'),
  );
  $form['pdf_info']['form_id'] = array(
@@ -308,7 +308,7 @@ function fillpdf_form_edit($form, &$form_state, $fid) {

  $form['submit'] = array(
    '#type' => 'submit',
-    '#value' => t('Submit'),
+    '#value' => t('Update'),
  );
  $form['delete'] = array(
    '#type' => 'submit',
@@ -325,9 +325,9 @@ function fillpdf_form_edit($form, &$form_state, $fid) {
  $rows = array();
  foreach ($q as $field) {
    $row = array(
-      check_plain($field->label), //editable
+      check_plain($field->label), // editable
      check_plain($field->pdf_key),
-      $field->value, //editable, expandable
+      $field->value, // editable, expandable
      ($field->replacements ? 'Yes' : 'No'),
      l(t('Edit'), "admin/structure/fillpdf/$fid/edit/{$field->pdf_key}"),
      l(t('Delete'), "admin/structure/fillpdf/$fid/delete/{$field->pdf_key}"),
@@ -372,8 +372,8 @@ function fillpdf_form_edit_submit($form, &$form_state) {
  ->condition('fid', $form['#pdf_form']->fid)
  ->execute();
    $form_state['redirect'] = "admin/structure/fillpdf/{$form['#pdf_form']->fid}";
-    drupal_set_message('Successfully updated form');
-    //$form_state['nid'] = $node->nid;
+    drupal_set_message(t('Successfully updated form.'));
+    // $form_state['nid'] = $node->nid;
  }
 }

@@ -408,7 +408,7 @@ function fillpdf_form_delete_confirm_submit($form, &$form_state) {
  db_delete('fillpdf_forms')
  ->condition('fid', $form['#pdf_form']->fid)
  ->execute();
-  drupal_set_message('Your form has been deleted.');
+  drupal_set_message(t('Your form has been deleted.'));
  $form_state['redirect'] = 'admin/structure/fillpdf';
 }

@@ -602,10 +602,10 @@ function fillpdf_field($op, $fid, $pdf_key = NULL) {
  if ($op == 'add') {
    drupal_set_title($pdf_form->title);
  }
-  else if ($op != 'edit') {
+  elseif ($op != 'edit') {
    return fillpdf_form_overview($pdf_form);
  }
-  else if ($pdf_key) {
+  elseif ($pdf_key) {
    $field = db_query("SELECT * FROM {fillpdf_fields} WHERE pdf_key = :pdf_key AND fid = :fid", array(':pdf_key' => $pdf_key, ':fid' => $fid))->fetch();
    if (!$field) {
      drupal_not_found();
@@ -673,7 +673,7 @@ function fillpdf_field_edit($form, &$form_state, $pdf_form, $field) {

  $form['submit'] = array(
    '#type' => 'submit',
-    '#value' => t('Submit'),
+    '#value' => t('Update'),
    '#weight' => 9,
  );

@@ -719,7 +719,7 @@ function fillpdf_field_edit_submit($form, &$form_state) {
    fillpdf_update_field($form['#pdf_form'], $edit_field, $form['#pdf_field']->pdf_key);
  }
  else {
-    //add a new field
+    // add a new field
    $edit_field = (object) $form_state['values'];
    db_insert('fillpdf_fields')
    ->fields(array(
@@ -767,8 +767,8 @@ function fillpdf_field_delete_confirm_submit($form, &$form_state) {
  ->condition('fid', $form['#pdf_field']->fid)
  ->condition('pdf_key', $form['#pdf_field']->pdf_key)
  ->execute();
-  drupal_set_message('Your field has been deleted.');
-  //return 'admin/structure/fillpdf/'. $form['#pdf_field']->fid;
+  drupal_set_message(t('Your field has been deleted.'));
+  // return 'admin/structure/fillpdf/'. $form['#pdf_field']->fid;
  $form_state['redirect'] = 'admin/structure/fillpdf/' . $form['#pdf_field']->fid;
 }


--- a/fillpdf.install
+++ b/fillpdf.install
@@ -100,7 +100,7 @@ function fillpdf_update_7001() {
 }

 /**
- * Add fields to store token replacements. 
+ * Add fields to store token replacements.
 */
 function fillpdf_update_7002() {
  db_add_field('fillpdf_forms', 'replacements', array('type' => 'text', 'size' => 'normal', 'not null' => FALSE));

--- a/fillpdf.module
+++ b/fillpdf.module
@@ -76,14 +76,14 @@ function fillpdf_menu() {
    'type' => MENU_CALLBACK,
  );
  $items['admin/structure/fillpdf/%/export'] = array(
-    'title' => t('Export Fill PDF field mappings'),
+    'title' => 'Export Fill PDF field mappings',
    'page callback' => 'fillpdf_form_export',
    'page arguments' => array(3),
    'access arguments' => $access,
    'type' => MENU_CALLBACK,
  );
  $items['admin/structure/fillpdf/%/import'] = array(
-    'title' => t('Import Fill PDF field mappings'),
+    'title' => 'Import Fill PDF field mappings',
    'page callback' => 'drupal_get_form',
    'page arguments' => array('fillpdf_form_import_form', 3),
    'access arguments' => $access,
@@ -141,9 +141,9 @@ function fillpdf_permission() {
 * @param array/int $nids or $nid, if you pass in one value it will merge with that node.
 *  If array, it will merge with multiple nodes, with later nids overriding previous ones.
 * @param array $webforms Array of webforms, of this strucure: array('nid'=>1, 'sid'=>1)
- * @param bool $sample True if you want to populate the form with its own field-names (to get a gist of PDF)
+ * @param bool $sample TRUE if you want to populate the form with its own field-names (to get a gist of PDF)
 */
-function fillpdf_pdf_link($fid, $nids = null, $webform_arr = null, $sample = false) {
+function fillpdf_pdf_link($fid, $nids = NULL, $webform_arr = NULL, $sample = FALSE) {
  $nids_uri = $webforms_uri = "";

  if (is_array($nids)) {
@@ -164,14 +164,16 @@ function fillpdf_pdf_link($fid, $nids = null, $webform_arr = null, $sample = fal
  }
  $sample = $sample ? '&sample=true' : '';

-  return url('', array('absolute' => true)) . "fillpdf?fid={$fid}{$nids_uri}{$webforms_uri}{$sample}";
+  return url('', array('absolute' => TRUE)) . "fillpdf?fid={$fid}{$nids_uri}{$webforms_uri}{$sample}";
 }



 /**
 * Get the data and form that need to be merged, from the $_GET, and print the PDF
- * @seealso fillpdf_pdf_link for $_GET params
+ *
+ * @see fillpdf_pdf_link()
+ * for $_GET params
 */
 function fillpdf_parse_uri() {
  // Avoid undefined index warnings, but don't clobber existing values
@@ -214,12 +216,14 @@ function fillpdf_parse_uri() {
 * @return doesn't return anything, actually constructs a page from scratch (pdf content-type)
 * and sends it to the browser or saves it, depending on if a custom path is configured
 * or not.
- * @seealso fillpdf_pdf_link for $_GET params
+ *
+ * @see fillpdf_pdf_link()
+ * for $_GET params
 */
-function fillpdf_merge_pdf($fid, $nids = null, $webform_arr = null, $sample = null, $force_download = FALSE, $skip_access_check = FALSE, $flatten = TRUE) {
+function fillpdf_merge_pdf($fid, $nids = NULL, $webform_arr = NULL, $sample = NULL, $force_download = FALSE, $skip_access_check = FALSE, $flatten = TRUE) {
  // Case 1: No $fid
  if (is_null($fid)) {
-    drupal_set_message('Fillpdf Form ID required to print a PDF', 'warning');
+    drupal_set_message(t('Fill PDF Form ID required to print a PDF.'), 'warning');
    drupal_goto();
  }

@@ -228,7 +232,7 @@ function fillpdf_merge_pdf($fid, $nids = null, $webform_arr = null, $sample = nu

  // Case 2: Only $fid -- just give them empty pdf
  if (!empty($nodes) && !empty($webforms) && !is_null($sample)) {
-    $host = url('', array('absolute' => true));
+    $host = url('', array('absolute' => TRUE));
    header("Location: " . $host . '/' . $fillpdf_info->url);
    drupal_exit();
  }
@@ -253,8 +257,8 @@ function fillpdf_merge_pdf($fid, $nids = null, $webform_arr = null, $sample = nu

    foreach ($webform_arr as $webform) {
      if (!$webform['sid']) { // user didn't specify submission-id, meaning they want most recent
-        $webform['sid'] = db_query('select sid from {webform_submissions}
-          where nid=%d and uid=%d order by submitted desc', $webform['nid'], $user->uid)->fetchField();
+        $webform['sid'] = db_query('SELECT sid FROM {webform_submissions}
+          WHERE nid=%d AND uid=%d ORDER BY submitted desc', $webform['nid'], $user->uid)->fetchField();
      }
      $webforms[] = array(
        'webform' => node_load($webform['nid']),
@@ -345,7 +349,7 @@ function fillpdf_merge_pdf($fid, $nids = null, $webform_arr = null, $sample = nu
  );
  drupal_alter('fillpdf_merge_fields_alter', $fields, $context);

-  $output_name = preg_replace('/[^a-zA-Z0-9_]/', '', $fillpdf_info->title) .'.pdf';
+  $output_name = preg_replace('/[^a-zA-Z0-9_]/', '', $fillpdf_info->title) . '.pdf';
  $pdf_data = _fillpdf_get_file_contents($fillpdf_info->url, "<front>");
  $fillpdf_remote_service = variable_get('fillpdf_remote_service', TRUE);
  $fillpdf_local_service = variable_get('fillpdf_local_service', TRUE);
@@ -353,7 +357,7 @@ function fillpdf_merge_pdf($fid, $nids = null, $webform_arr = null, $sample = nu
  if ($fillpdf_remote_service) {
    $api_key = variable_get('fillpdf_api_key', '0');
    $result = _fillpdf_xmlrpc_request(DEFAULT_SERVLET_URL, 'merge_pdf_enhanced', base64_encode($pdf_data), $fields, $api_key, $flatten);
-    if ($result->error == true) {
+    if ($result->error == TRUE) {
      drupal_goto();
    } //after setting error message
    $data = base64_decode($result->data);
@@ -370,7 +374,7 @@ function fillpdf_merge_pdf($fid, $nids = null, $webform_arr = null, $sample = nu
      }
    }
    catch (JavaException $e) {
-      drupal_set_message(java_truncate((string) $e), 'error');
+      drupal_set_message(check_plain(java_truncate((string) $e)), 'error');
      drupal_goto(); //after setting error message
    }
    $data = java_values(base64_decode($fillpdf->toByteArray()));
@@ -421,7 +425,7 @@ function fillpdf_merge_pdf($fid, $nids = null, $webform_arr = null, $sample = nu
  drupal_add_http_header('Expires', 0);
  drupal_add_http_header('Cache-Control', 'must-revalidate, post-check=0, pre-check=0');
  drupal_add_http_header('Content-type', 'application-download');
-  drupal_add_http_header('Content-Length', strlen($data));
+  drupal_add_http_header('Content-Length', drupal_strlen($data));
  drupal_add_http_header('Content-disposition', 'attachment; filename="' . $output_name . '"');
  drupal_add_http_header('Content-Transfer-Encoding', 'binary');
  echo $data;
@@ -495,7 +499,7 @@ function fillpdf_execute_merge($method, $fields, $fillpdf, $mode = 'url', $flatt
    case 'pdftk':
      module_load_include('inc', 'fillpdf', 'xfdf'); // Looks like I'm the first actually to use this! (wizonesolutions)
      $xfdfname = $filename . '.xfdf';
-      $xfdf = createXFDF(basename($xfdfname), $fields);
+      $xfdf = create_xfdf(basename($xfdfname), $fields);
      // Generate the file
      $xfdffile = file_save_data($xfdf, $xfdfname, FILE_EXISTS_RENAME);
      // Now feed this to pdftk and save the result to a variable
@@ -523,13 +527,13 @@ function fillpdf_execute_merge($method, $fields, $fillpdf, $mode = 'url', $flatt
 function fillpdf_parse_pdf($fid) {
  $filename = db_query("SELECT url FROM {fillpdf_forms} WHERE fid = :fid", array(':fid' => $fid))->fetchField();
  $content = _fillpdf_get_file_contents($filename, "<front>");
-  $fillpdf_remote_service = variable_get('fillpdf_remote_service', true);
+  $fillpdf_remote_service = variable_get('fillpdf_remote_service', TRUE);
  $fillpdf_local_service = variable_get('fillpdf_local_service', TRUE);

  // use fillpdf-service.com's xmlrpc service (must be registered)
  if ($fillpdf_remote_service) {
    $result = _fillpdf_xmlrpc_request(DEFAULT_SERVLET_URL, 'parse_pdf_fields', base64_encode($content));
-    if ($result->error == true) {
+    if ($result->error == TRUE) {
      drupal_goto("admin/structure/fillpdf");
    } //after setting error message
    $fields = $result->data;
@@ -544,7 +548,7 @@ function fillpdf_parse_pdf($fid) {
      $fields = java_values($fillpdf->parse());
    }
    catch (JavaException $e) {
-      drupal_set_message(java_truncate((string) $e), 'error');
+      drupal_set_message(check_plain(java_truncate((string) $e)), 'error');
      drupal_goto("admin/structure/fillpdf"); //after setting error message
    }
  }
@@ -617,11 +621,11 @@ function fillpdf_execute_parse($method, $fillpdf, $mode = 'url') {
  return $fields;
 }

-function _fillpdf_get_file_contents($filepath, $error_goto = null) {
+function _fillpdf_get_file_contents($filepath, $error_goto = NULL) {
  $filepath = drupal_realpath($filepath);
-  if ($error_goto && !(file_exists($filepath))) {
-    drupal_set_message("{$filepath} does not exist.  Check your
-      filesystem settings, as well as http://drupal.org/node/764936", 'error');
+  if ($error_goto && !file_exists($filepath)) {
+    drupal_set_message(t('@filepath does not exist. Check your
+      filesystem settings, as well as http://drupal.org/node/764936', array('@filepath' => $filepath)), 'error');
    drupal_goto($error_goto);
  }
  $handle = fopen($filepath, "r");
@@ -639,17 +643,18 @@ function _fillpdf_xmlrpc_request($url, $method) {
  $ret = new stdClass;
  if (isset($result['error'])) {
    drupal_set_message($result['error'], 'error');
-    $ret->error = true;
+    $ret->error = TRUE;
  }
-  else if ($result == false || xmlrpc_error()) {
+  elseif ($result == FALSE || xmlrpc_error()) {
    $error = xmlrpc_error();
-    $ret->error = true;
-    drupal_set_message("There was a problem contacting the Fill PDF service.
-        It maybe be down, or you may not have internet access.  [ERROR {$error->code}: {$error->message}]", 'error');
+    $ret->error = TRUE;
+    drupal_set_message(t('There was a problem contacting the Fill PDF service.
+      It may be down, or you may not have internet access. [ERROR @code: @message]',
+      array('@code' => $error->code, '@message' => $error->message)), 'error');
  }
  else {
    $ret->data = $result['data'];
-    $ret->error = false;
+    $ret->error = FALSE;
  }
  return $ret;
 }
@@ -658,7 +663,7 @@ function _fillpdf_xmlrpc_request($url, $method) {
 * Retrieve the PDF's fields.
 */
 function fillpdf_get_fields($fid) {
-  $result = db_query('select * from {fillpdf_fields} where fid = :fid', array(':fid' => $fid));
+  $result = db_query('SELECT * FROM {fillpdf_fields} WHERE fid = :fid', array(':fid' => $fid));
  $return = array(
    'pdf_key' => '',
    'label' => '',
@@ -691,7 +696,7 @@ function _fillpdf_process_destination_path($destination_path, $token_objects) {
  foreach ($types as $type) {
    $destination_path = token_replace($destination_path, array($type => $token_objects[$type]), array('clear' => TRUE));
  }
-  if (substr($destination_path, 0, 1) == '/') {
+  if (drupal_substr($destination_path, 0, 1) === '/') {
    // No further modifications needed
  }
  else {
@@ -719,7 +724,7 @@ function _fillpdf_replacements_to_array($replacements) {
 * Apply any field value transformations defined via the UI.
 * Note that the replacement arguments need to already have been run through
 *   _fillpdf_replacements_to_array().
- * @see _fillpdf_replacements_to_array().
+ * @see _fillpdf_replacements_to_array()
 */
 function _fillpdf_transform_field_value($value, $pdf_replacements, $field_replacements) {
  if (empty($pdf_replacements) && empty($field_replacements)) {

--- a/xfdf.inc
+++ b/xfdf.inc
 <?php

 /**
- * createXFDF
+ * @file
+ * Provides functions for creating XFDF files.
+ */
+
+/**
+ * create_xfdf
 *
- * Tales values passed via associative array and generates XFDF file format
+ * Takes values passed via associative array and generates XFDF file format
 * with that data for the pdf address sullpiled.
 *
 * @param string $file The pdf file - url or file path accepted
@@ -11,7 +16,7 @@
 * @param string $enc default UTF-8, match server output: default_charset in php.ini
 * @return string The XFDF data for acrobat reader to use in the pdf form file
 */
-function createXFDF($file, $info, $enc = 'UTF-8') {
+function create_xfdf($file, $info, $enc = 'UTF-8') {
  $data = '<?xml version="1.0" encoding="' . $enc . '"?>' . "\n" .
      '<xfdf xmlns="http://ns.adobe.com/xfdf/" xml:space="preserve">' . "\n" .
    '<fields>' . "\n";