1. Name:
    IIP Image
  2. Description:
    This module provides integration of IIPImage to Drupal + proper image converting.
    Read about IIPImage here: http://iipimage.sourceforge.net/

    My project creates a image field with a custom widget and formatter that provide TIFF image extension support, converting process and validation. Converting settings also made available in settings.
    An uploaded user's image goes through certain convert process and passes to IIP Server mod, which displays the image in Lightbox.
    Also provided custom thumbnail creating for uploaded images with an adjustable settings.

  3. Link to a sandbox project:
    http://drupal.org/sandbox/barbun/1286086
  4. Git repository link:
    git clone --branch 7.x-1.x http://git.drupal.org/sandbox/barbun/1286086.git iip_image
    cd iip_image
  5. For Drupal 7 use.
  6. I haven't seen any similar modules before.
    I'm a newbie at Drupal, at the moment i have 2 month of experience in it but my steps are confident and balanced.
    I have already posted a patch for Menutrails module that port it to Drupal 7 ( http://drupal.org/node/940206 , post #25), and also
    patch for lightbox (http://drupal.org/node/1287270)

    Thank you!
    My best,
    ROMAN

Files: 
CommentFileSizeAuthor
1.jpg110.91 KBbarbun

Comments

patrickd’s picture

Status: Needs review » Needs work

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

barbun’s picture

Status: Needs work » Needs review

Thank you, Patrick!

I fixed all you mentioned.

klausi’s picture

Status: Needs review » Needs work
  • 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?
klausi’s picture

Issue summary: View changes

Changed git clone path due to created new release branch

barbun’s picture

Status: Needs work » Needs review

Vielen Dank, Klaus)
Sorry, it took that long to fix problems, i was really busy with my work projects.

Concerning javascript includes in image_iip_page(), yes it has to be like that, because there's a special page. In other case it won't work.

My best,
ROMAN

Robertas’s picture

Status: Needs review » Needs work

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:

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

barbun’s picture

Thanks, Robertas!

Cleaned it, but still i have these two lives:

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
--------------------------------------------------------------------------------

The lines are:

310    _imagemagick_convert_exec($image_path . $image_ext . ' -define tiff:tile-geometry=' . $tilesize . ' -compress jpeg "ptif:' . $image_path . $image_dest . '"');
316    _imagemagick_convert_exec($image_path . $image_ext . '[0] -compress jpeg ' . $image_path . $image_dest . '');

If i break the line to make it less than 80 characters in a line it looks really awful and less understandable. So i left it like that, hope it's not a big deal. If it is - ok, i will adjust it.

My best,
ROMAN

barbun’s picture

Status: Needs work » Needs review

Thanks, Robertas!

Cleaned it, but still i have these two lines:

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
--------------------------------------------------------------------------------

The lines are:

_imagemagick_convert_exec($image_path . $image_ext . ' -define tiff:tile-geometry=' . $tilesize . ' -compress jpeg "ptif:' . $image_path . $image_dest . '"');
_imagemagick_convert_exec($image_path . $image_ext . '[0] -compress jpeg ' . $image_path . $image_dest . '');

If i break the line to make it less than 80 characters in a line it looks really awful and less understandable. So i left it like that, hope it's not a big deal. If it is - ok, i will adjust it.

My best,
ROMAN

targoo’s picture

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 !

barbun’s picture

Thanks, David!

Yes, it's still TO-DO to make it work also with private filesystems. Would be an awesome feature, i suppose.

prashantgoel’s picture

Status: Needs review » Needs work

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:

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

patrickd’s picture

Status: Needs work » Needs review

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

barbun’s picture

Thanks, master branch is cleaned.

As for Drupal Code Sniffer, I've mentioned that in my post #7. It breaks the readability of the code if I break those lines.
I don't know if it is a MUST or not, so I just left it like that.
If somebody can tell me for sure should I break those lines or not, I would be really thankful:)

patrickd’s picture

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

donatasp’s picture

Status: Needs review » Needs work

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.
denix’s picture

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

barbun’s picture

Hi, Denis!

I made you a maintainer for this project! Thank you!

klausi’s picture

Status: Needs work » Closed (won't fix)

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

klausi’s picture

Issue summary: View changes

Removed any mentioned CCK words.