[D7] Page2Images Website Thumbnail

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

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.

There is still a master branch, make sure to set the correct default branch: http://drupal.org/node/1659588 . Then remove the master branch, see also step 6 and 7 in http://drupal.org/node/1127732
Review of the 7.x-1.x branch:

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

  
  FILE: /var/www/drupal-7-pareview/pareview_temp/page2images.api.inc
  --------------------------------------------------------------------------------
  FOUND 0 ERROR(S) AND 2 WARNING(S) AFFECTING 2 LINE(S)
  --------------------------------------------------------------------------------
    28 | WARNING | Line exceeds 80 characters; contains 109 characters
   105 | WARNING | Line exceeds 80 characters; contains 87 characters
  --------------------------------------------------------------------------------
  

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.

Source: http://pareview.sh/ - PAReview.sh online service

• What xiukun.zhoiu said. You are still missing some code sniffer issues
• Please remove module variables you set with variable_set in an uninstall hook in the install file.
• I would format the code in line 97 a bit better. Just use the correct array structure. It would make it more readable, especially if you will end up using more than one variable.

  return array(
    'var' => array(
      'key' => 'value',
    ),
  );
  

• I might have this wrong but .api.inc is usually used to document hooks you created in your module. Now you are using it to include actual code. I would advice to rename this file to avoid confusion.
• Line 28 in your api.inc in the comment has two spaces between 'the website'.
• In some functions you have some nice comments, but in some other functions you are missing them. I advice you to add comments to make the code a bit better understandable. E.g. in page2images_get_content,or page2images_delete_catch_image. Just to let other programmers know what or why something is being done.
• The variable names in page2images_get_content could have a better name.

  foreach ($options_arr as $k => $v) {
      $options_str .= $k . '=' . $v . '&';
    }
  

  Would be handy of they are more descriptive. (Even though they probably mean $key and $value)

In .admin.inc file I don't find any use of including .api.inc file. You can remove it.

<?php
  module_load_include('inc', 'page2images', 'page2images.api');
?>

Rest other points are already included by @barthje.

Hi SuzhouKada,

After fixing all issue, plz change the status to "needs review", so other users can view it in needs review list and review your module.

Removing review links, you have not done any manual review, you just posted the output of an automated review tool. Make sure to read through the source code of the other projects, as requested on the review bonus page.

When you complete your manual reviews don't forget to add the "PAReview: review bonus" tag as indicated in http://drupal.org/node/1975228 , otherwise you won't show up on my high priority list.

* Please remove the master branch, as described here: https://drupal.org/node/1127732 in step 6.
* page2images_help(): Links should be created with l().
* theme_page2images_image(): if ($src = page2images_get_thumbnail($variables)) will always return true, as you are assigning a value to $src, this function will never return FALSE.
* page2images_admin_settings(): Description for page2images_get_account. Use a placeholder for the link, and l() to create link. See: https://api.drupal.org/api/drupal/includes!bootstrap.inc/function/t/7
* page2images.api.inc: All of the functions here could use some more documentation. What are the parameters expected and what does it return?

README.txt

Please take a moment to make your README.txt follow the guidelines for in-project documentation.

Hello,

The function page2images_get_thumbnail() includes the page2images.api.inc file which changes the PHP error_reporting settings, which I'm pretty sure is not a desired effect when you try to get a website thumbnail. Please remove the following line:
error_reporting(E_ALL ^ E_NOTICE);

The PAGE2IMAGES_CACHE_DAYS and PAGE2IMAGES_RESTFUL_KEY constants are not used. And I would also suggest not defining constants by using functions, it just feels better not using these function in there: variable_get(), file_default_scheme() and file_create_url().

Also you should probably use Drupal's file handling functions. Like file_unmanaged_save_data() or file_save() or why not use a custom stream wrapper and have your 'cached' files managed by Drupal, and create a hook_cron implementation and get rid of cached images and then remove the hackish page2images_delete_catch_image().

Also at this state the module doesn't really do much, it looks like and API for an API. It would be nice if there was a field formatter for links so you can render them as images and so they can be manipulated by modules like imagecache.

Regards,
Walter S.

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.