D7 Privatemsg Translator

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxKennethLancaster23886...

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

Hi Kenneth Lancaster,

Thank you for your efforts, I did some manual review of the code however didn't install it yet. Please find my comments:

Automated Review

Review of the 7.x-1.x branch (commit 65b6913):

There are some errors reported by automated review tools, did you already check them? See
http://pareview.sh/pareview/httpgitdrupalorgsandboxkennethlancaster23886...

Manual Review

Individual user account

[Yes: Follows] the guidelines for individual user accounts.

No duplication

[Yes: Does not cause] module duplication and/or fragmentation.

Master Branch

[No: Does not follow] the guidelines for master branch.

Licensing

[Yes: Follows] the licensing requirements.

3rd party assets/code

[Yes: Follows] the guidelines for 3rd party assets/code.

README.txt/README.md

[Yes: Follows] the guidelines for in-project documentation and/or the README Template.

Code long/complex enough for review

[Yes: Follows] the guidelines for project length and complexity.

Secure code

[Yes: Meets the security requirements.]

Coding style & Drupal API usage

[List of identified issues in no particular order. Use (*) and (+) to indicate an issue importance. Replace the text below by the issues themselves:

1. (*) Do not add .module in .info And .info file should end with new line.
2. (*) privatemsg_translator_admin_settings_form doc: is wrong.
3. (*) Unnecessary function doc is declared on line 95-98
4. (*) Variable assign should have single space before and after "=", see line(41, 43, 66, 68 and so on)
5. (*) Unnecessary parenthesis added. see line (130, 131, 142, 143 and so on)
6. (*) Use check_plain / t() on line 142, 143, 222 and 227.
7. (*) Do not use db_query() instead use db_select().
8. (*) Unused variable ($result) is declared on line 184 and 193.
9. (+) Try to use drupal_add_js/ Move javascript code to .js file. see line 178-180.
10. (+) Check curl is enabled or disabled using function_exists() in hook_requirements().

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This review uses the Project Application Review Template.

Hi Kenneth Lancaster,

privatemsg_translator_admin_settings_form doc: is wrong.
function name doesn't match with your comment doc.

use check_plain / t() on the lines you were indicating
perfect if it is already cleaned then no need to clean it again.

Do not use db_query() instead use db_select()
Yes i know db_query is faster than db_select, But i think you need to follow drupal coding standards like at some places you used db_select and at some places db_query. You can use db_query if query is more complex and affecting the performance using db_select.

Thanks.

@Kenneth: Hello, i have few notices which can help make your module be more better:

1. Take away module settings links to separate page, just add

$items['admin/config/privatemsgtranslator'] = array(
   ...
   'page callback' => 'system_admin_menu_block_page',
   'file' => 'system.admin.inc',
   'file path' => drupal_get_path('module', 'system'),
  );

2. Use system_settings_form to save forms variables

/**
 * Implements privatemsg_translator_admin_settings_form().
 */
function privatemsg_translator_admin_settings_form($node, &$form_state) {
  ...
  return system_settings_form($form);
}

/**
 * Process a validated privatemsg_translator_admin_settings_form_submit().
 */
function privatemsg_translator_admin_settings_form_submit($form, &$form_state) {
  $form_state['rebuild'] = TRUE;

  // Notify user.
  drupal_set_message(t('Privatemsg Translator settings have been saved.'));
}

This has been sitting unreviewed in the queue for nine months because the project owner assigned the review task to himself.

It is not a good idea to assign the issue to yourself if you want other people to review it. Please see this page about assigning ownership to issues.

Also, making it necessary to obtain a Google Translate api key to be able to test the project may limit the number of potential reviewers. You should provide some alternative (e.g. a free API that simulates Google's) for people who just want to test (and don't care if the "translation" is just changing text into uppercase).

Hi Kenneth Lancaster,

privatemsg_translator.module line 127

    if ($privatemsg_translator_recipient_lang_code == $privatemsg_translator_author_lang_code) {
      next($recipient);
    }
    else {

I think instead of the next you should use continue to skip the current item processing in the foreach loop.
Otherwise many of referenced variables will not be available under the code like $response_code, $response_decoded, etc which are initialized within the else condition block.

From the line 143 you have two blocks which do almost same things (if block is duplicated to else block) I think it would be great (for maintainability and performance) to organize them to a common function.

Line 172:

    if ($response_code != 200) {
      // Erorr message received, no translation received.
      echo '<script language="javascript">';

I think you shouldn't use echo to display any error, instead you can use the drupal_set_message or similar functions.
'alert(" Fetching translation failed! Server response code:' should be translatable -> t()

There are some typos in the code:
for instance
line:137 ...suject...
line:145 ...transalation
line:173 ..Erorr...

lots of missing parameter type from doc comments

Hello Kenneth Lancaster,

In file privatemsg_translator.module, line no. 186

$result_message = db_insert('privatemsg_translator_msg')->fields(array(
        'privatemsg_translator_mid' => $privatemsg_translator_mid,
        'privatemsg_translator_thread_id' => $privatemsg_translator_thread_id,
        'privatemsg_translator_recipient_id' => $privatemsg_translator_recipient_id,
        'privatemsg_translator_lang_code' => $privatemsg_translator_recipient_lang_code,
        'privatemsg_translator_message' => $privatemsg_translator_message,
      ))->execute();

It should be as

$result_message = db_insert('privatemsg_translator_msg')->fields(array(
        'privatemsg_translator_mid' => $privatemsg_translator_mid,
        'privatemsg_translator_thread_id' => $privatemsg_translator_thread_id,
        'privatemsg_translator_recipient_id' => $privatemsg_translator_recipient_id,
        'privatemsg_translator_lang_code' => $privatemsg_translator_recipient_lang_code,
        'privatemsg_translator_message' => $privatemsg_translator_message,
      ));
	  
$result_message->execute(); 

Or you can change it as follows:

db_insert('privatemsg_translator_msg')->fields(array(
        'privatemsg_translator_mid' => $privatemsg_translator_mid,
        'privatemsg_translator_thread_id' => $privatemsg_translator_thread_id,
        'privatemsg_translator_recipient_id' => $privatemsg_translator_recipient_id,
        'privatemsg_translator_lang_code' => $privatemsg_translator_recipient_lang_code,
        'privatemsg_translator_message' => $privatemsg_translator_message,
    ))->execute();

Same change at line no. 193 for $result_subject.

Thank you,

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

I'm a robot and this is an automated message from Project Applications Scraper.