Settings page behavior is confusing

Approved and thank you! This is a longstanding inconsistency.

+++ b/fillpdf.admin.incundefined
@@ -21,101 +21,74 @@ define('FILLPDF_REPLACEMENTS_DESCRIPTION', t("<p>Tokens, such as those from CCK,
+  // Convert legacy configuration variables to new fillpdf_service variable and delete.
+  $default = FALSE;
+  global $conf;
+  foreach (array('fillpdf_remote_service', 'fillpdf_local_service', 'fillpdf_local_php') as $variable_name) {
+    if (isset($conf[$variable_name])) {
+      if ($conf[$variable_name]) {
+        $default = $variable_name;
+      }
+      variable_del($variable_name);
+    }
+  }
+  if ($default) {
+    $variable_name_map = array(
+      'fillpdf_local_php' => 'pdftk',
+      'fillpdf_local_service' => 'local',
+      'fillpdf_remote_service' => 'remote',
+    );
+    variable_set('fillpdf_service', $variable_name_map[$default]);
+  }
+
+  // Assemble service options. Warning messages will be added next as needed.
+  $options = array(
+    'pdftk' => t('Use locally-installed pdftk: You will need a VPS or a dedicated server so you can install pdftk (') . l(t('see documentation'),  'admin/help/fillpdf') . t(').'),
+    'local' => t('Use locally-installed PHP/JavaBridge: You will need a VPS or dedicated server so you can deploy PHP/JavaBridge on Apache Tomcat (') . l(t('see documentation'),  'admin/help/fillpdf') . t(').'),
+    'remote' => t('Use fillpdf-service.com: Sign up for <a href="http://fillpdf-service.com">Fill PDF as-a-service</a>.'),
+  );
+
+  // Check for JavaBridge.
+  if (!(file_exists(drupal_get_path('module', 'fillpdf') . '/lib/JavaBridge/java/Java.inc'))) {
+    $options['local'] .= '<div class="messages warning">' . t('JavaBridge is not installed locally.') . '</div>';
+  }
+
+  // Check for pdftk.
+  $output = array();
+  $status = NULL;
+  exec('pdftk', $output, $status);
+  if (in_array($status, array(126, 127))) {
+    $options['pdftk'] .= '<div class="messages warning">' . t('pdftk is not properly installed.') . '</div>';
+  }

I think this is too overarching as it stands. We should only manipulate variables in a proper update hook - otherwise, this code will be hanging around for versions to come. I say we kill this or move it into fillpdf.install as an implementation of hook_update_N().

+++ b/fillpdf.admin.incundefined
@@ -21,101 +21,74 @@ define('FILLPDF_REPLACEMENTS_DESCRIPTION', t("<p>Tokens, such as those from CCK,
-    '#title' => t('Use SSL?'),
-    '#description' => t('It is recommended to select <em>Yes</em> for this option. Doing so will help prevent
-      sensitive information in your PDFs from being intercepted in transit.'),
-    '#default_value' => variable_get('fillpdf_remote_protocol', 'http'),
+    '#title' => t('Protocol Security'),
+    '#description' => t('It is recommended to select <em>Use HTTPS/SSL</em> for this option. Doing so will help prevent sensitive information in your PDFs from being intercepted in transit between your server and the remove service.'),
+    '#default_value' => variable_get('fillpdf_remote_protocol', 'https'),
     '#options' => array(
-      'http' => t('No'),
-      'https' => t('Yes'),
+      'https' => t('Use HTTPS/SSL'),
+      'http' => t('Do not use HTTPS/SSL'),

These are string changes and don't have to do with the behavior of a page. File a separate issue.

+++ b/fillpdf.admin.incundefined
@@ -193,7 +166,7 @@ function fillpdf_forms_admin_submit($form, &$form_state) {
+    drupal_set_message(t('<strong>%filename</strong> was successfully uploaded.', array('%filename' => $file->filename)));

Relevant issue, but not in the scope of this patch.

+++ b/fillpdf.admin.incundefined
@@ -216,7 +189,7 @@ function fillpdf_forms_admin_submit($form, &$form_state) {
+    drupal_set_message(t('Error saving file to %dir.', array('%dir' => $dir)), 'error');

Relevant issue, but not in the scope of this patch.

+++ b/fillpdf.admin.incundefined
@@ -246,7 +219,7 @@ function fillpdf_form_edit($form, &$form_state, $fid) {
+    '#collapsed' => TRUE,

Relevant issue, but not in the scope of this patch.

+++ b/fillpdf.admin.incundefined
@@ -256,7 +229,7 @@ function fillpdf_form_edit($form, &$form_state, $fid) {
+    '#description' => l(t('See which fields are which in this PDF'), fillpdf_pdf_link($fid, NULL, NULL, TRUE)) . '<br />' .

Relevant issue, but not in the scope of this patch.

+++ b/fillpdf.admin.incundefined
@@ -372,7 +345,7 @@ function fillpdf_form_edit_submit($form, &$form_state) {
+    drupal_set_message(t('Successfully updated form'));

Relevant issue, but not in the scope of this patch.

+++ b/fillpdf.admin.incundefined
@@ -408,7 +381,7 @@ function fillpdf_form_delete_confirm_submit($form, &$form_state) {
+  drupal_set_message(t('Your form has been deleted.'));

Relevant issue, but not in the scope of this patch.

+++ b/fillpdf.admin.incundefined
@@ -602,10 +575,10 @@ function fillpdf_field($op, $fid, $pdf_key = NULL) {
-  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();
@@ -767,7 +740,7 @@ function fillpdf_field_delete_confirm_submit($form, &$form_state) {

@@ -767,7 +740,7 @@ 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.');

Relevant issue, but not in the scope of this patch.

+++ b/fillpdf.moduleundefined
@@ -518,38 +519,37 @@ function fillpdf_execute_merge($method, $fields, $fillpdf, $mode = 'url', $flatt
-  //create fields

Relevant issue, but not in the scope of this patch.

This patch overall will need tests too.

This works, and I like the way you've done it this time. We probably should have put the pdftk check into another issue, but eh, I don't really mind. This is a broad issue anyway.

Committed to 7.x-1.x. Needs backport to 6.x-1.x when there's a chance.

#1430390: Coding standards: remove whitespace broke this one too.

I will test this and if it works commit it, and resolve the whitespace issue.