[D7] Image progressive

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.

• Your repository link includes your username, the correct one is: http://git.drupal.org/sandbox/depresz/2222075
• ./image_progressive.module: all functions should be prefixed with your module/theme name to avoid name clashes. See http://drupal.org/node/318#naming
  function image_gd_image_progressive(stdClass $image, array $params) {

Reported by pareview tool : http://pareview.sh/pareview/httpgitdrupalorgsandboxdepresz2222075

Module works as intended.

Only issue I can see if the naming of

function image_gd_image_progressive(stdClass $image, array $params)

Should perhaps be
image_progressive_image_gd(stdClass $image, array $params)

To conform with coding standards

@depresz Module works in terms of functionality.
Nice and simple module.
But, Little more of documentation on project page and listing advantages of using Progressive effect will help users to identify the real worth of the module.
Explain bit about Interlacing and how is it useful in terms of image loading performance. What does Progressive effect does to the image.

And below is the PAReview of the module ->
http://pareview.sh/pareview/httpgitdrupalorgsandboxdepresz2222075 which says that all the functions used in the module should be prefixed with the name of the module.
Change function image_gd_image_progressive(stdClass $image, array $params) to function image_progressive_gd_image(stdClass $image, array $params) { or something which seems more meaningful to you but with correct prefix.

Hi depresz,

Your README.txt is quite nice, I think it would be a good start for a documentation page. There is a small typo, which I corrected in the attached patch.

I found three issues with your code.

First, there is no need to call module_load_inc as there is no file to include! Simple remove this function call.

Second, there is no need for the wrapper function image_progressive_effect_inc. You can put everything directly into image_progressive_effect.

Third, the gd lib implementation of your effect needs to fit the naming convention to be actually called. It needs to be named image_gd_image_progressive as it needs to follow this pattern: image_{toolkitname}_{method}. (Actually, you have pointed that out in comment #3- I know that the other reviewers have suggested to do different. AFAIK there is no other way.)

This will cause PAReview to find an error or warning, however it will not work otherwise.

If you apply the patch, you'd get a positive review from me ;)

By the way: here is a way to check if an image is saved interlaced/progressive:
http://highloadtools.com/progressivejpeg

Automated Review

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

The only thing found is:
FILE: /var/www/drupal-7-pareview/pareview_temp/image_progressive.module

--------------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
--------------------------------------------------------------------------------
44 | ERROR | Unknown type hint "stdClass" found for $image
60 | ERROR | Unknown type hint "stdClass" found for $image
--------------------------------------------------------------------------------

It appears that there is some discussion in the community that "stdClass" should
be object instead, as mentioned at https://www.drupal.org/node/711918. That said,
this error certainly could be considered a false positive. Please note, I'm not
trying to be nit-picky with this, I'm curious as well and am not sure what the
standard is. I'm mentioning this only for the sake of thorough-ness and curiosity.

Manual Review

• Duplication
• Yes/No

No

• Multiple applications
• /No
• README.txt
• Yes - Follows the guidelines for in-project documentation.
• NOTE: The README and project documentation assumes that the installer is familiar with how to use the image
  manipulation built-in to Drupal. When I found the "CONFIGURATION" section in README.txt I was left with the
  opinion that this is not going to be intuitive to a new administrator, especially one who has not worked with
  Drupal's image system before. I would add some text that says something to the effect of:
  ---
  This module has no implicit configuration, it merely gives you another option when you are setting
  up your image settings. When you edit an image style at admin/config/media/image-styles, you will
  be given an option to add interlacing to the images. Here's how to do that:
  ---
  I've included a patch with my suggested placement.
• Master Branch
• Yes - Follows the guidelines for master branch.
• 3rd party code
• Yes - Follows the guidelines for 3rd party code.
• Yes
• Individual user account
• Yes - 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
  Just follow the fixes mentioned in the ealier comment by @valderama and I think this is a very
  solid and lightweight module!

Automated Review

PAReview.sh (See: http://pareview.sh/pareview/httpgitdrupalorgsandboxdepresz2222075git) produced:

FILE: /var/www/drupal-7-pareview/pareview_temp/image_progressive.module
--------------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
--------------------------------------------------------------------------------
44 | ERROR | Unknown type hint "stdClass" found for $image
60 | ERROR | Unknown type hint "stdClass" found for $image
--------------------------------------------------------------------------------

Please note that stdClass is not a base class. PHP classes do not automatically inherit from any class. All classes are standalone, unless they explicitly extend another class. Best practice is to not have a type hint in place for declaration for generic classes. Just omit the type hint, and document the type using @param object $image (as you've already done).

Manual Review

Individual user account

Yes: User depresz follow the guidelines for individual user accounts.

No multiple applications

Yes

No duplication

No.
This sounds like a feature that already exists in the ImageAPI Optimize project.
At least there is a patch commited to support progressive images.

Module duplication and fragmentation is a huge problem on drupal.org and we prefer collaboration over competition. Please clarify what your module does that is lacking in ImageAPI Optimize. You may also open an issue in the ImageAPI Optimize issue queue to discuss what you need. You should also get in contact with the maintainer(s) to offer your help to move the project forward. If you cannot reach the maintainer(s) please follow the abandoned project process.

If the above fails for whatever reason please get back to us and set this back to "needs review".

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements

3rd party code

Yes: Follows the guidelines for 3rd party code.

README.txt

No: Exists, but does not follow the guidelines for in-project documentation and the README.txt Template.

Code long/complex enough for review

No: The project is only 24 lines of code when comments and blanks are stripped. It does not follow the guidelines for project length and complexity. As already stated, this belongs to some other image API module that does more.

Secure code

Yes. (the module is tiny, with no user input and no admin interface) If "no", list security issues identified.

Coding style & Drupal API usage

See note under automatic review.

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.