Correct.li

Welcome,

Please add the direct link to your sandbox repository in the issue summary.

We really need more hands in the application queue and recommend to get a review bonus so we can review your application sooner :)

While waiting for an in-depth review of you module, you can start fixing some code styling issues detected by automatic tools:
http://ventral.org/pareview/httpgitdrupalorgsandboxklopsen1606640git

Regards

Just looked at your code and found this problems:

• In file correct_li.admin.inc, drupal does not have hook_admin_settings(). correct_li_admin_settings_form() is the form callback and it should get 2 parameters, $form and &$form_state
• The string passed to t() function should not include any html tags, take a look at http://api.drupal.org/api/drupal/includes!bootstrap.inc/function/t/7
• Default value of the second parameter of variable_get() is NULL, so there is no need to pass an empty string as the second argument.
• Don't use $_GET['q'], instead you can use request_uri() function.

I found some other problems in your code that I'm not sure about their solutions.. Let's wait for a more experienced developer to jump in! :)

Regards

Hi there,

Firstly, nice work! Well comment tidy code, very readable.

You've still got a .info file in your master branch (I noticed when I mucked up the git clone).

Manual review of the 7.x-1.x branch:

• correct_li.info has the line configure = admin/config/system/correctli but your hook_menu() defines the path as admin/config/system/correct_li.
• In correct_li_help() function the single case clause in your switch is not broken at the end. I know it contains a return but for completeness sake ...
• Would it be possible to use drupal_add_js() on line 68 instead of directly putting the markup in?

I did a quick install and click around - just a couple of suggestions:

• Would it be more sense for the config settings to live inside Web Services like where you find twitter and fb like modules, RSS publishing, etc. I think the path would need to be admin/config/services/correct_li instead
• Admin page is nice and straight forward and self explanatory and fields have good help text - only suggestion is pop a link to correct.li in the api help text so it reads Site API key provided after registration at Correct.li site

Other than those few things, module works great, installs and uninstalls without hiccup using drush. Good work!

Hi again,

A few more points.
I'm not a big JS user but concerned about the drupal_add_js calls you've added still, have a look here http://drupal.org/node/304258 it seems to shed a lot of light on best practice for including Javascript including php variables. Or there's the general page here http://drupal.org/node/756722.

In correct_li.admin.inc line 25: you could do these links by just putting a placeholder in the t() and using an l() in the parameters rather than hard coding the link tag inside string literal. Also there's no replacement value for !panels in the parameters.
Line 51 also with the l() thing
And again line 135

Other than that I'd say that this is damn good!

Please, remove everything from master branch and set 7.x-1.x branch as default on version control page

.install file

You do not need to set variable_set('correct_li_language', 'en'); on hook_install. It's what second element is for - to provide default value if variable is not set.
Some if structures also check if variable is empty. It probably won't be, because you are setting the second parameter.

.module file

 if (!empty($api_key) &&
  _correct_li_visibility_pages() &&
  _correct_li_visibility_user($user)) 

You may place all the conditions in one line. 80 charecter limit is only for comments and array multiple elements.

Drupal coding standards tells us to use underscore instead of camel case in variable names.
Your $correct_li settings does not follow it.

drupal_add_js('http://correct.li/api?api=' . $api_key
    . '&language=' . $correct_li_language, 'external');

Some pages might have https:// protocol. Browsers will alert users about unsecure content. If correct.li has it's https version you probably can add //correct.li without protocol, hovewer I'm not sure if Drupal will allow this.

I'm not sure about php_eval, but you have own permission. Consider adding alert in permission description that only user with trusted roles should have this permission because it may result in evaling php code. Or you might add second permission only to trusted users.

admin.inc.php file

Maybe you can add all enabled languages rather than predefined.

You need not to set '#required' => FALSE because it is default option.
The same is with '#multiple'.

.js file

Use Drupal.behavior instead of document.ready.
Whats more you can freely use $("body") instead of jQuery("body"). These two lines make sure of that:
(function($) {
})(jQuery);

Automatic review

Fix issues listed on http://ventral.org/pareview/httpgitdrupalorgsandboxklopsen1606640git

I believe these are not RTBC blockers, and I have not found any security issues. I'm setting into RTBC. If someone thinks otherwise please feel free to change.

@klopsen
Consider getting a review bonus to have your application reviewed sooner.

I'm not sure about php_eval, but you have own permission. Consider adding alert in permission description that only user with trusted roles should have this permission because it may result in evaling php code. Or you might add second permission only to trusted users.

Sorry, this is a release blocker. It's very important security holes like this not make it into releases. This is complicated because you're executing the PHP on every page load, so you would need to limit access to writing PHP to the variable rather than reading PHP from the variable. And because there are a few modules that allow writing to any variable, you don't have a good way of limiting access there.

Your default correct_li_pages value doesn't include PHP. Would it work to just remove the PHP eval functionality entirely?

Closing due to lack of activity. Feel free to reopen if you are still working on this application.

Git link added