[D7] CleanTalk

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

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,

Please remove master branch and create new branch (like 7.x-1.x) then delete master branch also provide new link for git in issue summary.

Please change line
git clone --branch master znaeff@git.drupal.org:sandbox/znaeff/2116907.git cleantalk
to line
git clone http://git.drupal.org/sandbox/znaeff/2116907.git cleantalk

Thanks,
Vitaliy Sytnik

Hi znaeff,
First move your sandbox project from Master branch to Version branch. If you don't know, follow this Moving from a master to a major version branch.

Second thing, i have reviewd your project on pareview with your master git branch, it shows number of errors, that must be address first.

Kamlesh Patidar

Have you considered importing those files using Libraries? This would eliminate the pareview issues and would be a much more Drupal way of doing things

Hi , very nice module. While installation I had some problems but README.txt helped a lot.

Suggestions :

1) Please mention in description about libraries and CleanTalk PHP-antispam classes dependencies;
2) While I tried to install cleantalk through drush and I got message about cleantalk php classes. Please put some space before "'Make sure 2 files cleantalk.class.php and JSON.php are located there.";
3) In cleantalk_form_alter() you can use

  in_array($form_id, array('user_register_form', 'comment_node_page_form', ' comment_node_article_form',))

4) In automated reviews there are some warnings about $form_state['input']. Do you really need this array and $form_state['values'] does not contain info you need.

Thank you, very nice module!

Hello there, please be patient, there are many other projects undergoing the review process and it can take a little time. If you want to speed up the process please help in reviewing other modules and themes and gain a review bonus as mentioned here https://drupal.org/node/1975228 ... Having a review bonus will put your module application on the high priority list

db_query("SELECT ..");

is used many times, using db_select() instead would be good idea.

Just a suggestion, will get back with detailed review.

Using db_query("SELECT .."); is fine and is preferred for simple static queries, see https://drupal.org/node/310075

/**
 * Implements hook_menu().
 */
function cleantalk_menu() {
  $items['admin/config/cleantalk'] = array(
    'title' => 'Antispam by CleanTalk',
    'page callback' => 'drupal_get_form',
    'page arguments' => array('cleantalk_settings_form'),
    'access callback' => TRUE,
  );
  return $items;
}

In the above code change, remove 'access callback' => TRUE, setting access callback key of the menu item to TRUE, means that anyone can access the menu

'access arguments' => array('administer site configuration'),

and the cleantalk_settings_form($form, &$form_state) should go in a separate file called cleantalk.admin.inc

So the complete change should be

/**
 * Implements hook_menu().
 */
function cleantalk_menu() {
  $items['admin/config/cleantalk'] = array(
    'title' => 'Antispam by CleanTalk',
    'page callback' => 'drupal_get_form',
    'page arguments' => array('cleantalk_settings_form'),
    'access arguments' => array('administer site configuration'),
    'file' => 'cleantalk.admin.inc',
  );
  return $items;
}

In the above code 'access callback' would be user_access which I have skipped as in case access callback is not included it is user_access and access argument is set to array('administer site configuration')

Good catch, a_thakur!

Of course that is an access bypass security issue currently blocking this application. And please don't remove the security tag, we keep that for statistics and to show examples of security problems.'

Just a code review.

• Do pareview bonus.
• Do pareview issues.
• Use database abstraction layer instead of regular queries so your project supports different kinds of databases. For your reference: http://browse-tutorials.com/tutorial/sql-queries-examples-drupal-7
• It is suggested to remove your variables one by one instead of using LIKE.
• Use drupal_exit insead of exit.
• Consider making templates out of bigger chunks of html.

Pareview issues - http://pareview.sh/pareview/httpgitdrupalorgsandboxznaeff2116907git
Review bonus - https://drupal.org/node/1975228

So $form_state['values'] won't work in your case?

Code review:

*** cleantalk.info
configure = admin/config/cleantalk
(Minor) I would advise to change it to "admin/config/content/cleantalk" so it gets positioned under "Content authoring" + change hook_menu() accordingly (like Mollom does).

*** cleantalk.install

function cleantalk_uninstall() {
  db_query("DELETE FROM {variable} WHERE name = 'ct_automod'");
  db_query("DELETE FROM {variable} WHERE name = 'ct_comments'");
  db_query("DELETE FROM {variable} WHERE name = 'ct_key'");
  db_query("DELETE FROM {variable} WHERE name = 'ct_status'");
  cache_clear_all('variables', 'cache');
}

(Normal) Use variable_del() function instead of DB queries and remove cache_clear_all() call.
(Minor) Variables should be starting with full module name (to prevent namespace collision); it's better to rename them before the 1st stable release or you would need to implement hook_update_N() to rename them. Marked as minor as I don't think there will be a module named "ct".

