[D7] IMCE unzip

Very nice module.
I have tried and tested the module - and works perfectly for me.
Few comments:

Line 120: Potential problem: drupal_set_message() only accepts filtered text, be sure all !placeholders for $variables in t() are fully sanitized using check_plain(), filter_xss() or similar. (Drupal Docs)

    drupal_set_message(t('Error for ' . check_plain($file->filepath) . ' = ' . imce_unzip_err($zip)), 'error');

severity: criticalLine 272: Potential problem: drupal_set_message() only accepts filtered text, be sure all !placeholders for $variables in t() are fully sanitized using check_plain(), filter_xss() or similar. (Drupal Docs)

    drupal_set_message(t('Error for ' . check_plain($file->filepath) ), 'error');

Once these issues are fixed, I will gladly mark the module as "Reviewed and tested by community"

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

• Run coder to check your style, some issues were found (please check the Drupal coding standards):
  

  Severity minor, Drupal Commenting Standards, Internationalization, Drupal Security Checks, Drupal SQL Standards, Drupal Coding Standards
  
  sites/all/modules/pareview_temp/test_candidate/imce_unzip.module:
   +4: [minor] Comment should be read "Implements hook_foo()."
   +23: [minor] Comment should be read "Implements hook_foo()."
   +69: [normal] The $string argument to t() should not begin or end with a space.
   +253: [normal] The $string argument to t() should not begin or end with a space.
  
  Status Messages:
   Coder found 1 projects, 1 files, 2 normal warnings, 2 minor warnings, 0 warnings were flagged to be ignored
  

• Lines in README.txt should not exceed 80 characters, see the guidelines for in-project documentation.
• Remove the translations folder, translations are done on http://localize.drupal.org
• Remove "project" from the info file, it will be added by drupal.org packaging automatically.
• @file doc block is missing in the module file, see http://drupal.org/node/1354#files .
• Comments: there should be a space after "//", see http://drupal.org/node/1354#inline
  

  imce_unzip.install:11:  //update profiles. add unzip settings.
  imce_unzip.install:20:  //register custom content function
  imce_unzip.install:25:  //register custom profile process
  imce_unzip.install:35:  //update profiles. delete mkdir settings.
  imce_unzip.install:44:  //unregister custom content function
  imce_unzip.install:49:  //unregister custom profile process
  imce_unzip.js:2://add hook:load. process unzip form 
  imce_unzip.js:11:	  //add hook:navigate. set dirops visibility
  imce_unzip.js:18://implementation of imce.hookOpValidate	
  imce_unzip.js:33://implementation of imce.hookOpSubmit
  imce_unzip.js:41://change fops states.
  imce_unzip.js:60://fix browser cache showing the non-cropped image when filename is not changed.
  imce_unzip.js:78://custom response. keep track of overwritten files.
  imce_unzip.js:89:	//clean up 
  imce_unzip.module:141:        //drupal_set_message( "filename=".print_r($filename,true)." zdir =". print_r($zdir,true)." ext=".print_r($ext,true), 'success' );
  imce_unzip.module:185:            //validation ok
  imce_unzip.module:322:    //array('unzip' => array_map('rawurlencode', $imce['unzip']));
  imce_unzip.module:328:  //create file object
  imce_unzip.module:329:  //resize image
  imce_unzip.module:351:  //save the file
  

• ./imce_unzip.module: comment lines should break at 80 characters, see http://drupal.org/node/1354#general
  

          //drupal_set_message( "filename=".print_r($filename,true)." zdir =". print_r($zdir,true)." ext=".print_r($ext,true), 'success' );
  

• Comments should be on a separate line before the code line, see http://drupal.org/node/1354#inline
  

  ./imce_unzip.js:66:  return imce.unzipFids[fid] ? (url +'?'+ imce.unzipFids[fid]) : url;//add suffix to prevent caching.
  ./imce_unzip.js:84:	  if (imce.conf.dir && imce.cache[imce.conf.dir]) {//load from the cache
  ./imce_unzip.js:88:	  else $.ajax(set);//live load
  ./imce_unzip.module:68:    '#submit' => $imce['perm']['unzip'] ? array( 'imce_unzip_submit' ) : NULL, //permission for submission
  ./imce_unzip.module:210:            if ( $width && $height ) {  // process image
  ./imce_unzip.module:237:            else {  // process regular files
  ./imce_unzip.module:257:          } //end validator empty
  ./imce_unzip.module:258:        } //end ext != filename
  ./imce_unzip.module:259:      } //end nb extracted files
  ./imce_unzip.module:260:    } //end while $zip_entry = zip_read($zip)
  ./imce_unzip.module:270:  } //end zip
  

