[D7] Imagesize check

Hi neetu,

1- First change Link to git repository to this
git clone --branch 7.x-1.x http://git.drupal.org/sandbox/neetumorwani/2086453 imagesize_check
2- On line 68 .module file you have unused nid variable, remove it

$nid = db_insert('imagesize_check')
            ->fields(array(
              'image_name' => $uri->name,
              'image_size' => $image_info['file_size'],
              'image_path' => $uri->uri,
              'image_filename' => $uri->filename,
            ))->execute();

Thanks for your effort.

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.

On line 110 change your link because it takes me to nowhere. change this
l(t(' here '), 'admin/reports/image_preset_errors/image-preset-settings');
to this
l(t(' here '), 'admin/reports/imagesize-check-errors/imagesize-check-settings');

@neetumorwani,

Module works in terms of functionality. Though, there are couple of small coding standards you need to modify.
imagesize_check.module line#60

if ($imagesize_check_type == 0) {
      // If there is no image that exceeds the size limits.
    }
    else {
      foreach ($images as $keys => $uri) {
        $image_info = image_get_info($uri->uri);
        dsm($image_info);
        $image_size = ($image_info['file_size']) / 1000;
        if ($image_size > $imagesize_check_type) {
          db_insert('imagesize_check')
            ->fields(array(
              'image_name' => $uri->name,
              'image_size' => $image_info['file_size'],
              'image_path' => $uri->uri,
              'image_filename' => $uri->filename,
            ))->execute();
        }
      }
    }

• Instead of having an empty if condition, you should just use a negative condition to do what's being done in the else condition.
• Remove dsm on line#66
• Hardcoded pager limit to 25, Can we have it configurable for admins?

This module is looking nice which I really appreciate.
I was going through your module, I found some minor bugs , here they are:-

1. There are too much of dependencies I found, Can you please make it less like if you wish you can provide one checkbox for mailing purpose. If checkbox is checked, then user can use HTML formatted form.
2. You have used variable_get('imagesize_check_type' . $key) on line 61. But after uninstalling this module, this has to be deleted. For this, you have to do something like this in hook_uninstall in your .install file:-
   

   hook_uninstall(){
     variable_del('imagesize_check_type');
   }

3. On admin/reports/imagesize-check-errors , I am seeing that Image id is regularly changing as cron run. I think you are deleting all the entry and then inserting it into your custom table.
   Anyhow, my suggestion is you can just update the values in custom table.

Automated tested by coder module

1. 182 | ERROR | No space found after comma in function call
2. 240 | ERROR | Array indentation error, expected 4 spaces but found 0

I think you are able to fix these, I would love to maintain this module in future. :)

Please have a look on this one also :-
http://pareview.sh/pareview/httpgitdrupalorgsandboxneetumorwani2086453git

I'm excited about the aspect ratio report --

https://drupal.org/node/2175071

@Neetumorwani,

I think you should change the name of the module in light of above feature.

Hi Neetu,

I found a few errors and warnings using the automated review at pareview.sh.

Results of the review can be found here:

http://pareview.sh/pareview/httpgitdrupalorgsandboxneetumorwani2086453git

Manual Review:

On the Image Glitches report the line that says "You can set the values for the maximum memory size of each image preset style fromhere" needs a space between from and here, and also a full stop as can be seen in this image:

This is not a bug but I feel that the size of the input boxes on the configuration page are unnecessarily large considering they will only have a few characters in:

Cheers,
Felix

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/imagesize_check.module
  --------------------------------------------------------------------------------
  FOUND 2 ERRORS AND 1 WARNING AFFECTING 3 LINES
  --------------------------------------------------------------------------------
   4 | ERROR   | Doc comment short description must start with a capital letter
   8 | WARNING | Format should be "* Implements hook_foo().", "* Implements
     |         | hook_foo_BAR_ID_bar() for xyz_bar().", or "* Implements
     |         | hook_foo_BAR_ID_bar() for xyz_bar.tpl.php.".
   9 | ERROR   | There must be exactly one blank line before the tags in a doc
     |         | comment
  --------------------------------------------------------------------------------
  

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. "'access arguments' => array('Image Glitches Record'),": permissions are usually all lower case.
2. imagesize_check_menu(): this vulnerable to access bypass attacks. The path /admin/reports/imagesize-check-errors/imagesize-check-settings is not protected at all, allowing any anonymous attacker to change variables for this module. And please don't remove the security tag, we keep that for statistics and to show examples of security problems.
3. "'#markup' => 'Enter the maximum size (in Kilobytes) an image style could have. Default is 100KB.',": all user facing text must run through t() for translation. Same in imagesize_check_cron(), please check all your strings.
4. imagesize_check_admin_form(): @return is wrong and not necessary here, see https://drupal.org/node/1354#forms
5. imagesize_check_mail(): this is a hook implementation and should be documented as such, see https://drupal.org/node/1354#hookimpl . Same for imagesize_check_cron(). Please check all your hooks.
6. imagesize_check_record(): do not call render() here, always return a nested render array without invoking render(), drupal_render() or theme(). Drupal will render it later for you. See https://drupal.org/node/930760
7. imagesize_check_cron(): if I have cron configured to run every 10 minutes I will be flooded with mail. You should check for a certain interval before executing the logic, ideally that should be configurable.

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

