Multiple upload alone

Hi,

I've taken a look through your module. First of all I installed it, and it worked as advertised! I have a few comments below.

• I think it would be neater to offer this functionality directly on the node edit form (use a hook_form_alter() to modify the upload element), rather than as a new local task. Is there a particular difficulty in that approach?
• If you do use a new local task, I think convention expects the task name to be a short command, eg. upload zip
• You don't use Doxygen style headers to describe functions. Some custom functions don't describe what they do, or their preconditions (eg. _multiuploadalone_writefile() references arg(1) as the nid so can only be called from a node page). Also instead of cfr. you can use @see.
• I don't think you should have the version number in the .info file.
• hook_menu
  • The access callback checks for edit access, but not for upload access. An unauthorized user can attach files to nodes in this way.
  • If the page is just a form, it's typical to use a page callback of drupal_get_form() and pass the name of the form as an argument.
  • You could move pretty much all of the module into a separate file (to lower memory use when on shared hosts) by specifying the new file name in the 'file' key of the menu item.
• In multiuploadmodule.module on line 81 you start renaming potentially executable files, to help prevent exploits. It seems to me (I'm no expert) that as you've already munged the filename, you should just ensure that the extension (if there is on) matches one of the members of the $extensions array.
• $tmp_filepath on line 200 seems unnecessary, as do _multiuploadalone_upload_page_init() and _multiuploadalone_upload_content (but maybe they're for something you're planning on adding?).
• I see you've already tried to use a PHP zip library - I think that would be a nice thing to add if you can.
• The node edit form
  • I personally find the function of the multiupload button a bit counter-intuitive - it actually attempts to save the node, which is an action that I really want to know when I trigger.
  • If you have JS enabled you will end up with an unpublished node, if not you might not. The JS behavior can be mirrored server-side by displaying a confirmation form if the user submits the form via the multiupload button and they have the node set to published.
  • There's no warning about the JS unpublishing, and I'd be a bit surprised as a user if I specifically set the node to published, and then when I saved it was unpublished.
  • In the JS you use $(document).ready(function(); it's preferred to use Drupal behaviors.
  • The JS for changing the button color can be achieved via CSS I believe.

Hi chirale,

Sorry for the delay. I've taken a look through the latest, and the code and functionality are much nicer to my mind at least! I've put some notes below, but most of these are really quite minor I think.

multiuploadalone.module

1. l17: you can omit the final argument, as it's assumed to be the same as the module name if not given. Also, this include could be made optional (only if there's something in the upload element). Not sure that'll actually make a noticeable difference tho :)
2. l19: you forgot to call t(). I also wonder if it might not be an idea to record this using watchdog(). Oh wait, looking closer it seems that it's not possible for this to ever error currently!!
3. l42: there's another missed t().
4. l43: the upload element's set as required. I don't understand why this is, and it doesn't seem to work for me anyway! (The element's marked as required, but I can still save without a file.) Maybe I'm missing something :)
5. l46: I think it would be cooler to pull in the same limits that upload.module uses. The only way I can see to do that is using _upload_file_limits().
   

   multiuploadalone.inc

6. l33: I think it's better to use drupal_install_mkdir(). (The name does make you think it's only for install time, but the code seems generic.)
7. l28-41: I find it very confusing to have an argument called $mode, give it a default value, then overwrite it (l33), then do a switch on it as if it hadn't been overwritten (l34), then repeat the code from the switch (l50-51) because the switch didn't do what you'd first expect!
8. l36: I'd indent this to make it easier to understand.
9. l51: It's a good idea to do a function_exists('exec') and respond gracefully if not, as exec() can be disabled for security reasons. You can also check for this in hook_requirements(), but I don't think that should replace the inline check, as someone might enable the module on a host with support and then migrate it to a different one.

General

10. I think the way the multiupload element works now is much more nicely integrated and fine for a live project. As a future direction I think it would be neater still if you could integrate with the existing upload element. You could add a checkbox to select if it's to be extracted or not (in case someone wanted to upload a zip and keep it that way).
11. In _multiuploadalone_upload I personally find it weird to return TRUE on error, but maybe that's just me :)
12. There's still one or two places that aren't fully complying with the coding standards. I'm not sure how important it's considered to cross all of these Ts, but I'll point it out just for completeness!
    1. Some of the functions and hooks don't have Doxygen style headers.
    2. There's a few lines that go over 80 characters and aren't declarations/definitions.
