Foresight Images

There are several dynamic image resizing modules on drupal.org alread such as http://drupal.org/project/cs_adaptive_image (several more examples are linked from that page). Perhaps you should clarify on your project page & application how your module is different from those alternatives.

PAReview.sh turned up a bunch of issues; you may want to look at http://ventral.org/pareview/httpgitdrupalorgsandboxgigabates1859182git (most of it is whitespace/punctuation nitpicking but there are a few actual code issues. http://drupal.org/node/322731 should provide you with some insight on the install issues.

When "check bandwidth" is disabled, the url for my 2100x1400 image is output as sites/default/files/styles-foresight/1000x1000xundefined/huge/public/LHC.jpg. The "huge" image style is scale& crop 1000x1000 but that url returns the full size image. Checking the log reveals several warnings starting with

Invalid image dimensions in image_gd_create_tmp()

and culminating in an error:

Image scale and crop failed using the gd toolkit on public://LHC.jpg (image/jpeg, 2168x1452)

which I'm guessing explains the image not being resized. If I try to access the "non-foresight" version of the "huge" image (sites/default/files/styles/huge/public/LHC.jpg) everything works fine.

When "check bandwidth" is on, it just outputs the url as sites/default/files/styles/huge/public/LHC.jpg with no errors in the log.

Hi gigabates,

Thanks very much for contributing this very nice module, adding more information to the README.txt file and fixing the coding standards errors.

I am wondering whether I have been testing your latest version of the module, which I think I did, since I checked out the head from your GIT, branch 7.x-1.x. This problem is so obvious that you might already have found it but perhaps just not committed.

1 - In file foresight.module, at line 69:

function foresight_api_permission() {
  return array(
    'administer foresight' => array(
      'title' => t('Administer Foresight'),
      'description' => t('Configure how Foresight images aware are displayed.'),
    ),
  );
}

After installing the module, the permission is not displayed on the admin/people/permissions/list Permissions page, because of the function name.
I think you should simply change:
foresight_api_permission
to be foresight_permission (I've tested it and it works fine, as expected).
Maybe you simply changed your module name at a later stage of development and it's one of the find/replace that didn't go through.

2 - In file foresight.module, at line 112:

      'description' => 'Pixel ratio aware image using Foresight library',

There's a t() call on the label property but not on description.
Perhaps you should consider adding a call to t() function, as you did in all other places in your module where you have a 'description' property.
I think that's the only place I found in your module where a call to t could be missing, but you could do another quick check I guess.

So far, that's all I could find and I hope it helps improving your very nice module.

Cheers!

Forgot to change status.
Removing PAReview: review bonus tag: Reviews have been provided.

Hope these comments help.
Cheers!

Hi gigabates,

Lines 378 and 379 in foresight.module:
$width_ratio = $request_width / $original_dimensions['width'];
$height_ratio = $request_height / $original_dimensions['height'];
There are division operations on above lines. Consider the exception of the denominator being zero.

Yes, exception scenarios need to be considered as an robust module.

Please add all your reviews to the issue summary.

manual review:

1. foresight_init(): why can't you use Drupal.settings with drupal_add_js()? See http://drupal.org/node/756722
2. "watchdog('image', 'Unable to generate the derived image located at %path.'": better use your own module name instead of "image" here?
3. "return drupal_access_denied();": I think it is sufficient to just return MENU_ACCESS_DENIED.
4. "'#field_suffix' => 'minutes',": all user facing text must run through t() for translation.

But that are no blockers, otherwise looks RTBC to me. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Related with #11.

Hi gigabates,

This caught my attention and I asked myself the same question as you did.

Actually, I was imagining that perhaps, if an image was corrupt, or missing from the server (but still recorded somewhere), or for any other reasons, maybe image_get_info() would return 0 dimensions.....
I'm not really sure either, so that's why I said maybe: it would need to be investigated further in code.

But I just wanted to also share my concern on this, after I saw your exchanges with Jordan_Fei.

I would be interested if anyone else encountered this type of issues before, and in which circumstances.
Thanks for all your comments and feedbacks.
Cheers!

Thanks again for clarifying that and pulling the reference out from the PHP code.

So the suggested scenarios above, in #14 could correspond to this particular error case, couldn't they?

if an image was corrupt (image headers corrupt for some reason, for example), or missing from the server (but still recorded somewhere), or for any other reasons, maybe image_get_info() would return 0 dimensions.....

If I didn't misunderstand your answer, these could be among the potential error cases that would be likely to happen.

Thanks again for all your replies.
Cheers!

Hi gigabates,

Thanks for your prompt and precise answer.

That's what I had assumed.
I know for sure everything should be fine with Foresight Images, as far as this issue is concerned, but I was and still am curious to see if anyone could report encountering this type of errors on a real case scenario before.

Feel free to let me know if you hear anything from anyone about that, I would still be interested.

Thanks again for sharing this great module and nice work.
Cheers!

no other objections for more than a week, so ...

Thanks for your contribution, gigabates!

I updated your account to let you 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 get 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.