Google Translate Gadget

Manual Review:

• Only README.txt file must be on master branch. See step 5 in http://drupal.org/node/1127732
• LICENSE.txt is not necessary. Drupal.org will add it automatically on packaging
• In function google_translate_gadget_admin_settings_submit on google_translate_gadget.admin.inc, you must comment function purpose and add params description. See guidelines: http://drupal.org/node/1354#functions
• It's not necessary to declare files[] on google_translate_gadget.info if there isn't class definition or interface.
• Format of hook comment definition is not correct on google_translate_gadget.module line 46. See guidelines: http://drupal.org/node/1354#hookimpl

I get a empty folder using

git clone http://git.drupal.org/sandbox/zenlan/1524286.git google_translate_gadget

.
Maybe your last git commit didn't succeed.

- Remove unnecessary new lines after function name - like:

function google_translate_gadget_uninstall() {

  variable_del('google_translate_gadget_code');
}

- Put the description in google_translate_gadget_menu() in t() function.

- Instructions in README.txt are good and helpful. Didn't had any problems setting up the module.

- You can use placeholders to put this whole text in one t() functions:

  $description = t('Grab the code from') . ' ' .
    l(t('Google Translate Gadget'),
    'http://translate.google.com/translate_tools',
    array('attributes' => array('target' => '_blank'))) . ' ' .
    t('and paste it below') . '<br /> ' .
    t('CSS class is block-google-translate-gadget') . '<br /> ' .
    t('Id is block-google-translate-gadget-gadget');

Zenlan,

I dig the simplicity of this module!

And my experiments with it on my local box worked flawlessly.

If I were going to pick gnits, here's the stuff I'd suggest:

Use 'system settings form' as opposed to a plain old 'return $form'. This allows for more end-user messaging regarding the form being saved. (After pasting in the google translate js and hitting 'save', nothing really alerted me that the page had been submitted. Seems like the drupal community prefers mucho end-user friendliness.....so I'm just sayin.) Also, using 'system settings form' would add another button with the good-old 'reset to defaults' button allowing end-users to quickly kill that variable.

Also, if you used 'hook_permission', you could allow finer-grained access control as opposed to the blanket 'administer site configuration' permission. (And since you're essentially allowing javascript embedded into the site, you might want to keep this in mind since it could be considered bad for site security.)

Finally, there isn't any validation on form submission either. If you did a "function google_translate_gadget_admin_settings_validate" you could keep users from putting in all types of unwanted stuff (or make sure they're entering in wanted stuff).

But yeah, easy-peasy-lemonsqueazy. Nice work.

Great and simple module.

1 - Unnescary space on line 47 of admin.inc

The only other thing I would suggest is adding the config path to your README.txt for clarity (/admin/config/regional/google_translate_gadget & Configruation > Google Translate Gadget Settings)

Also, I'm not aware of the licensing restrictions with the div + script provided by google, but allowing a freform box for someone to paste unchecked javascript into could be a security loophole. So I'd reccomend using hook_permission as suggested above.

PAReview.sh suggests:

sites/all/modules/pareview_temp/./test_candidate/google_translate_gadget.admin.inc:
+18: [normal] The $string argument to t() should not begin or end with a space.

Status Messages:
Coder found 2 projects, 2 files, 1 normal warnings, 0 warnings were flagged to be ignored

FILE: .../modules/pareview_temp/test_candidate/google_translate_gadget.admin.inc
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
43 | ERROR | Missing comment for param "$form" at position 1
44 | ERROR | Missing comment for param "$form_state" at position 2
--------------------------------------------------------------------------------

Nice reviews, thanks!

manual review:

1. google_translate_gadget_settings_form_submit(): why do you need that function? system_settings_form() is supposed to save the value for you?
2. What is the purpose of your module? I could just add a custom block on my site and paste the snippet from google there? Your module does not really do more, right?
3. google_translate_gadget_permission(): if I have that permission I can insert arbitrary script code into the variable, which means I could do XSS attacks, which means this is a site p0wning permission. Therefore the permission has to be marked as restricted. Basically the same security implications as using a filter format that allows arbitrary HTML and javascript.
4. This project is too short to approve you as git vetted user. We are currently discussing how much code we need, but everything with less than 120 lines of code or less than 5 functions cannot be seriously reviewed. However, we can promote this single project manually to a full project.

So I really don't think this module is suitable for this review process. You have another sandbox, maybe that one is more appropriate for review? If so then please close this application as won't fix and open a new one for your other project.

Removing review bonus tag.