13. It might be an idea to mention that files starting with a dot will be ignored (maybe).
14. Given you're thinking of maybe expanding to support other archive types, perhaps multiuploadalone_upload_zip_validate() could be renamed to something a bit more generic? Following on from that, and just as a suggestion, if you know now that you might be adding support for other archive types, you can code with that in mind: currently where you have the extension zip and mime type application/zip hardcoded, they could be replaced by a function call that returns an array. This might make it easier to modify later.

HTH

Andy

Hi,

Sorry for the delay in getting back to you.

I've put some notes below, and indicated the issues I think are blockers by asterisks*.

I try to use that function, but I have to include install.inc

Ah sorry I didn't realise that: I think you definitely don't want to use it - it's not worth including a 22KB file just for that imho.

multiuploadalone.module

1. * The example of using #file_validators from upload_element's README.txt is this:

   '#file_validators' => array(
     'file_validate_size' => array(16384),
     'file_validate_extensions' => array('txt gif patch diff jpg jpeg'),
   ),

   So on line 63 I think you should have 'file_validate_extensions' => array(implode(' ', $settings['allowed_extensions'])),. Similarly, $allowed_extensions on line 124 should be implode(', ', $settings['allowed_extensions']) (or something similar). Currently it only displays the first element of the array.

2. *
   

   I've added phase runtime along install phase to alert the administrator on error.

   That's great. However I don't think it's very nice to manually call hook_requirements() (line 39) on the node edit form:

   • You're calling exec() on every nodeform build, which is not good from a performance point of view (it wouldn't scale well) as I understand.
   • The docs say the hook is for the status page, so developers might expect to be able to add more heavy code to that function, without realising it's actually also called on every nodeform build.
   • You're including the install file, which might grow (eg. often hook_schema() is large).

   Personally I think it's best to keep hook_requirements absolutely the same, and just use a simple function_exists('exec') for the nodeform check.

3. I think it's worth wrapping lines 19-22 in if (user_access('upload files')) as that way you don't unnecessarily load the include file for users who won't need it (lower memory usage).

multiupload.inc

1. * I think you want to delete l53-54 (currently the command's executed twice).
2. * l59: you've commented out the rmdir - to me that's necessary. Is there a particular problem you were having with it? As it is, I think you'll end up with loads of directories in the tmp dir.
3. The following's a bit vague to me.
   

   Some files could be renamed or ignored on upload

4. If you enable the module via Drush, you get the requirements message repeated twice. I presume that Drush first calls the install check and then the runtime check. You could use static variable to keep track of whether it's done a check to avoid this. (Obviously this is really really minor!)

HTH

Looks good to me.

* Git release branch is missing, see http://drupal.org/node/1015226
* README.txt is missing
* Remove the translations folder, translations are done on localize.drupal.org
* Remove empty install/uninstall functions
* "// Insert files within archive in {upload} table after save (new node) or update (existing node)" comment lines should not exceed 80 characters and should end with a "."
* "// Load" load what? be more specific in a comment

• remove LICENSE.txt, this will be added by drupal.org packaging automatically
• multiuploadalone_nodeapi(): do not use "/** ... */" style comments in function bodies, use "//" there.
• Also in multiuploadalone_form_alter() and in other places.
• "exec($cmd, $output, $return_vars);": executing shell stuff is never a good idea ... why can't you use http://www.php.net/manual/en/book.zip.php ?
• "$file->destination = file_destination(file_create_path($dest .'/'. $file->filename), $replace);": code style, there should be a space before and after ".". Also elsewhere.

• multiuploadalone_requirements(): "foreach ($execoutput as $info) {": you don't do anything in that loop, remove it.
• "/** create file object **/": replace the comment style with "//"
• "A blank line forms a paragraph. There should be no trailing white-space": I think this is a left over from the documentation template?

Thos things are quite minor, otherwise RTBC for me. Now while we are waiting for the access grants to create full projects would you be so kind to do a review of the other applications pending? Just pick one from this list: http://drupal.org/project/issues/projectapplications?status=8

Two smaller issues:

Please take a moment to make your project page follow tips for a great project page.

This is not great for translatability because it requires the version to be on the end:

35 'value' => $t('Unzip command available. Version information: ') . check_plain($info),

better is

35 'value' => $t('Unzip command available. Version information: @version', array('@version' => $info),

The @ substitution includes a call to check_plain.

Thanks for your contribution, chirale! Welcome to the community of project contributors on drupal.org.

I've granted you the git vetted user role which will let you promote this to a full project and also create new projects as either sandbox or "full" projects depending on which you feel is best.

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

As you continue to work on your module, keep in minde: Commit messages - providing history and credit and Release naming conventions.