Sandbox Page:
https://drupal.org/sandbox/jribeiro/2174305

Git Repository:
git clone --branch 7.x-1.x http://git.drupal.org/sandbox/jribeiro/2174305.git webform2sftp

Reviews of other projects
https://drupal.org/comment/8455627#comment-8455627
https://drupal.org/comment/8455427#comment-8455427
https://drupal.org/comment/8455197#comment-8455197
https://www.drupal.org/node/2390489#comment-9419867
https://www.drupal.org/node/2386767#comment-9419913
https://www.drupal.org/node/2373075#comment-9419951

Overview

This module provides the functionality to export webform submissions to a secure SFTP server.

Features

  • Security: The module focuses on the integrity of the data to be exported, sending the data via secure connection (sFTP) and be possible to encrypt (PGP) the data file before sending.
  • Multiple Integration: The module serves multiple webforms by separating the settings for each one.
  • File data: Is basically a file containing a submission in each line, separating the field values ​​for a character of your choice, usually pipe "|".
  • Schedule and automation: You can automate and schedule data exports, the Job will be executed by system cron in the time you set.
  • Notification: You can configure a email to be sent when the Job executes successfully.
  • Encryption: Is possible to use PGP encryption in the data file before it is sent to a sFTP.

Requirements

  • This module depends of webform module.
  • IMPORTANT: The module uses the PHP extension SSH2 to connect to SFTP server and PHP GnuPG extension for file PGP encryption, so installing these extensions are REQUIRED to module works, otherwise errors will occur.
CommentFileSizeAuthor
#22 webform2sftp.txt6.86 KBer.pushpinderrana
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jribeiro’s picture

Issue summary: View changes
jribeiro’s picture

Issue summary: View changes
Issue tags: +PAreview: review bonus

Issue Tags

PA robot’s picture

Status: Needs review » Needs work

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.

jribeiro’s picture

The errors and warnings are now fixed.

jribeiro’s picture

Status: Needs work » Needs review

The errors and warnings are now fixed.

caiovlp’s picture

Status: Needs review » Needs work

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.

jribeiro’s picture

Status: Needs work » Needs review

Thanks for your review @caiovlp,

- Related to security issues, hook_requirements is implemented, to check and warning about the required PHP extensions.
- Related to t() for exception messages, I fixed the call to the watchdog, so that it is translatable now.

Thank you again.

pijus’s picture

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

jribeiro’s picture

Thanks for your review @pijus,

I fixed the comments in .install file hooks.

About the Line 83, I'm actually using the operator "=", nothing to be changed in this case.

caiovlp’s picture

Status: Needs review » Reviewed & tested by the community

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.

klausi’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: -PAreview: review bonus

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.

jribeiro’s picture

Status: Needs work » Needs review

@klausi,
Thanks for your detailed review, helped to improve the quality of the module and my learning.

Follow the responses:
1 - I modified to file be inserted only when necessary.
2 - Fixed. Implemented hook_form_FORM_ID_alter().
3 - The problem is that when I add this function in admin.inc, an error is displayed, that the function can not be located. For this reason, I kept on .module file.
4 - I not had enough time to Implement test cases, maybe in the next release.
5 - Changed to db_query() when was possible.
6 - Fixed. Changed the field type to password.
7 - Fixed.
8 - Fixed all watchdog() functions.
9 - Fixed: all strings checked.
10 - Actually this folder was not necessary. Removed from the project, function also reviewed.
11 - This line was removed.
12 - Fixed to set GNUPGHOME to temporary path.
13 - All supressed warnings were removed.

gabrielmachadosantos’s picture

Status: Needs review » Reviewed & tested by the community

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

dixon_’s picture

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

+1 RTBC

jribeiro’s picture

Issue summary: View changes
jribeiro’s picture

PA robot’s picture

Status: Reviewed & tested by the community » Closed (duplicate)
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’s picture

Status: Closed (duplicate) » Reviewed & tested by the community
naveenvalecha’s picture

Status: Reviewed & tested by the community » Needs review

@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.

natanmoraes’s picture

Status: Needs review » Reviewed & tested by the community

RTBC +1

er.pushpinderrana’s picture

Assigned: Unassigned » er.pushpinderrana

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

er.pushpinderrana’s picture

Assigned: er.pushpinderrana » Unassigned
Status: Reviewed & tested by the community » Fixed
FileSize
6.86 KB

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:

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.

jribeiro’s picture

@er.pushpinderrana , Thanks for your support!!

I fixed the errors and code standards issues and promoted the module :)

Thanks again! Keep pushing!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.