[D7] Image Compresssion TinyPNG/JPG

Automated Review

[pareview.sh / drupalcs / coder] Tests are good:Pareview

Manual Review

Individual user account

[Yes: Follows] the guidelines for individual user accounts.

No duplication

[Yes: Does not cause] module duplication and/or fragmentation.

Master Branch

[Yes: Follows] the guidelines for master branch.

Licensing

[Yes: Follows] the licensing requirements.

3rd party assets/code

[Yes: Follows] the guidelines for 3rd party assets/code.

README.txt/README.md

[Yes: Follows] the guidelines for in-project documentation and/or the README Template.

Code long/complex enough for review

[Yes: Follows] the guidelines for project length and complexity.

Secure code

[Yes: Meets the security requirements

Coding style & Drupal API usage

1. The code looks good, the only thing i would recommend is to use less empty lines ( in function hook_menu for example).
2. Recommended is to use elements requirements, installation, configuration on the project page to turn it into something like this.

This review uses the Project Application Review Template.

In the past I was looking for this integration. I personally, love TinyPNG. There is a place holder that you should consider asking the developer if I can give you the name of this project - https://www.drupal.org/project/tinypng.

I believe this is a Duplication of a module - ImageAPI Optimize (or Image Optimize) have a patch to integrate their module with TinyPNG services. Duplication is not a blocker; however, the Drupal community holds a strong "collaboration rather than competition" ethos, which values joining forces on improving one awesome project rather than building several sub-standard ones that overwhelm end users with choices.

Here are few minor warning from coder:

• tinypngjpg.scald.manual.inc
  

  Line 19: in most cases, replace the string function with the drupal_ equivalent string functions
    if (!empty($key) && strlen($key) > 0) {

• tinypngjpg.inc
  

  Line 41: in most cases, replace the string function with the drupal_ equivalent string functions
      $headers = substr($response, 0, curl_getinfo($request, CURLINFO_HEADER_SIZE));
  Line 43: in most cases, replace the string function with the drupal_ equivalent string functions
        if (substr($header, 0, 10) === "Location: ") {
  Line 46: in most cases, replace the string function with the drupal_ equivalent string functions
            CURLOPT_URL => substr($header, 10),
  Line 102: in most cases, replace the string function with the drupal_ equivalent string functions
    $json_undecoded = str_replace(substr($response, 0, curl_getinfo($request, CURLINFO_HEADER_SIZE)), '', $response);
  Line 143: in most cases, replace the string function with the drupal_ equivalent string functions
      if (substr($header, 0, 19) === "Compression-Count: ") {
  Line 144: in most cases, replace the string function with the drupal_ equivalent string functions
        variable_set('tinypngjpg_api_key_allowance', substr($header, 19));

• tinypngjpg.scald.batch.inc
  

  Line 112: in most cases, replace the string function with the drupal_ equivalent string functions
    if (!empty($key) && strlen($key) > 0) {

No complains from Pareview.sh - http://pareview.sh/pareview/httpgitdrupalorgsandboxchenderson2457559git

I do not see any blockers; however, consider to integrate this module with the ImageAPI Optimize (or Image Optimize) instead of creating your own project.

@chenderson,

Sounds great, if you wish to promote your own project due the differences between your project and the ImageAPI Optimize, try to talk to the developer that took the tinypng name to see if would give you that URL.

@chenderson,

I usually use Coder module alone without Sniffer because it show different warnings and errors from Coder Sniffer. In addition, Coder Sniffer is integrate with Pareview.sh so there is need for me to used it locally. If you want to get those warning, you would have to install the coder module, then find your module in the modules page (admin/modules) and click Code Review. From there you would have to click on SELECTION FORM and select the check box "minor (most)". This would make your review very picky and it would show you those warning.

@chenderson,

Apparently, there is another project trying integrate Drupal with TinyPng #2471060: [D7] TinyPNG On Upload, which is also been under-review.

Next up on my review queue.

Automated Review

Review of the 7.x-1.x branch (commit a5a9241):

• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

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

Secure code

(*) There is a security problem with this module. As part of project review mentoring, I am assigning this to @darol100 to find it.

Coding style & Drupal API usage

Remove the .gitignore from the repo. This like this should go in your global ignore.

Why cURL and not drupal_http_request()?

All of your variable_get() should have a default value.

Why the exif_imagetype() and not use the MIME type that is in the $file object? Comment needed.

tinypngjpg_compress_for_batch(), don't concatenate with t(). Use placeholders.

tinypngjpg_bulk_import_form() has untranslated strings. Use t() w/ placeholders.

tinypngjpg_bulk_import_form_submit(), you don't need the leading slash in the drupal_goto().

In general single quoted strings are preferred over double-quoted ones.

tinypngjpg_start_batch_process(). Avoid using header() and die() directly. See http://drupal.stackexchange.com/questions/91011/can-a-batch-process-be-s...

tinypngjpg_process_finished(), the check_plain is superflous here. There is no user input.

In the .module file you shouldn't have any logic at the top level. You need to move these includes somewhere else.

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

The security problem is the blocker. Otherwise, I didn't see anything.

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This review uses the Project Application Review Template.

OK, no response about the security problem.

In tinypngjpg_admin_settings(), line 15, the variable_get() is used directly in #markup without sanitization. This is user input and considered unsafe. Use t() w/ a placeholder. See https://www.drupal.org/node/28984

Sorry for the delay. The review admins have a big queue to go through, and we do it by age. I will check the security fix tonight (which was the only blocker).

And to answer your question, third-party content is considered unsafe. In general, we require sanitization here.

I read `git diff a5a9241`, and my security issue was addressed via proper use of t() w/ an @arg placeholder. Nothing else jumped out at me. Setting RTBC and assigning to @er.pushpinderrana, if he has time.

Automated Review

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

Review of the 7.x-1.x branch (commit 1884ed5):

• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

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

Coding style & Drupal API usage

(*) tinypngjpg_bulk_import_form(): doc block is wrong, this is not a hook. See https://www.drupal.org/coding-standards/docs#forms. Same applies for tinypngjpg_bulk_import_form_submit(), tinypngjpg_compress_form_submit() and tinypngjpg_batch(). Need a proper doc block with @param and @return, check for other functions too.

(+) tinypngjpg_batch(): BatchAPI code looks wired to me. That's not the right way to do this at all. Use the batch sandbox to store details from one batch iteration to the next, it's what it's there to do.

foreach ($images as $image) {
    $batch['operations'][] = array(
      'tinypngjpg_process',
      array($image),
    );
  }

You should only add the operation once, and let the operation iterate over a set number of results, using the sandbox to mark how far it got each time. If you go with sandbox then there is no need to use $_SESSION to hold batch results.

tinypngjpg_bulk_import_form_submit(): IMO do NOT use drupal_goto() in form submit handlers, $form_state['redirect'] is the correct way.

tinypngjpg_extract_error_message(): Instead of using json_decode(), should use drupal_json_decode().

tinypngjpg_save_error(): Instead of using time(), should use REQUEST_TIME.

(*) tinypngjpg_compress(): drupal_set_message(check_plain('Compression failed: ' . $error['Error'] . ', ' . $error['Message']), 'error');The check_plain does not suit for static text. Use t() with "@" or "%" instead, see https://api.drupal.org/api/drupal/includes!bootstrap.inc/function/t/7

tinypngjpg_init(): hook_init() is run on every single page request and even on drush invocations. Could you add a comment here why you can't use hook_page_build() or others?

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

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.