Review of the 7.x-1.x branch

Automated Review

Best practice issues identified by pareview.sh / drupalcs / coder

None found using drush dcs or pareview.sh

Manual Review

• Duplication
• No duplicates found.
• Multiple applications
• No
• README.txt
• Follows the guidelines for in-project documentation.
• Master Branch
• Follows the guidelines for master branch.
• 3rd party code
• Follows the guidelines for 3rd party code.
• Yes
• Individual user account
• Follows the guidelines for individual user accounts.
• Yes
• Code too short/too simple
• Follows the guidelines for project length and complexity.
• Licensing
• Yes
• Security
• No security issues identified.
• Coding style & Drupal API usage

  Code review here using this format:

  (*) There is no configuration entry in the module *.info file, but I see configuration entries in hook_menu. (https://drupal.org/node/542202)
  (*) Function imagesize_check_update_aspect_ratio_report() needs more inline comments. This appears to be the
  meat of the module and better comments documenting the the function will help with bugs.

  The following code where the results of a t() call are appended to the results of an l call should be changed into
  one t() call. See https://api.drupal.org/api/drupal/includes%21common.inc/function/l/7 (Look for the directions
  on how to use "t()" on this page.)

    if (!$check) {
      $output .= "<br />";
      $output .= t('There are no images with glitches in the memory size
        because no maximum image size is been set for image presets. You can
        set the values for the maximum memory size of each image preset
        style from ') .
      l(t('here'), 'admin/reports/imagesize-check-errors/imagesize-check-settings');
      $output .= "<br />";
    }
  

  The following code, in imagesize_check_ratio_check_record() has several l() functions concatenated together. Perhaps using
  one t() call here, with each link as a named placeholder would work better here. At a minimum, it would shorten the line
  and make maintenance easier.

    foreach ($answer as $row) {
      $rows[] = array(
        substr($row->image_name, 0, 60),
        $row->usage_count,
        $row->image_style,
        $row->image_original_aspect_ratio,
        $row->image_aspect_ratio,
        $row->image_diff . '%',
        l(t('Edit'), "file/$row->fid/edit") . ' | ' . l(t('Usage'), "file/$row->fid/usage") . ' | ' . l(t('Delete'), "file/$row->fid/delete") . ' | ' . l(t("View"), image_style_url($row->image_style, $row->image_path)),
      );
    }
  

  (*) Major finding
  Minor finding
  (*) Major finding

  The starred items (*) are fairly big issues and warrant going back to Needs Review. The rest of the comments in the code walkthrough are recommendations.

Forgive me if any of this has been done, I started the review over the weekend but wasn't able to finish it.

Automated Review

Nothing

Manual Review

The naked hook_menu that klusi mentioned has been fixed.

Adding a dependency on file_entity is going to limit the usage of this module. Not all sites that have images will be
using this module. And, installing file_entity just for some reports may intruduce some subtle differences in how files
get handled.

You don't need to explicitly uninstall the schema; drupal_uninstall_modules() does this for you.

Wildcard variable deletions should be avoided because of namespace conflicts.

Why does the query in imagesize_check_record have the node_access tag? Not seeing node being queried against.

Will this work with access module when styles end up in private://?

Rather than building markup directly, your page callbacks should build up render arrays.

Where is PASC being used? You have a hard dependency on it, but they you module detect it? If you are just implementing a PASC hook,
then you don't need a dependency (or a module detect it). If it is installed, then it will be picked up.

Where are you using File Entity? It seems that you only generate a link to the edit page? If so, you can remove
the hard dependency, and module detect to make this link.

You really need to make email a configurable setting.

May want to checkout format_size() for size info you output to the user.

You have some unused variables here and there. An IDE like Aptana, Eclipse, or PHPStorm can help find these.

You are sending the email to everyone in the administrator role? I have lots of clients where the admins have nothing to
do with content. Think of some better long term options. You can also do that query with one SELECT.

What if I don't want to do a full cron run to check image sizes? A user with your permission may also not have access
to run cron.

Consider moving the settings form to a .admin.inc file, and the page callbacks to a .pages.inc file, and then wire
them up in the hook_menu. This can help keep memory usage down.

This looks like it can be a handy module, but it needs some TLC. However, it looks like the blocking issues mentioned above have been addressed, and I am not seeing any other blockers. Marking RBTC.

manual review:

1. imagesize_check_record(): why is there a node_access tag added to the db query that does not run against the node table? Please add a comment.
2. 'Run cron manually': all user facing text must run through t() for translation.
3. imagesize_check_cron(): this will not work. If my cronjob runs every 10 minutes and imagesize_check_cron is set to 24 hours then the implementation will never be executed. Please test changes to your code if they actually work.
4. Your module does not include any test file, did you consider writing automated tests with simpletest? See https://drupal.org/simpletest
5. Still all the wrong theme(), render() and drupal_render() calls that should be replaced with nested render arrays. Just look at any page callback of a core module how they build render arrays and return them.
6. Plus everything mpdonadio said.

Although there are so many open issues in your code that should be fixed they are not absolute critical application blockers, so ...

Thanks for your contribution, neetu morwani!

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.

changing to use the more common tag, so the less common one can be deleted, so it does not show up in the auto complete and confuse people.