[D7] Yandex.Captcha

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

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.

Change links to the Yandex captcha examples

I'm attaching links to each file.

• http://drupalcode.org/sandbox/oboz/2075395.git/blob/HEAD:/yandex_captcha...

68       'description' => t('Administer Yandex.Captcha settings'),

Missing space, and missing full-stop on the end of the sentence. Maybe it better be reworded as:

       'description' => t('Administer Yandex Captcha settings.'),

 159     watchdog('Yandex.Captcha', 'Unable to connect with the Yandex CleanWeb server: @errno: @errstr', array(
 160       '@errno' => $request->code,
 161       '@errstr' => $request->status_message,
 162     ), WATCHDOG_ERROR);

instead of @variable use %variable which indicates that the string should be HTML escaped and highlighted with theme_placeholder() which shows up by default as emphasized. Read more at: https://api.drupal.org/api/drupal/includes%21common.inc/function/t/6 (unfortunately the D7 documentation doesn't explain the differences)
The same thing happens in other places in your code, too.

.

• http://drupalcode.org/sandbox/oboz/2075395.git/blob/HEAD:/yandex_captcha...

  19     '#description' => t('The key given to you when you register on Yandex services and get it <a href="@url" target="_blank">in API documentation area</a>.', array('@url' => 'http://api.yandex.ru/key/form.xml?service=cw')),

Maybe here you should use '!url' and l() here.

Sorry, somehow I got rid of the bonus review here.

I don't see other problems in the code.

I have reviewed the code and here is what I found:

1. "variable_get('yandex_captcha_key', ...)" - not consistent, sometimes you use FALSE, sometimes NULL, mostly an empty string '' - it should be the same default value every time ;)
2. 3 functions - yandex_captcha_get_captcha(), yandex_captcha_check_captcha() and yandex_captcha_ping_server() - contain the same piece of code - call to external URL with some params, checking the response code and setting the same error message - it might be good to move this code to another function (yandex_http_request? :)) and just call it with params for the external URL.
3. As I see, you fallback to the Math CAPTCHA in the yandex_captcha_captcha(), but you don't do it when refreshing the CAPTCHA via AJAX call (inconsistency).

^^ These are minor issues so leaving it as RTBC. The module is usable and ready to go, all these things can be improved later in the issue queues.

Below are some general thoughts on the CAPTCHA refresh functionality (probably related to the CAPTCHA modules suite overall, not only to this module application):

1. I don't like that the captcha/yandex_captcha_refresh menu entry is not protected. It leads to the yandex_captcha_ajax_refresh() function which unconditionally calls the _captcha_generate_captcha_session() and this function does db_insert(). So it seems like anyone can send GET requests to this URL which will cause code processing and DB load (and then a call to the external service - see #3 below).
2. It calls the CAPTCHA module internal function _captcha_generate_captcha_session() - is there another way? meaning without using this "_" prefixed function?
3. It then calls the yandex_captcha_get_captcha() which does drupal_http_request() to the external service. (Again, not-protected menu entry causing the site to do processing and wait for the external service answer.)
4. The yandex_captcha_ajax_refresh() function after calling the _captcha_generate_captcha_session() function does the db_update() on the captcha_sessions DB table - is there another way? We shouldn't be doing any direct DB updates on the other modules data, there should be an API for this.

Seems like there is an opportunity here to write some general CAPTCHA refresh solution that will work for all CAPTCHA types and will use better approach.

P.S. It shouldn't be assigned to the applicant, right? Now we should wait for Git admin to take a look on this issue, will try to find one online.

manual review:

1. "$GLOBALS['conf']['cache'] = FALSE;": do you want to mark the response as not cacheable here? Then use drupal_page_is_cacheable(FALSE);
2. yandex_captcha_captcha(): do not build image markup manually, use theme('image', ...) instead. That will also ensure with drupal_attributes() that the URL does not contain any malicious characters or script tags, as it uses check_plain() to sanitize the output.
3. "Drupal.behaviors.imageCaptchaRefresh": that should be your module name, something like Drupal.behaviors.yandexCaptchahRefresh.

But that are not blockers, so ...

Thanks for your contribution, oboz!

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.