niccottrell [niccottrell]

Hi,

Sounds promising.

Please could you add here the required comparison with any existing Drupal modules. If none, at least the nearest few in a similar domain (I suppose translation) with why they don't do at all what you've done.

Hello, and thank you for applying for a CVS account. I am changing the status as per previous comment, and adding the review tags.

This review is only partial.

1. The JavaScript file doesn't make use of jQuery.

2.   $form['sprawk'] = array(
       '#type' => 'fieldset',
       '#title' => t('Sprawk Team Configuration'),
       '#description' => t('These settings should correspond to an existing Sprawk team registered at the !url website.', 
         array('!url' => l("sprawk.com", 'http://www.sprawk.com/'))) . ' ' . 
         t('If you don\'t have a Sprawk team account, !url for free.',  array('!url' => l(t('register now'), "http://www.sprawk.com/en/auth/signupTeam.action"))) ,
       '#collapsible' => FALSE
     );
   

   Avoid to escape the string delimiter inside a string, especially if the string is passed to t().
   Using l() in that way is not correct, as reported in the documentation for t(), which suggests to use code similar to the following:

     t('See the <a href="@t-doc">t() documentation</a>.', array('@t-doc' => 'http://api.drupal.org/api/drupal/includes--common.inc/function/t/6'));
   

3. drupal_add_js(drupal_get_path('module', 'sprawk') . '/sprawk.js');
   drupal_add_js(drupal_get_path('module', 'sprawk') . '/cache.js');
   drupal_add_css(drupal_get_path('module', 'sprawk') . '/sprawk.css');
   

   That code should go inhook_init(), or (better) in any form builder where those files are required. That would avoid to load them in pages handled by other modules.

4.   $form['sprawk']['test_pass_div'] = array(
       '#type' => 'markup',
       '#value' => '<div class="testpass">' .
           '<input type="button" onclick="sprawk.testpass();" value="' . t("Test connection") . '" />' .
           '&nbsp;<span id="testpassresult">&nbsp;</div>',
     );
   

   The code should use Drupal behaviors.

5.   $form['sprawk']['clear'] = array(
       '#prefix' => '<b>Clears Sprawk Cache</b><br>There are ' . number_format($data->total) . ' items currently in cache<br>',
       '#type' => 'submit',
       '#value' => t('Reset cache'),
       '#submit' => array('sprawk_clear_cache_submit'),
     );
   

   Strings shown in the user interface should be translated (in that case, consider using t()-placeholders).
   <b>, and <br> are not tags that should appear in XHTML output (which is the output returned from Drupal).

6.   $form['sprawk']['ext_link'] = array(
       '#type' => 'markup',
       '#value' => '<div style="margin-top:16px;">For free assistance configuring sprawk with Drupal contact <a href="mailto:support@sprawk.com">support@sprawk.com</a> or visit <a target="_blank" href="http://www.sprawk.com/help/">sprawk.com/help</a></div>',
     );
   

   CSS styles should be applied using a class or a HTML ID. URLs, and mail addresses should be passed in the string (which needs to be translated) through t()-placeholders.

7.   $form['sprawk']['testing'] = array(
       '#type' => 'markup',
       '#value' => '<!-- ' . sprawk_testEncoding() . ' -->',
     );
   

   Why isn't the code using an hidden form field?

8.       $form[$fieldset][$field . '_to_lang'] = array(
           '#title' => t("Translate from %s to: ", $fromlang),
           '#type' => 'select',
           '#value' => $tolang_code,
           '#options' => array_merge($languages),
           '#attributes' => array(
             'onchange' => 'sprawk_aproxtimer("' . $element . '");'
           ),
           '#description' => '<div id="sprawk_approxtime_' . $element . '"></div>',
         );
   

   The first argument of t() is a literal string; any dynamic value (including the result of concatenating two strings) is not translated (which means that calling t() with a dynamic value is like not calling it at all).
   The reason is that the strings to translate are found from a script that looks for any strings passed to t(), and save them in the translation template. That script is not able to get the value of a variable, as the module is not executed to get such values.

   Events are normally attached to HTML elements using Drupal behaviors.

9. Remove any debugging code, including the commented out one.

Hello, niccottrell. I am sorry nobody made any further review of the code, and you had to wait.

