[D7] webform2sftp

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxjribeiro2174305git

I'm a robot and this is an automated message from Project Applications Scraper.

This looks really good, and I even looked for security issues, missing hook_requirements, missing dependencies, and all that stuff, but could not find any major issues. I just found some minor issues:

- Missing t() function for some exception messages @ webform2sftp/class/webform2sftp.classes.inc
- I couldn't find out how this module would escape line breaks when printing variables. I might have missed it, but if this is not being handled it could break your file.

Other than that the code looks spotless.

I gone through the module and did not find any major issues. Just found some minor issues/improvements -

webform2sftp.install
Line 63 and 76: Comment should be read "Implements hook_install()." and "Implements hook_uninstall().".

webform2sftp.module
Line 83 : '=' is default value in the condition() method, If no operator is specified, = is assumed.
->condition('nid', $component['nid'])

See the documentation - https://drupal.org/node/310086

Good work Jean!

Looked it over again and tested out functionality as well... Looks really good and certainly much better than most modules out there.

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

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards).
  

  
  FILE: /home/klausi/pareview_temp/README.txt
  --------------------------------------------------------------------------------
  FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
  --------------------------------------------------------------------------------
   60 | ERROR | Files must end in a single new line character
  --------------------------------------------------------------------------------
  

• DrupalPractice has found some issues with your code, but could be false positives.
  

  
  FILE: /home/klausi/pareview_temp/webform2sftp.install
  --------------------------------------------------------------------------------
  FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
  --------------------------------------------------------------------------------
   137 | WARNING | Unused variable $cid.
  --------------------------------------------------------------------------------
  
  
  FILE: /home/klausi/pareview_temp/inc/webform2sftp.common.inc
  --------------------------------------------------------------------------------
  FOUND 0 ERROR(S) AND 4 WARNING(S) AFFECTING 3 LINE(S)
  --------------------------------------------------------------------------------
   237 | WARNING | Do not use the raw $form_state['input'], use
       |         | $form_state['values'] instead where possible
   238 | WARNING | Do not use the raw $form_state['input'], use
       |         | $form_state['values'] instead where possible
   238 | WARNING | Do not use the raw $form_state['input'], use
       |         | $form_state['values'] instead where possible
   444 | WARNING | Unused variable $file_type.
  --------------------------------------------------------------------------------
  
  
  FILE: /home/klausi/pareview_temp/inc/webform2sftp.tabledrag.inc
  --------------------------------------------------------------------------------
  FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
  --------------------------------------------------------------------------------
   65 | WARNING | Unused variable $weight.
  --------------------------------------------------------------------------------
  

• Codespell has found some spelling errors in your code.
  

  ./inc/webform2sftp.webform.inc:79: transfered  ==> transferred
  ./inc/webform2sftp.common.inc:218: sucessfully  ==> successfully
  

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:

1. webform2sftp.module: why do include the common inc file globally on every single page request? You should only include it in function bodies when you actually need it.
2. webform2sftp_form_alter(): if you are targeting only one form you should use hook_form_FORM_ID_alter() instead.
3. webform2sftp_global_settings_form_handler_files_submit(): why is this function located in the module file and not in the inc file? Please add a comment.
4. I could not find any test files in the repository, did you consider writing automated simpletests? See https://drupal.org/simpletest
5. webform2sftp_get_webforms(): Do not use db_select() for simple static queries, use db_query() instead. See http://drupal.org/node/310075. Also elsewhere.
6. webform2sftp_global_settings_form(): why is the sftp password form field not of type password? Please add a comment.
7. '#prefix' => t('<strong>Trigger File</strong>'),: HTML tags should be outside of t() where possible. Also elsewhere.
8. "watchdog('webform2sftp', 'The encrypted file ' . basename($file) . ' was successfully generated');": do not concatenate variables directly into the watchdog() message, use placeholders instead. Also elsewhere.
9. '#markup' => '<strong>Components Order</strong><br/>The order that components will be exported in file to sftp': all user facing text must run through t() for translation. Please check all your strings.
10. class/gnupg: what is this directory used for? Looks like random gpg config files that should not be in a webroot? Does that config directory also include GPG private keys? Then this would be a security issue: they should be considered private files and should be outside of the webroot, e.g. in the private files directory or somewhere else. I think you should clear that up in your configuration and README.txt.
11. __DIR__ does only exist in PHP 5.3 and higher, so either specify that as dependency in the info file or switch to dirname(__FILE__).
12. gnupgCreateNewInstance(): why do you use putenv() for the same GNUPGHOME variable twice?
13. sftpWriteFile(): why do you suppress PHP warnings with "@" here? Please add a comment. Also elsewhere. Usually you should not do that as it makes debugging problems harder.

So I think the gpg config files are a blocker right now. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Module looks great with the new fixes, moving to reviewed.

I've had a look through the code and it looks good. Would be nice to see this move through :)

+1 RTBC

Multiple Applications

It appears that there have been multiple project applications opened under your username:

Project 1: https://www.drupal.org/node/2390607

Project 2: https://www.drupal.org/node/2390569

Project 3: https://www.drupal.org/node/2190225

As successful completion of the project application process results in the applicant being granted the 'Create Full Projects' permission, there is no need to take multiple applications through the process. Once the first application has been successfully approved, then the applicant can promote other projects without review. Because of this, posting multiple applications is not necessary, and results in additional workload for reviewers ... which in turn results in longer wait times for everyone in the queue. With this in mind, your secondary applications have been marked as 'closed(duplicate)', with only one application left open (chosen at random).

If you prefer that we proceed through this review process with a different application than the one which was left open, then feel free to close the 'open' application as a duplicate, and re-open one of the project applications which had been closed.

I'm a robot and this is an automated message from Project Applications Scraper.

@jribeiro,
Thanks for your contribution.I am setting the issue status "needs review". you need to wait some one else to review the project application and make it RTBC.

RTBC +1

Assigning to myself for next review, which will hopefully be soon.

Automated Review

(+) Best practice issues identified by pareview.sh / drupalcs / coder. Yes, See attached (webform2sftp.txt).

Manual Review

(+) When I access webform/sftp-export url, it produces following warning that need to be fix.
Warning: Creating default object from empty value in _webform2sftp_get_components() (line 179 of C:\xampp\htdocs\drupal\sites\all\modules\webform2sftp\inc\webform2sftp.webform.inc).

(+) webform2sftp_configure_form(): When using explicitly set HTML id attribute prefix it with module name to avoid name collisions. Alternatively, HTML id attributes can be passed through drupal_html_id().

A hook_help() would be nice.

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.

But these are not blocking issues. I also tested module functionality w.r.t XSS, CSRF and Sql Injection and it worked as expected. Good Job!

Also, Blocking issues from #11 have been addressed and after reading through `git diff bc7a3f2..HEAD` didn't see anything major so...

Thanks for your contribution, Jean Ribeiro!

I updated your account so you can promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and stay involved!

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.

Thanks to the dedicated reviewer(s) as well.