Closed (fixed)
Project:
Drupal.org security advisory coverage applications
Component:
module
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Reporter:
Created:
14 Feb 2012 at 19:47 UTC
Updated:
31 Jul 2012 at 14:26 UTC
Jump to comment: Most recent file
Comments
Comment #1
svenaas commentedAs a fellow project applicant I thought I'd pull my weight and take a look. For starters I ran your project through ventral's online PAReview code checker. It found these problems:
I took a look at your code by hand as well. The module is pretty straightforward, using a couple of configuration settings to add a chunk of JavaScript to the bottom of the page. I do not see any obvious problems. I was pleased to see the t() calls where I expected them (especially given the nature of the project).
Comment #2
Robertas commentedMarking as "needs work".
Comment #3
berdyshev commentedWe have fixed all this issues. Please review again.
Comment #4
targoo commentedhere my manual review
62 '#description' => t('If you don\'t have FormID, you should create new feedback form on www.onehourtranslation.com.'),
1) t() should only contain labels (no links)
2) do not use \'
57 '#default_value' => variable_get('mlft_enabled', 0),
3) // Form API will NOT sanitize the '#options' attribute on other elements than select boxes.
http://drupal.org/node/28984
4) You define getter but not setter. Why ?
variable_get('mlft_enabled', 0)
I am not sure the module offer yet all the features...
please check http://groups.drupal.org/node/195848
Comment #5
patrickd commented@targoo
thank you for your efforts in the issue queue, but using more
<code>tags for code would be great ;-)Comment #6
DavidS commented