[D7] Copyscape

Git clone failed for http://git.drupal.org/sandbox/web247/2350161.git while invoking http://pareview.sh/pareview/httpgitdrupalorgsandboxweb2472350161git

Git repository is empty. Aborting.

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 find my findings:

• Tabs should be replaced by double spaces.
• Useless empty lines in function comments
  

  /**
   * Implementation of hook_menu().
   *
   */

• Some functions are missing comments
• A lot of foreach are defining $key but are not used
• Curly braces should be used even when it's technically optional.
• Control statements should have one space between the control keyword and opening parenthesis, to distinguish them from function calls.
• In admin form, it would be more user friendly to give a list of roles with checkboxes and a select list with users
• It may be better for readability to have two separate admin forms: 1 for copyscape settings, another for content type configuration.
• It may be also a good idea to add the checkbox on the field setting (admin/structure/types/manage/article/fields/field_tags for example). So the contributor can configure copyscape while creating fields.

Hi web247,

I have had a look at you module. I can only read the code as I do not have an account with Copyscape. However I do have a few comments and suggestions. I have only marked point 7 as a bigger issue but I think 3, 5 and 6 are blockers also however I am not sure.

Automated Review

You have quite a few errors showing up see http://pareview.sh/pareview/httpgitdrupalorgsandboxweb2472350161git
Their is some documentation about drupal coding standards here https://www.drupal.org/coding-standards is needed.

Manual Review

README.txt/README.md

Your README.txt could follow the template.

Also your project page could do with some more information. Again there is a template.

Coding style & Drupal API usage

1. copyscape.info, It would be helpful to include this line in the .info so that you can get to the config page from the modules.
   configure = admin/config/copyscape/settings
2. copyscape.module line 32, Typo with "tabel"
3. copyscape.module line 167 function copyscape_api_text_search($text, $encoding, $full = null, $operation = 'csearch')
   You have an optional $full which is always overriden by $full = 10. You then are checking if it isset on the next line when it would always be set.

   Seems like the $full is not used at all and can be removed from copyscape_api_text_search() and copyscape_api_text_search_internet()

4. copyscape.module line 181, function copyscape_api_call($operation, $params = array(), $xmlspec = null, $postdata = null)
   You are creating a url for &url. I think you could use the function url() instead.
5. copyscape.module line 209, $sites is not used.
6. copyscape.module line 313, In a few places you are creating variables and only using them once, You could you them direct.
7. copyscape.install
   (*) You will need to remove all the variables you create if someone uninstalls the module. You can do this with hook_uninstall() in the .install file.

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

I'm a robot and this is an automated message from Project Applications Scraper.

Hi,

We've done a major refactoring of the code respecting Drupal's Coding Standars.
We've also implemented the recommendations from the previous messages (thanks for taking the time).

Code has been pushed to 7.x-1.x branch.

Thank you!

Multiple Applications

It appears that there have been multiple project applications opened under your username:

Project 1: https://www.drupal.org/node/2460777

Project 2: https://www.drupal.org/node/2820903

As successful completion of the project application process results in the applicant being granted the 'Create Full Projects' permission, there is no need to take multiple applications through the process. Once the first application has been successfully approved, then the applicant can promote other projects without review. Because of this, posting multiple applications is not necessary, and results in additional workload for reviewers ... which in turn results in longer wait times for everyone in the queue. With this in mind, your secondary applications have been marked as 'closed(duplicate)', with only one application left open (chosen at random).

If you prefer that we proceed through this review process with a different application than the one which was left open, then feel free to close the 'open' application as a duplicate, and re-open one of the project applications which had been closed.

I'm a robot and this is an automated message from Project Applications Scraper.

• copyscape_node_form_validate(): In validate function you are checking if field value is updated or not. You are usig same form_state value for original and updated content. So the if condition is always true.
  

  $field_content_original = $form_state['values']{$field_name}[LANGUAGE_NONE][0]['value'];
  $field_content_updated = $form_state['values']{$field_name}[LANGUAGE_NONE][0]['value'];
  
  // If the field was not modified, we interate over to the next field.
  if (strcmp($field_content_updated, $field_content_original) === 0) {
    continue;
  }

• copyscape_api_call(): It is checking either username or apikey is empty. But using wrong variable 'copyscape_username' instead of 'copyscape_api_username'. So the function will return FALSE always.

  $copyscape_url = variable_get('copyscape_api_url', COPYSCAPE_DEFAULT_API_URL);
  $copyscape_username = variable_get('copyscape_username', '');
  $copyscape_api_key = variable_get('copyscape_api_key', '');
  
  // If the settings for accessing Copyscape were not done, exit.
  if (empty($copyscape_username) || empty($copyscape_api_key)) {
  return FALSE;
  }

• Load the inc files when it really required instead of loading globally.
• I suggest to use drupal_http_request() instead of curl request.
• Please add direct links to your reviews instead of issue link.
  eg: https://www.drupal.org/node/2828431#comment-11799478

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

I'm a robot and this is an automated message from Project Applications Scraper.

Git errors:

• There is still a master branch, make sure to set the correct default branch: https://www.drupal.org/node/1659588 . Then remove the master branch, see also step 6 and 7 in https://www.drupal.org/node/1127732

Review of the 7.x-1.x branch (commit f6b8245):

• Remove "version" from the ./copyscape.info file, it will be added by drupal.org packaging automatically.
• Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.
• DrupalPractice has found some issues with your code, but could be false positives.
  

  FILE: /home/klausi/pareview_temp/copyscape.module
  -------------------------------------------------------------------------
  FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
  -------------------------------------------------------------------------
   241 | WARNING | Unused variable $field_name.
   380 | WARNING | There must be no blank line following an inline comment
  -------------------------------------------------------------------------
  

• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

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. project page is too short. What is the purpose of the module? How does it work and integrate the API into Drupal? What is the use case? Please fill taht out according to https://www.drupal.org/node/997024
2. copyscape_schema(): foreign keys are missing for nid to the node table. See https://api.drupal.org/api/drupal/includes%21database%21schema.inc/group...
3. "// The user doesn't have any remaining fails, block him.": don't assume that all your users are male. You can use "them" instead, for example.
4. "Helper function of copyscape API" is not a good function comment. You should describe what the function is doing instead. See https://www.drupal.org/docs/develop/coding-standards/api-documentation-a...
5. copyscape_api.inc: the usage of global variables is ugly here. Is this by design when using xml_set_element_handler()?
6. copyscape_results_list(): do not call theme() at the end, just return a render array. Drupal core will render it later for you and it is easier for other modules the alter something. See https://www.drupal.org/node/930760
7. copyscape_results_list(): if I understand the code correctly then $percentage is built from untrusted third party content that you received via the API call? Then this needs to be sanitized before printing to HTML to avoid XSS vulnerabilities. Make sure to read https://www.drupal.org/node/28984 again. This is currently an application blocker.
8. "$header = array('Node ID', 'Result', 'Updated');": all user facing text must run through t() for translation.

Git errors:

• The last commit message is just one word, you should provide a meaningful short summary what you changed. See https://www.drupal.org/node/52287

Review of the 7.x-1.x branch (commit 337cddb):

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.
• DrupalPractice has found some issues with your code, but could be false positives.
  

  
  FILE: /home/klausi/pareview_temp/copyscape.module
  ----------------------------------------------------------------------
  FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
  ----------------------------------------------------------------------
   222 | WARNING | Unused variable $field_name.
  ----------------------------------------------------------------------
  

• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

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.

Otherwise looks good to me now.

Thanks for your contribution, Adrian!

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.