1. Files available from third-party sites should not be committed in Drupal.org repository.
   The license of the JavaScript file is then different from GPL license, which the license you agree to use for the files you commit to the repository.

   /*
    MIT LICENSE
    Copyright (c) 2007 Monsur Hossain (http://www.monsur.com)
   
    Permission is hereby granted, free of charge, to any person
    obtaining a copy of this software and associated documentation
    files (the "Software"), to deal in the Software without
    restriction, including without limitation the rights to use,
    copy, modify, merge, publish, distribute, sublicense, and/or sell
    copies of the Software, and to permit persons to whom the
    Software is furnished to do so, subject to the following
    conditions:
   
    The above copyright notice and this permission notice shall be
    included in all copies or substantial portions of the Software.
   
    THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
    EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
    OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
    NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
    HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
    WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
    FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
    OTHER DEALINGS IN THE SOFTWARE.
    */
   

2. The version = line should be removed from the .info file.

3.   $form['sprawk'] = array(
       '#type' => 'fieldset',
       '#title' => t('Sprawk Team Configuration'),
       '#description' => t('These settings should correspond to an existing Sprawk team registered at the <a href="@url">sprawk.com</a> website.', 
         array('@url' => 'http://www.sprawk.com/')) . ' ' . 
         t('If you don\'t have a Sprawk team account, <a href="@url">register now</a> for free.',  array('@url' => 'http://www.sprawk.com/en/auth/signupTeam.action')) ,
       '#collapsible' => FALSE
     );
   

   The string delimiter should not be escaped inside a string, especially when the string is the argument passed to t().
   I don't see any reason to split the form field description in two; it would be better to use a single string, if both the strings are used only once in all the module code.

4.     '#prefix' => '<div class="testpass">',
       '#suffix' => '&nbsp;<span id="testpassresult"></span></div>',
   

   The module should use CSS class names that are prefixed with its short name, to avoid conflict with existing modules, including Drupal core modules.

5.   $form['sprawk']['clear'] = array(
       '#prefix' => t('There are @total items currently in cache.', array('@total' => number_format($data->total))),
       '#type' => 'submit',
       '#value' => t('Reset cache'),
       '#submit' => array('sprawk_clear_cache_submit'),
     );
   

   The prefix should probably be a separated form field, and it should use format_plural().

6.       $form[$fieldset][$field . '_tr_btn'] = array(
           '#type' => 'markup',
           '#value' => "<input class=\"sprawk-tr-button\" " .
               "type=\"button\" onclick=\"sprawktest('$edit_id');\" " .
               "value=\"Translate\" />",
         );
         $form[$fieldset][$field . '_rev_btn'] = array(
           '#type' => 'markup',
           '#value' => "<input class=\"sprawk-rev-button\" " .
               "type=\"button\" onclick=\"sprawk_revert('$edit_id');\" " .
               "value=\"Revert\" title=\"" . t("Revert to original") . "\"/>",
         );
   

   Form fields are generated through the form API.
   JavaScript events are added through Drupal behaviors.

7.   if (strlen(trim($orig)) == 0) {
       // text was blank;
      	return $orig;
     }
   

   It would be enough to use

     if (!trim($orig)) {
       // text was blank;
      	return $orig;
     }
   

8.   $params["src"] =  $base_url . request_uri(); // send the url of the page on this site to sprawk
   

   The code should probably use url().

9.           // convert entities (like accented chars) back to normal form
             $contentDec = html_entity_decode($content, ENT_NOQUOTES, 'UTF-8');
    

   Drupal has the decode_entities() function that doesn't depend from a specific PHP version.

10. See http://drupal.org/coding-standards to understand how a module should be written. In particular, see how the code should be formatted (with more attention to the control structures); see how functions defined from the module should be named.

11. watchdog('sprawk', "No fields", WATCHDOG_NOTICE);
    

    The function call is missing an argument.

12.   if (variable_get('sprawk_debug', 0)) {
        watchdog("sprawk", "fields: " . var_export($fields, TRUE));
      }
    

    Use placeholders.

13. Remove any debugging code that unconditionatelly print debugging output on the page.

14.     $result = $cache->data;
        $json = json_decode($result["output"], TRUE);
        error_log("getTopics json cache: " . var_export($json, TRUE));
    

    The function is not defined in PHP4, while Drupal 6 is still compatible with PHP4; if the module needs PHP5, then it needs to declare this dependency.

15. function sprawk_testEncoding() {
      $swedish=SPRAWK_CHK;
      $encodedTarget = "%C3%A5%C3%A4%C3%B6"; // this is what should be sent to sprawk
      $res = 'original=' . $swedish . "\n" .
        'encodedTarget=' . $encodedTarget . "\n" .
        'urlencode=' . urlencode($swedish) . "\n" .
        'urlencode(utf8_encode)=' . urlencode(utf8_encode($swedish)) . "\n" .
        'rawurlencode=' . rawurlencode($swedish) . "\n" .
        'rawurlencode(utf8_encode)=' . rawurlencode(utf8_encode($swedish)) . "\n" .
        'utf8_encode=' . utf8_encode($swedish) . "\n" .
        'utf8_decode=' . utf8_decode($swedish) . "\n";
      $language->language = 'fr';
      $res .= '_sprawk_filter_translate=' . _sprawk_filter_translate($swedish);
      return $res;
    }
    

    The $language variable is not defined; setting $language->language doesn't have the desired effect.
    I guess the code is referring to the global $language variable, even if it doesn't seem a good idea to change that variable for testing purposes without to restore its original value.