• ./imce_unzip.module: all functions should have doxygen doc blocks, see http://drupal.org/node/1354#functions
  

  
  function _imce_unzip_scale_image($imce, $file, $maxw, $maxh, $buf) {
  

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

  function imce_js_unzip(&$imce) {
  

• There should be no space after the opening "(" of a control structure, see http://drupal.org/node/318#controlstruct
  

  imce_unzip.install:13:  foreach ( $profiles as $i => $profile ) {
  imce_unzip.install:14:    foreach ( $profile['directories'] as $j => $directory ) {
  imce_unzip.install:37:  foreach ( $profiles as $i => $profile ) {
  imce_unzip.install:38:    foreach ( $profile['directories'] as $j => $directory ) {
  imce_unzip.module:7:  foreach ( element_children($form['profile']['directories']) as $key ) {
  imce_unzip.module:27:  if ( imce_perm_exists($imce, 'unzip') ) {
  imce_unzip.module:51:  if ( $imce['maxfilestoextract'] ) {
  imce_unzip.module:52:    if ( $imce['maxfilestoextract'] != 0 ) {
  imce_unzip.module:61:  if ( $imce['dimensions'] ) {
  imce_unzip.module:87:  if ( !empty($unzip) ) {
  imce_unzip.module:97:  if ( file_exists($filepath) ) {
  imce_unzip.module:112:  if ( $file->filemime != 'application/zip' ) {
  imce_unzip.module:119:  if ( !is_resource($zip) ) {
  imce_unzip.module:124:  if ( $zip ) {
  imce_unzip.module:128:    if ( $imce['extensions'] != '*' ) {
  imce_unzip.module:136:      if ( $nb < $imce['maxfilestoextract'] || $imce['maxfilestoextract'] == 0 ) {
  imce_unzip.module:143:        if ( $zdir != '.' && $zdir != '__MACOSX' ) {
  imce_unzip.module:145:          if ( !file_exists($newdirpath) ) {
  imce_unzip.module:146:            if ( module_exists('imce_mkdir') && $imce['perm']['mkdir']
  imce_unzip.module:153:              foreach ( explode('/', $zdir) AS $k ) {
  imce_unzip.module:155:                if ( !file_exists($dirpath . '/' . $tmp) ) {
  imce_unzip.module:169:        if ( drupal_strtolower($filename) != $ext && (!preg_match('/^\._/', $filename)) && (!preg_match('/^__MACOSX/', $filename)) ) {
  imce_unzip.module:179:          if ( !empty($validators) ) {
  imce_unzip.module:199:            if ( zip_entry_open($zip, $zip_entry, "r") ) {
  imce_unzip.module:203:              if ( $image ) {
  imce_unzip.module:210:            if ( $width && $height ) {  // process image
  imce_unzip.module:212:              if ( $width > $maxw || $height > $maxh ) {
  imce_unzip.module:214:                if ( $scaledFile ) {
  imce_unzip.module:227:                if ( $_file = db_fetch_object(db_query("SELECT f.* FROM {files} f WHERE f.filepath = '%s'", $newFile->filepath)) ) {
  imce_unzip.module:239:              if ( $fp ) {
  imce_unzip.module:244:                if ( $_file = db_fetch_object(db_query("SELECT f.* FROM {files} f WHERE f.filepath = '%s'", $newFile->filepath)) ) {
  imce_unzip.module:263:    if ( $imce['perm']['delete'] && $deleteZipFile ) {
  imce_unzip.module:265:      if ( !empty($deleted) ) {
  imce_unzip.module:307:  foreach ( $zip_errors as $const_name => $error_message ) {
  imce_unzip.module:308:    if ( defined($const_name) and constant($const_name) === $errno ) {
  imce_unzip.module:319:  if ( $imce['perm']['unzip'] ) {
  imce_unzip.module:337:  if ( !image_scale($temp, $file->filepath, $maxw, $maxh) ) {
  imce_unzip.module:345:  if ( $_file = db_fetch_object(db_query("SELECT f.* FROM {files} f WHERE f.filepath = '%s'", $file->filepath)) ) {
  

• There should be no space after the opening "(" of an array, see http://drupal.org/node/318#array
  

  imce_unzip.install:12:  $profiles = variable_get('imce_profiles', array( ));
  imce_unzip.install:21:  $funcs = variable_get('imce_custom_content', array( ));
  imce_unzip.install:26:  $funcs = variable_get('imce_custom_process', array( ));
  imce_unzip.install:36:  $profiles = variable_get('imce_profiles', array( ));
  imce_unzip.install:45:  $funcs = variable_get('imce_custom_content', array( ));
  imce_unzip.install:50:  $funcs = variable_get('imce_custom_process', array( ));
  imce_unzip.module:53:      $maxFilesNb = '<div><em>' . t('Only %nb files will be extracted from the zip file', array( '%nb' => $imce['maxfilestoextract'] )) . '</em></div>';
  imce_unzip.module:62:    $maxSizeImage = '<div><em>' . t('Extracted image files exceeding the maximum allowed image size (%dimensions) will be scaled down.', array( '%dimensions' => $imce['dimensions'] )) . '</em></div>';
  imce_unzip.module:68:    '#submit' => $imce['perm']['unzip'] ? array( 'imce_unzip_submit' ) : NULL, //permission for submission
  imce_unzip.module:84:  $unzip = imce_process_files($form_state['values']['filenames'], $imce, 'imce_unzip_file', array( $deleteZipFile ));
  imce_unzip.module:88:    drupal_set_message(t('Unzip of %file successful', array( '%file' => $unzip[0] )));
  imce_unzip.module:157:                  imce_mkdir_batch($imce, array( $k ));
  imce_unzip.module:215:                  drupal_set_message(t('Extraction and scaling of %filename.', array( '%filename' => $scaledFile->filename ), 'succes'));
  imce_unzip.module:226:                $update = array( );
  imce_unzip.module:234:                drupal_set_message(t('Extraction of %filename.', array( '%filename' => $newFile->filename ), 'succes'));
  imce_unzip.module:243:                $update = array( );
  imce_unzip.module:250:                drupal_set_message(t('Extraction of %filename.', array( '%filename' => $newFile->filename ), 'succes'));
  imce_unzip.module:253:                drupal_set_message(t("Can't create %filename: ", array( '%filename' => $newFile->filename ), 'error'));
  imce_unzip.module:264:      $deleted = imce_process_files(array( $file->filename ), $imce, 'imce_delete_file');
  imce_unzip.module:266:        drupal_set_message(t('File deletion successful: %files.', array( '%files' => utf8_encode(implode(', ', $deleted)) )));
  imce_unzip.module:338:    drupal_set_message(t('Can not scale image %filename', array( '%filename' => $file->filepath ), 'error'));
  imce_unzip.module:344:  $update = array( );
  

• All text files should end in a single newline (\n). See http://drupal.org/node/318#indenting
  

  ./imce_unzip.install ./imce_unzip.js ./imce_unzip.module
  

This automated report was generated with PAReview.sh, your friendly project application review script. Go and review some other project applications, so we can get back to yours sooner.

Definitely is an interesting module! :) Please note that I haven't actually run/tested the code. Thanks.

Whups, forgot to change the status! ;)

Stephen -- check out klausi's PAReview.sh script at http://drupal.org/sandbox/klausi/1320008

All of the "needs review" feedback you're seeing in your project application is coming from the script's output.

Agree that having all possible checks present in coder would be ideal, and I think there's been quite a bit of discussion around that already. There's also been talk of completely automating/integrating some of the coding standards reviews upon project application. The process is obviously not perfect yet, but it's a step in the right direction.

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

• ./imce_unzip.js: comment lines should break at 80 characters, see http://drupal.org/node/1354#general
  

    // fix browser cache showing the non-cropped image when filename is not changed.
  

• Comments should be on a separate line before the code line, see http://drupal.org/node/1354#inline
  

  ./imce_unzip.module:264:          } // end validator empty
  ./imce_unzip.module:265:        } // end ext != filename
  ./imce_unzip.module:266:      } // end nb extracted files
  ./imce_unzip.module:267:    } // end while $zip_entry = zip_read($zip)
  ./imce_unzip.module:277:  } // end zip
  

• ./imce_unzip.module: all functions should have doxygen doc blocks, see http://drupal.org/node/1354#functions
  

  
  function _imce_unzip_scale_image($imce, $file, $maxw, $maxh, $buf) {
  

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

  function imce_js_unzip(&$imce) {
  

• All text files should end in a single newline (\n). See http://drupal.org/node/318#indenting
  

  ./imce_unzip.info ./imce_unzip.install ./imce_unzip.module ./imce_unzip.js
  

This automated report was generated with PAReview.sh, your friendly project application review script. Go and review some other project applications, so we can get back to yours sooner.

Switched back to needs review, so in-depth reviews won't be blocked by coding standart issues.

Automated review attached -- 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. Go and review some other project applications, so we can get back to yours sooner.

Review of the 6.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.

manual review:

• imce_unzip_install(): the tasks in there look like they belong into hook_enable(), not hook_install().
• same for hook_uninstall() and hook_disable().
• "drupal_set_message(check_plain(t('Error for ' . $file->filepath . ' = ' . imce_unzip_err($zip))), 'error');": use the correct placeholders in t(), then you don't need to do the check_plain() yourself.
• imce_unzip_err(): you should use t() in that function, so that translation extractors can pick up the string literals.

I know, getting feedback on your application took long, you can speed up the process with #1410826: [META] Review bonus.

No reviewer feedback for over 7 weeks ... I'm going to mark this RTBC due to the process stalling out, and the 7 week wait for reviewer feedback with no response.

Apologies for the delay, thank you for your patience, and let's get this pushed through!

Thanks a lot for this really cool module! And holy moly, am I impressed that you were able to have zero code standard warnings -- that takes total dedication, and we want that sort of dedication and collaboration in the community!

I agree with both you and klausi about the hook_install() and hook_uninstall() tasks -- although I lean more with klausi with the tasks modifying user profiles (what about new users since the disabling?). I think it is something that you'll need to sit back and think through. Absolutely not an application blocker, though!

Your code is gorgeous. My only suggestion is to add a blank line to the end of your files -- it minimizes conflicts in version control, so I'm sure you'll grow to love it.

I am recommending that your application be approved. Now make more really cool modules like this one ;)

Congratulations, Stephen! You are now a "vetted" git user and can promote experimental sandboxes to full projects. Very excited to see this and other contributions from you!

Thank you gargsuchi, natemow, et al for your reviews, and thank you jthorson, ceardach for bringing this back up.