function cleantalk_requirements()
(Minor) You don't need to use the DRUPAL_ROOT in the file_exists() calls - just $path should be enough.

function cleantalk_enable()
(Normal) Why do you need it? - hook_requirements() should be enough, right?

*** cleantalk.module

define('CLEANTALK_DRUPAL_VERSION', '7');
define('CLEANTALK_MODULE_VERSION', '1.0');
define('CLEANTALK_MODULE_HOMEURL', 'http://www.drupal.org/project/cleantalk');

(Normal) Why do you need these constants?

function cleantalk_form_alter()
// drupal_set_message($form_id); // print form ID - great for gebug!
(Minor) While it is great for debug - xdebug is much better :) I would recommend to remove this line.

(Major) Looks like you are protecting comment forms only for article and page content types.
You'd better use the hook_form_BASE_FORM_ID_alter() for comments - https://api.drupal.org/api/drupal/modules%21system%21system.api.php/func...
It will be function cleantalk_form_comment_form_alter(&$form, &$form_state, $form_id) that will action on all comment forms.

function cleantalk_entity_presave()
(Minor) HUGE... might be good to move chunks of code to separate functions.

function cleantalk_comment_view_alter()
(Major) Completely wrong permissions checking. You should use just user_access('administer comments').
You shouldn't check if user is logged in or not; you shouldn't check if user has administrator role; and you definitely don't need to calculate what roles have the 'administer comments' permission. Just simple plain user_access('administer comments').

/**
 * Implements hook_comment_view_alter().
 */
function cleantalk_comment_view_alter(&$build) {
  if (user_access('administer comments')) {
    $result = db_select('cleantalk_cids', 'c')
      ->fields('c', array('ct_request_id', 'ct_result_comment'))
      ->condition('cid', $build['#comment']->cid, '=')
      ->range(0, 1)
      ->execute();
    if ($result->rowCount() > 0) {
      $ct_result = $result->fetchObject();
      if (!empty($ct_result->ct_result_comment)) {
        $build['comment_body'][0]['#markup'] .= '<hr>' . $ct_result->ct_result_comment;
      }
    }
  }
}

You can also use your own permission if you want - see hook_permission() and user_access().

*** cleantalk.admin.inc
(Minor) variable_get('ct_key', 'enter key') - these calls should be consistent; here you use 'enter key', in 2 other places just ''
You can do this in the form element:
'#default_value' => $ct_key ? ct_key : 'enter key',
(Normal) <a target="__blank" ... - why do you use 2 underscores here?
(Normal) You should use '#element_validate' => array('element_validate_integer_positive') for the 'ct_comments' form element - then the $ct_comments_min variable won't be needed and your checks at the start can be removed. It will simplify this function.

*** Overall:
(Minor) Hard to read the complex if () statements - need to check what Drupal code standard tells about it.

README.txt
(Minor) Typo "registraton"
(Minor) "it's" should be changes to "its":
- "3. Install CleanTalk module as usual and put access key into it's settings."
- "* Try to put comment with "stop_word" in it's body."

I'm ready to mark it RTBC after the "Normal" and "Major" issues are fixed - all the others can be discussed and fixed later using the issue queue.

Thanks
AndyB

Alexey,

Good-good - marking as RTBC. My main concern was with permissions checking and you have fixed it. The next step is to wait for the project application admin to process this request. You can try to find one on IRC to speed up the process.

Regarding the comments:

Unfortunately no. When I erase my library files manually, and then enable the module, hook_requirements() doesn't catch this, but hook_enable() does.
I see this is very unusual situation but I have to be sure that all needed files are present and admin is informed when not. I think it's better to keep this function. Do you?

Good to know. If it works this way when you test it and you are completely sure you need it - keep it. I would need to do my own check on this as in the modules I wrote in the past the hook_requirements() implementation can block the installation of the module.

I made 3 functions because of exactly 3 needed forms.

Looks like misunderstanding here. I was talking that you are protecting comment forms only for the article and page content type. And if the user will create some custom content type and enable comments for it - then this content type comment form won't be protected by your module. And I was suggesting you the solution on how to hook on every comment form for any content type created by the user.
As for the fix you have implemented - you have created the 3 implementations of hook_form_FORM_ID_alter() - so you should fix the comment for them, it should be "Implements hook_form_FORM_ID_alter()." for all 3 functions. And then in the _cleantalk_form_alter() you don't need to check for these 3 form ids (the first if statement) - you will for sure get only one of these 3 ids there.

