[D7] POEditor

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

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 softescu,

Here's a quick manual review:

Instead of using hook_init to add css to every page, declare it in your .info file:
https://drupal.org/node/542202#stylesheets

In .install:

<?php
function poeditor_install() {
  variable_set("poeditor_token", NULL);
}
?>

I don't think this is necessary, since using variable_get, the default is always NULL: https://api.drupal.org/api/drupal/includes%21bootstrap.inc/function/vari...

That's all I've got time for right now, hope that helps! I'll try to come back and give a more thorough review.

softescu,

Please provide a non-maintainers git clone link in the issue summary.

Also, README.txt is missing, see the guidelines for in-project documentation.

Remove the master branch, see also step 6 in http://drupal.org/node/1127732.

In poeditor.admin.inc, If you're setting

<?php
$poeditor_token = variable_get('poeditor_token');
?>

you might as well use it on line 23 for the #default_value instead of calling variable_get() again.

Also, I noticed that you're creating

<?php
function poeditor_token() {
  return variable_get('poeditor_token');
}
?>

Which you could use instead.

<?php
$form['poeditor_configuration']['poeditor_token'] = array(
    '#type' => 'textfield',
    '#title' => t('POEditor Token'),
    '#default_value' => variable_get('poeditor_token'),
    '#size' => 60,
    '#maxlength' => 128,
    '#required' => TRUE,
  );
?>

Do you need some sort of validation for poeditor_token?

Review of the 7.x-1.x branch:

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

manual review:

1. poeditor_select_terms_original(): do not concatenate variables into SQL strings, use placeholders instead. Make sure to read https://drupal.org/writing-secure-code again.
2. poeditor_select_terms_original() looks a bit expensive, are you sure that you want to run that on every single cron run, which can be every 5 minutes on some production sites?
3. poeditor_save(): this looks MySQL or DB specific, why do you need CONVERT(source USING utf8)? Please add a comment. Does that work on other DBs, too?
4. The code for poeditor_api_language_export() and poeditor_api_list_project_languages() looks almost the same. I would suggest to add a helper function that just takes the config, performs the request and returns the decoded result to avoid so much code duplication.
5. "$projects_available[0] = '- No project selected -';": all user facing text must run through t() for translation.
6. "$i . ' ' . t('translations have been copied from POEditor.')": do not concatenate variables to t(), use placeholders with t() instead.
7. poeditor_process_form(): this is vulnerable to CSRF exploits. Do not execute actions on GET requests without a prior confirmation form or security tokens. See also http://epiqo.com/en/all-your-pants-are-danger-csrf-explained . And please don't remove the security tag, we keep that for statistics and to show examples of security problems.

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

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

Sorry for the delay. Make sure to review more project applications and get a new review bonus and this will get finished faster.

manual review:

1. poeditor_cron(): inline comments are missing, what is happening in which part?
2. poeditor_get_token(): just a copy of drupal_get_token(), so this function is not necessary? Same for poeditor_valid_token(), just use the Drupal API functions?
3. poeditor_delete_confirm_form(): This is still vulnerable to CSRF exploits. As long as 'confirm' is set in the POST request the delete operation will be executed without any validation. The delete action should only be executed in a submit handler of the confirm form, not during form building. See also https://drupal.org/node/178896

Otherwise looks pretty good, the CSRF issue is a blocker right now.

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

Other applications reviewed

Removed automated review comments from the issue summary.

manual review:

1. poeditor_get_token(): just a copy of drupal_get_token(), so this function is not necessary? Same for poeditor_valid_token(), just use the Drupal API functions?
2. poeditor_process_form(): This is still vulnerable to CSRF exploits. If you want to validate a token you need to get it out of the request, not by computing it yourself. You need to add the token for example in poeditor_compare_lang_add() as query parameter to the link or in the menu path to make it useful.

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

• In hook_cron() you have some duplicated code, both cases of if (empty($cron_poeditor_date)) {...} else {...} have the same 2 if statement checks inside.
• Consider using entity field queries in poeditor_select_terms(), they would be a lot shorter and the query object could be used in both cases with only slight modification. Similarly in poeditor_select_terms_original()
• Can you comment on why you're using CONVERT() in poeditor_save()? That does seem like a mysqlism.

Those don't seem like major issues though.

----
Top Shelf Modules - Crafted, Curated, Contributed.

manual review:

1. No need for token verification in poeditor_form() since you are not performing any database changing write operations there, you just return a confirm form.
2. poeditor_api_add_terms(): don't pollute your API functions with CSRF tokens - the validation should be handled outside in menu callbacks. Same for poeditor_save() and possibly others. You don't need to validate CSRF tokens in form submit handlers, because the form API has already verified the form CSRF token. You only need to protect places where you execute write operations on GET requests, i.e. where the user did not submit a form with CSRF protection to initiate the action.
3. "poeditor_count_terms($lang->language) . ' ' . t('of') . ' ' . $termsindrupal;": do not concatenate translatable strings into t(), use placeholders with t() instead. Also elsewhere.

Although you should definitely fix those issues I agree that they are not critical blockers, so ...

Thanks for your contribution, softescu!

I updated your account so you can promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and stay involved!

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.