IIP Image

Please do not push into the master branch, follow the instructions on naming conventions here:
http://drupal.org/node/1015226, git branching: http://drupal.org/node/1066342

Some issues can be detected by the coder module (http://drupal.org/project/coder)
(Be shure to set it to "minor (most)")

iip_image.module

Line 166: in most cases, replace the string function with the drupal_ equivalent string functions
  $conv->uri = substr($conv->uri, 0, -3) . 'tif';
Line 245: in most cases, replace the string function with the drupal_ equivalent string functions
    $item['uri'] = substr($item['uri'], strripos($item['uri'], '/' . $instance['settings']['file_directory']));
Line 296: in most cases, replace the string function with the drupal_ equivalent string functions
  $image_name = substr($file->filename, 0, -3);
Line 297: in most cases, replace the string function with the drupal_ equivalent string functions
  $image_path = substr($file->uri, 0, strlen($file->uri)-3);
Line 299: in most cases, replace the string function with the drupal_ equivalent string functions
  $image_ext = substr($file->filename, -3);
Line 330: Format should be * Implements hook_foo().
 * Implementation of hook_menu().
Line 355: Use "elseif" in place of "else if"
  else if (!file_exists('sites/all/libraries/iip_image/javascript/mootools-1.2-core-compressed.js')) {
Line 358: Use "elseif" in place of "else if"
  else if (!file_exists('sites/all/libraries/iip_image/javascript/mootools-1.2-more-compressed.js')) {
Line 361: Use "elseif" in place of "else if"
  else if (!file_exists('sites/all/libraries/iip_image/javascript/iipmooviewer-1.1-compressed.js')) {
Line 367: in most cases, replace the string function with the drupal_ equivalent string functions
      $somepic->uri = substr($somepic->uri, 0, strripos($somepic->uri, '.'));
Line 368: String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
      $basepath = $_SERVER["DOCUMENT_ROOT"].base_path();

remove ; $Id$ line.
This is deprecated since drupal.org is using git

also remove the last three lines of your info:

version = "7.x-1.0"
core = "7.x"
project = "iip_image"

version will be added automatically on release and you set the core twice

Please remove the LICENSE.txt it will be added automatically when you create a release.

regards

• The term "CCK" does not exist anymore in Drupal 7. Replace it everywhere just with "fields"
• lines in README.txt should not exceed 80 characters
• only list files in the info file that contain classes or interfaces
• Remove dpm() debug statements from your code
• module file: @file doc block missing, see http://drupal.org/node/1354#files
• _file_generic_settings_tile_size(): all functions in your module must be prefixed with your module name to avoid name clashes.
• image_iip_page(): do not embed javascript like this, see drupal_add_js(). Or is this such a special page callback that you need to do this?

Changed git clone path due to created new release branch

There are still files other than README.txt in the master branch, make sure to remove them. See also step 5 in http://drupal.org/node/1127732

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

• ./iip_image.module: all functions should be prefixed with your module/theme name to avoid name clashes. See http://drupal.org/node/318#naming
  
  

  <br>
  function image_iip_page($id) {<br>
  

• Drupal Code Sniffer has found some code style issues (please check the Drupal coding standards). See attachment.

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. Get a review bonus and we will come back to your application sooner.

</code></p><code>
<p>FILE: ...al-7-pareview/sites/all/modules/pareview_temp/test_candidate/README.txt<br>
--------------------------------------------------------------------------------<br>
FOUND 1 ERROR(S) AND 22 WARNING(S) AFFECTING 23 LINE(S)<br>
--------------------------------------------------------------------------------<br>
 13 | WARNING | Line exceeds 80 characters; contains 152 characters<br>
 14 | WARNING | Line exceeds 80 characters; contains 152 characters<br>
 15 | WARNING | Line exceeds 80 characters; contains 146 characters<br>
 22 | WARNING | Line exceeds 80 characters; contains 102 characters<br>
 23 | WARNING | Line exceeds 80 characters; contains 107 characters<br>
 24 | WARNING | Line exceeds 80 characters; contains 98 characters<br>
 28 | WARNING | Line exceeds 80 characters; contains 82 characters<br>
 29 | WARNING | Line exceeds 80 characters; contains 83 characters<br>
 34 | WARNING | Line exceeds 80 characters; contains 127 characters<br>
 35 | WARNING | Line exceeds 80 characters; contains 92 characters<br>
 36 | WARNING | Line exceeds 80 characters; contains 119 characters<br>
 40 | WARNING | Line exceeds 80 characters; contains 133 characters<br>
 41 | WARNING | Line exceeds 80 characters; contains 122 characters<br>
 42 | WARNING | Line exceeds 80 characters; contains 107 characters<br>
 43 | WARNING | Line exceeds 80 characters; contains 100 characters<br>
 45 | WARNING | Line exceeds 80 characters; contains 144 characters<br>
 51 | WARNING | Line exceeds 80 characters; contains 115 characters<br>
 59 | WARNING | Line exceeds 80 characters; contains 82 characters<br>
 61 | WARNING | Line exceeds 80 characters; contains 111 characters<br>
 64 | WARNING | Line exceeds 80 characters; contains 107 characters<br>
 67 | WARNING | Line exceeds 80 characters; contains 119 characters<br>
 68 | WARNING | Line exceeds 80 characters; contains 120 characters<br>
 80 | ERROR   | Files must end in a single new line character<br>
--------------------------------------------------------------------------------</p>
<p>FILE: ...-pareview/sites/all/modules/pareview_temp/test_candidate/iip_image.info<br>
--------------------------------------------------------------------------------<br>
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)<br>
--------------------------------------------------------------------------------<br>
 7 | ERROR | Files must end in a single new line character<br>
--------------------------------------------------------------------------------</p>

FILE: ...areview/sites/all/modules/pareview_temp/test_candidate/iip_image.module<br>
--------------------------------------------------------------------------------<br>
FOUND 23 ERROR(S) AND 3 WARNING(S) AFFECTING 22 LINE(S)<br>
--------------------------------------------------------------------------------<br>
 100 | WARNING | Line exceeds 80 characters; contains 91 characters<br>
 163 | ERROR   | Inline comments must start with a capital letter<br>
 169 | ERROR   | You must use "/**" style comments for a function comment<br>
 174 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or<br>
     |         | question marks<br>
 177 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or<br>
     |         | question marks<br>
 181 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or<br>
     |         | question marks<br>
 184 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or<br>
     |         | question marks<br>
 187 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or<br>
     |         | question marks<br>
 209 | ERROR   | More than 2 empty lines are not allowed<br>
 275 | ERROR   | Missing parameter type at position 1<br>
 278 | ERROR   | Data type of return value is missing<br>
 301 | ERROR   | Missing parameter type at position 1<br>
 319 | WARNING | Line exceeds 80 characters; contains 162 characters<br>
 319 | ERROR   | No space before comment text; expected "// shell_exec('convert<br>
     |         | ' . $image_path . $image_ext . ' -define tiff:tile-geometry='<br>
     |         | . $tilesize . ' -compress jpeg "ptif:' . $image_path .<br>
     |         | $image_dest . '"');" but found "//shell_exec('convert ' .<br>
     |         | $image_path . $image_ext . ' -define tiff:tile-geometry=' .<br>
     |         | $tilesize . ' -compress jpeg "ptif:' . $image_path .<br>
     |         | $image_dest . '"');"<br>
 325 | WARNING | Line exceeds 80 characters; contains 113 characters<br>
 325 | ERROR   | No space before comment text; expected "// shell_exec('convert<br>
     |         | ' . $image_path . $image_ext . '[0] -compress jpeg ' .<br>
     |         | $image_path . $image_dest . '');" but found<br>
     |         | "//shell_exec('convert ' . $image_path . $image_ext . '[0]<br>
     |         | -compress jpeg ' . $image_path . $image_dest . '');"<br>
 337 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or<br>
     |         | question marks<br>
 340 | ERROR   | Space found after object operator<br>
 341 | ERROR   | Space found after object operator<br>
 342 | ERROR   | Space found before object operator<br>
 342 | ERROR   | Space found after object operator<br>
 411 | ERROR   | Closing brace indented incorrectly; expected 4 spaces, found 6<br>
 412 | ERROR   | Expected "}\nelse {\n"; found "}\n\nelse { "<br>
 412 | ERROR   | Closing brace must be on a line by itself<br>
 413 | ERROR   | Closing brace indented incorrectly; expected 2 spaces, found 4<br>
 416 | ERROR   | Files must end in a single new line character<br>
--------------------------------------------------------------------------------<br>

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

Hi

Maybe you need to add in your readme.txt that the module will only work for public filesystems.

I leave it to 'needs review'.

Good work by the way !

There are still files other than README.txt in the master branch, make sure to remove them. See also step 5 in http://drupal.org/node/1127732
Review of the 7.x-1.x branch:

• Drupal Code Sniffer has found some code style issues (please check the Drupal coding standards). See attachment.

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. Get a review bonus and we will come back to your application sooner.

FILE: ...areview/sites/all/modules/pareview_temp/test_candidate/iip_image.module
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 2 WARNING(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
 309 | WARNING | Line exceeds 80 characters; contains 163 characters
 315 | WARNING | Line exceeds 80 characters; contains 114 characters
--------------------------------------------------------------------------------

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

don't block deeper reviews because of these few minor issues found by automated tools

most coding standarts are suggestions, you must not follow all of them perfectly, but whenever possible

Manual review

_iip_image_resize() uses styles' path.

If you're not using image styles, you should not put thumbnails in that directory and use your own. Or you could remove _iip_image_resize() altogether. See next point.

_iip_image_resize() is not necessary.

You're using _iip_image_resize() to create preview thumbnail in advance, but this is not necessary. You can use theme('image_style') to generate thumbnail. This would include use of 'preview_image_style' which is defined in iip_image_field_widget_info(), but currently not used.

A lot of string substitution to replace public:// stream URIs.

This is not necessary in a lot of places where Drupal core functions are used, because those are already aware of streams. I might be wrong, but I believe there is only one place where real path is required and that is when passing file path to IIP server. This is also required for _imagemagick_convert_exec(), but I suggest you use _imagemagick_convert() to remove the need to do so.

Dead code in iip_image_field_formatter_view()

if (isset($link_file)) guards against not-existent variable.

When $instance['settings']['file_directory'] is defined, assignment inside if ($instance['settings']['file_directory']) produces wrong absolute path if field is configured to display original (not styled) image. This might be potential security risk.

Permission to view image

Define permission for accessing 'image_iip/%'.

Make it user friendly

You should make this module a bit more user friendly by incorporating some of your README.txt documentation inline. My suggestion would be:

• Check that `convert' exists using _imagemagick_check_path() and reporting errors to user. Apparently, just enabling imagemagick module is not enough :)
• Inform user that TIFF images should have tiles or IIP Viewer will fail. Although IIP Server logs that TIFF image is not tiled, JavaScript alert "Unexpected response from server /fcgi-bin/iipsrv.fcgi" is no help at all.
• Allow to configure IIP Server's location.

Hi Roman,

I'm Denis, one of the IIPImage maintainers. Thanks for the great work, I use Drupal a lot, but I would never had the time to create a module!
If you agree we can get in touch,

best regards,
Denis

Closing due to lack of activity. Feel free to reopen if you are still working on this application.

Removed any mentioned CCK words.