'enter key' is needed in admin form only.

Yes, I do understand this. What I was talking is that it should be the same default value in every call to variable_get() - so it should be variable_get('cleantalk_authkey', '') and then the 'enter key' text should be used only in the form element - see above on the code I wrote for this.

Also, as you have added the '#element_validate' => array('element_validate_integer_positive') you don't need to keep the checks in the beginning of the cleantalk_settings_form() function. This element validation will ensure that the value is not lower than 1.

Thanks
AndyB

Alexey,

Sure, you can continue to improve the code. Hope your project will be approved soon.

AndyB

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

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

  FILE: /home/klausi/pareview_temp/cleantalk.module
  --------------------------------------------------------------------------------
  FOUND 1 ERROR AFFECTING 1 LINE
  --------------------------------------------------------------------------------
   642 | ERROR | [x] Whitespace found at end of line
  --------------------------------------------------------------------------------
  PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
  --------------------------------------------------------------------------------
  

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. cleantalk_schema(): descriptions for the tables and fields are missing. What are the used for? What do the columns mean? See https://api.drupal.org/api/drupal/includes!database!schema.inc/group/sch...
2. cleantalk_enable(): this function can now be removed since you are already checking the library in hook_requirements().
3. cleantalk_entity_presave(): why do you spit out a completely custom error page? How is a theme supposed to override or style that content, it seems to me it cannot? You need to use appropriate theme functions. So it looks to me you want to cancel the comment submission here? You cannot do that in a presave hook. You must implement a form validation function and add that to the comment form, then you can set form errors and prevent the comment from being inserted properly.
4. cleantalk_entity_presave(): no need for the library error handling here, you are already checking that in hook_requirements(). Same in _cleantalk_send_feedback().
5. cleantalk_entity_presave(): instead of having this very long function you should split this up to hook_user_presave() and hook_comment_presave(), makes the code way more maintainable.
6. "$user_agent = htmlspecialchars((string) $_SERVER['HTTP_USER_AGENT']);": why do you use htmlspecialchars() here, you are not printing the user agent back to HTML so there is no XSS attack vector? Make sure to read handling text in a secure fashion again: https://drupal.org/node/28984
7. cleantalk_entity_presave(): no need for your 6 line if statements where you check every key in a nested array. Just use isset($entity->comment_body[$comment_lang][0]['value']) or !empty($entity->comment_body[$comment_lang][0]['value']) which will not throw PHP notices and is much, much shorter.
8. cleantalk_entity_presave(): instead of db_update()/db_insert() you could just use db_merge()?
9. cleantalk_entity_presave(): do not call drupal_mail_system(), use drupal_mail() instead and implement hook_mail() properly so that modules can alter the message.
10. cleantalk_comment_view_alter(): $ct_result->ct_result_comment is untrusted third party text, correct? Then it needs to be sanitized before printing to prevent XSS exploits. Or is it sanitized later when the comment is rendered? Please add a comment.
11. cleantalk_registry_files_alter(): why do you need this? Please add a comment. Shouldn't the libraries API cover that for you?
12. _cleantalk_form_alter(): javascript should be added with #attached on the render array and should be in dedicated JS files.
13. _cleantalk_form_alter(): why do you need to create form input fields on your own in HTML here and cannot use the form API? Please add a comment.

pareview.sh came up clean, but there are a lot of long lines and some bad indentations. Fix these if possible.

cleantalk_settings_form(). Just use a blank value for the default; your description is fine.

Avoid HTML in t(). Make the link with l() and pass in as a placeholder. This occurs in a few places.

cleantalk_requirements(), avoid passing in variables to t().

(*) Don't disable the module just b/c the library dependency is missing. Just make it not functional. You have useful
info in that message which will go away.

Shouldn't comment be a dependency in the .info?

The number in cleantalk_validate_comment() should probably be a setting.

The query in cleantalk_validate_comment could be a single db_query(). It's not a dynamic query.

The syntax of your second query is a little weird. Instead of {comment}.subject you normallu set
your own table alias and then just use comment.subject

cleantalk_validate_comment() should use the Field API instead of direct access to the $node properties.

(*) You have a dependency on the Libraries API, but you aren't using it. See https://drupal.org/node/1342238,
in particular hook_libaries_info().

_cleantalk_form_alter(), use REQUEST_TIME instead of time().

_cleantalk_form_alter(), use drupal_html_id()?

The setTimeout() in _cleantalk_form_alter is bad. You can probably use inline JS to add something better.

(*) The hidden element and JS in _cleantalk_form_alter, along with directly accessing $_POST, is a
serious API issue.

