[D7] Export frontend translations

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

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.

Automated Review:

There are some typos in front_translations_remove.txt

• Line 951: seperated ==> separated
• Line 953: seperated ==> separated
• Line 1441: specfic ==> specific
• Line 1479: specfic ==> specific
• Line 1963: succesful ==> successful
• Line 1985: succesful ==> successful

Manual Review
export_frontend_strings.module

• Line 13: In your hook menu you're using the path 'admin/config/csv-translation'. Perhaps consider using 'admin/config/regional/csv-translation' to place this menu item in the "Regional and Language" settings.
• Line 143: You're using _locale_import_one_string_db() which is a "private" function of the locale module and should not be called from outside the module according to Drupal coding standards. There is probably another function in the locale module that is used as a wrapper around this function to be used in outside modules. See this page - https://drupal.org/node/70335 - and this comment - https://drupal.org/comment/3254490#comment-3254490 - for more details.

Storing string in the public file system will allow access bypass. Not all front-end strings are viewable by all people. The current implementation would extract those string and place them in a public file system.

Hacking core is not good. Please figure out a different way to implement this module's functionality. Perhaps hook into:

if (!isset($custom_strings[$options['langcode']])) {
    $custom_strings[$options['langcode']] = variable_get('locale_custom_strings_' . $options['langcode'], array());
  }

hook_menu -- Use this format: See https://drupal.org/node/300997

    'page callback' => 'drupal_get_form',
    'page arguments' => array('export_frontend_strings_import_form'),
    'access arguments' => array('administer site configuration'),
    'file' => 'export_frontend_strings.admin.inc',

It's best not to call pseudo private functions from other modules i.e. _locale_import_one_string_db(). Perhaps duplicate the code locally.

Automated Review

Best practice issues identified by pareview.sh / drupalcs / coder

Manual Review

• Duplication
• No
• Multiple applications
• No
• README.txt
• Follows the guidelines for in-project documentation.
• Master Branch
• Follows the guidelines for master branch.
• 3rd party code
• Follows the guidelines for 3rd party code.
• Individual user account
• Follows the guidelines for individual user accounts.
• Code too short/too simple
• Follows the guidelines for project length and complexity.
• Licensing
• No
• Security
• No security issues identified.
• Coding style & Drupal API usage

  •   $form['csv_file_download']['download_markup'] = array(
        '#markup' => '<p>' . t('Strings collected: @count', array('@count' => $collected_count)) . '</p>' . l(t('Download'), 'admin/config/regional/csv-translation/download'),
      );

    Rather than concatenating the link, insert it into the translated string as a variable replacement.

  • export_frontend_strings_import_form
    There's no validation handler to confirm the file uploaded is a csv file. That would be a good thing.

  • function export_frontend_strings_install() {
      variable_set('export_frontend_strings_collected', array());
      variable_set('export_frontend_strings_exclude', array());
    }

    No need to set the values in hook_install, rather use default values from the variable_get.

  • export_frontend_strings_preprocess_html
    I'm not able to understand what is going on with the js logic in here. Could you provide some comments?

  • _export_frontend_strings_import_one_string_db()
    ...
    $exists = db_query("SELECT COUNT(lid) FROM {locales_target} WHERE lid = :lid AND language = :language", array(':lid' => $lid, ':language' => $langcode))->fetchField();
    

    The array should wrap the values onto a new line.

  Starred items (*) are fairly big issues and warrant going back to Needs Review. The rest of the comments in the code walkthrough are recommendations.

Extra Branch

It appears you are working in a "dev" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.

There are no release blockers. Moving to RTBC.

Blocking issues (performance):

1. What are you trying to do in export_frontend_strings_init()? This function will be executed on every non-cached page request, is it safe to modify locale's static like that? The same applies to export_frontend_strings_preprocess_html(), that gets called on most page renders, surely your module's code doesn't need to affect every page request?

Non-blocking issues:

• You don't need to set variables in export_frontend_strings_install(), just provide a default when calling variable_get().
• Is export_frontend_strings_import_form_submit() assuming 'en' is the base language? This might not always be the case
• You may be able to use db_merge() in _export_frontend_strings_import_one_string_db() to simplify some of the insert/update code

Potentially blocking (duplication):

• How does this module differ from other translation import/export modules like potx?

Automated Review

Found errors in automated review: http://pareview.sh/pareview/httpgitdrupalorgsandboxrang5012184179git

Manual Review

Coding style & Drupal API usage

Found 2 warnings and 1 fatal error, with this can not be review more

re #14: Can you be more specific in your review on the fatal error? It isn't clear what action you wish from @rang501.

re #15: for example this:
in this function "export_frontend_strings_preprocess_html"

$merge = array_keys($diff);
+if(!is_array($merge)) {
+ $merge = array($merge);
+}
_export_frontend_strings_merge($merge);