16. The module doesn't remove the Drupal variables it defines, when it gets uninstalled. (Tip: don't remove the variables using a SQL query that matches all the variables that have a name like 'sprawk_%'.)

17. function sprawk_stats_main_page() {
       $output = "<ul>\n" . 
        "<li><a href='" . url('admin/settings/sprawk/sprawk_stats/result_stats') . "'>" . t("Show Report by result group") . "</a></li>\n" .
        "<li><a href='" . url('admin/settings/sprawk/sprawk_stats/result_term') . "'>" . t("Show Report by translated term") . "</a></li>\n" .
        "</ul>\n";
      return $output;
    }
    

    In this case, it would be better to use l().

18. function sprawk_stats_by_group() {
      //$qry = "SELECT COUNT(api_date) AS calls, api_date, lang, code FROM {sprawk_log} GROUP BY api_date, code, lang ORDER BY api_date DESC";
      //$qry_count = "SELECT COUNT( DISTINCT api_date, lang, code ) FROM {sprawk_log}";
      $qry = "SELECT COUNT(api_date) AS calls, api_date, lang, code FROM {sprawk_log} WHERE uri='/api/translateHtmlSnippet' GROUP BY api_date, code, lang ORDER BY api_date DESC"; 
      $qry_count = " SELECT COUNT(DISTINCT api_date, lang, code) FROM {sprawk_log} WHERE uri = '/api/translateHtmlSnippet'";
      $r = pager_query($qry, 20, 0, $qry_count);
      while ($data = db_fetch_object($r)) {
         $table .= "<tr>\n";
         $table .= "<td>" . $data->calls . "</td>\n";
         $table .= "<td>" . $data->api_date . "</td>\n";
         $table .= "<td>" . $data->lang . "</td>\n";
         $table .= "<td>" . $data->code . "</td>\n";
         $table .= "</tr>\n";
      }
      
      $output = "<table border=0><tr><td>\n" .
       "<table cellpadding=3 border=1>\n" .
       "<tr>\n" .
      "<th>" . t("Number of Calls") . "</th><th>" . t("Date") . "</th><th>" . t("Language") . "</th><th>" . t("Code") . "</th>\n" .
      "</tr>\n" .
       $table .
       "</table><br>\n" .
       "</td></tr>\n" .
       "<tr><td>\n" .
       theme('pager', NULL, 20, 0) .
      "</td></tr></table>";
      
      return $output; 
    }
    

    The code outputs a HTML table without using a theme function that Drupal has.

19. function sprawk_stats_clear($confirm = '') {
      if ($confirm && !is_array($confirm)) {
         $qry = "TRUNCATE TABLE {sprawk_log}";
         db_query($qry);
         drupal_set_message(t("Sprawk Log information was clear: %data", array('%data' => var_export($confirm, TRUE))));
         drupal_goto('admin/settings/sprawk/sprawk_stats');
      }
      else {
        $output = t("Are you sure you want to delete Sprawk log information?") . "\n" .
          "<ul>\n";
          "<li>" . l(t("Yes"), 'admin/settings/sprawk/sprawk_stats/clear/1') . "</li>\n" .
          "<li>" . l(t("No"), 'admin/settings/sprawk/sprawk_stats') . "</li>\n" .
          "</ul>\n";
      }
      return $output;
    }
    

    There is a Drupal function to use in those cases.

Hi. Please read all the following and the links provided as this is very important information about your CVS Application:

Drupal.org has moved from CVS to Git! This is a very significant change for the Drupal community and for your application. Please read the following documentation on how this affects and benefits you and the application process:
Migrating from CVS Applications to (Git) Full Project Applications

• The status of this application will be put to "postponed" and by following the instructions in the above link, you will be able to reopen it.
• Or if your application has been "needs work" for more than 5 weeks, your application will be marked as "closed (won't fix)". You can still reopen it, by reading the instructions above.

As per previous comment, I am setting this issue to won't fix.

Since new users can now create full projects, applications have a different purpose and they are handled on a different issue queue. See Apply for permission to opt into security advisory coverage for more information.