(*) Figure out how to get rid of $GLOBALS['ct_request_id']. This will be really buggy in some situations. Same goes
for session variables. This really seems like it should be a fairly simple, synchronous process that is overcomplicated
right now. I would read through Mollom and Akisment for inspiration.

Can you just use variables for the server changes?

Use drupal_json_encode() instead of json_encode().

manual review:

1. cleantalk_schema(): descriptions for the fields are missing. What are the used for? What do the columns mean? Every columns should have a 'description' in the array.
2. cleantalk_enable() is still there?

Looks like you forgot to commit and push the changes? Please work through my and mpdonadio's feedback again.

manual review:

1. _cleantalk_check_spam(): why do you need to start a session? Please add a comment.
2. The _cleantalk_ct_result() function holding the global state is ugly, but not sure how it could be removed. The question is why you need cleantalk_comment_presave(), did you try to get the comment $form_state['comment'] in the validation function an just set the status there?
3. _cleantalk_filter_response(): should only be called when you are actually printing it. "When handling data, the golden rule is to store exactly what the user typed. When a user edits a post they created earlier, the form should contain the same things as it did when they first submitted it. This means that conversions are performed when content is output, not when saved to the database" from https://www.drupal.org/node/28984 . So that function should be used when you add the content to the render array for example.

But that are not critical application blockers, so I think this is RTBC.

Assigning to dman as he might have time to take a final look at this.

Looks like there has been lots of direct code review. I also visually scanned things, including the additional library (mixed tabs and spaces and other stuff there) but it doesn't look like there are any blockers left. I won't do another line-by-line.
Direct access to the SESSION and calling drupal_session_start() inside form_alter confuses me, but I can't understand the code well enough to see if there is a good reason for that. It's probably necessary I guess.

* I installed the module, and the library, and got an access key from the service...
* I simply could not find any menu item for configuration - not even on /admin/config . Even after checking permissions (despite being UID1) and flushing cache etc.
* I had to get into the code to find that the expected URL was /admin/config/cleantalk (It's not in the readme) . I don't know why this is hidden in this way.
* I pasted in the Access key, and it seemed to be valid.
* testing a registration with the blacklisted email was successfully blocked, but I could not see how to change the text that was displayed to the user.
* Also, and importantly, this action was not logged locally in the watchdog. Without that, I certainly would not be recommending anyone use this service as-is. You really need to log when a rejection like this happens so that admins have a chance of troubleshooting.
* I found that if the network or the remote service was unavailable for any period, the application that would normally be blocked was allowed straight through immediately, leaving forms that were previously protected unprotected. This connection failure was not logged either, meaning it looks possible for some bad submissions to go through which would otherwise be blocked, and leave no way to diagnose why they got through while others did not.
* I enabled comments and posted a comment with " stop_word in the body" but it went through. Wondered if that was my user account.
* I created and logged in with a new lesser user account in a new browser session, I was still able to post with " stop_word in the body " in that accounts first post and it got through.
* I gave that user ability to add articles, and fount that they could also still post nodes with stop_word in the body.

So, my current user assessment is that:

• the configs are inaccessible
• it doesn't let the admin know what it is and isn't doing in any way, does not log when it is dropping user submissions.
• it immediately fails to work if there is any connection interruption with the remote service ,
• does not yet seem to even work as described as "testing" in the instructions with comments (though this may just be a bug?)
• does not seem to catch the test word in node body submissions either (though it did catch a registration)
• I could not find any further configs or visibility on what was and wasn't being blocked, or any way to modify my own blacklist or tweak parameters

If you want to update the project description and clarify that the issues I think are pretty big are just "by design" or something then maybe the code can go up as a project, but generally we expect that a module actually works as advertised before that happens.

Given the time that has been spent on this project, the *code* looking OK, and the previous reviews, I'd really wanted to just give it a quick run-through and then promote it right now.
But right now it still seems unfit-for-purpose: "does not work for me". I'm not keen to promote a module that does not yet do what it says.

OK, let's put this to "needs work" then and wait for znaeff to look into the issues. Thanks for the thorough review dman!

Alexey has been in touch and we identified that my worst issues were due to a configuration issue on the server side. So that goes down to some documentation, and a policy adjustment on their end - not a code bug.
I still want to see some logging when this system is sending my submissions around the world and back, but I'm OK with that being a feature request. Same with the need to scan actual node or forum body submissions.
Whatever "Needs Work" I listed and is left can be dealt with in the project issue queue before a stable point release. And will be easier to work on there as individual issues.
Code presented here meets the project application standards, and the module is usable. Thanks for your patience and cooperation - so lets get this up finally!

--------------------

Thanks for your contribution, znaeff!

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.