I have created this module to allow users accessing webforms with multiple steps/pages to be able to skip to different steps/pages of the form, instead of the default functionality of Webform, which requires visiting each step/page of the form one at a time via "Previous" and "Next" buttons.

Project link
https://www.drupal.org/project/webform_skip/

Git instructions

git clone --branch 7.x-1.x  https://git.drupal.org/project/webform_skip.git

PAReview checklist*
https://pareview.sh/pareview/https-git.drupal.org-project-webform_skip.git

* There is one warning on PAReview, but the functionality is required to reset the hidden form element used by the module.

Thank you,
Joe

Comments

joecorall created an issue. See original summary.

joecorall’s picture

joecorall commented
Issue summary: View changes
vernit’s picture

vernit
he/him
Primary language English
Location India
commented

Still PAReview errors available. Please resolve those.

FILE: ...e1101/web/vendor/drupal/pareviewsh/pareview_temp/webform_skip.module
--------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
--------------------------------------------------------------------------
 130 | WARNING | Do not use the raw $form_state['input'], use
     |         | $form_state['values'] instead where possible
vernit’s picture

vernit
he/him
Primary language English
Location India
commented
Status: Needs review » Needs work
joecorall’s picture

joecorall commented
Status: Needs work » Needs review

Hello,

Thank you for reviewing! I mentioned in the initial post about the one warning PAReview displays, and that I believe it's a false positive. The $form_state['input'] is only used in order to to reset the value of a hidden form variable: https://git.drupalcode.org/project/webform_skip/blob/7.x-1.x/webform_ski...

I am not using the value stored in $form_state['input'] anywhere in the module's code, only resetting it to zero. Is this still a valid concern?

vernit’s picture

vernit
he/him
Primary language English
Location India
commented

Yes, As $form_state['input'] was not validated or sanitized.

joecorall’s picture

joecorall commented

I'm not sure I understand. I'm not using the value, it's just being set to its initial value of zero. Why does it need to be validated or sanitized?

oranges13’s picture

oranges13
she/her
Location Michigan
commented
Status: Needs review » Needs work

This sounds like a very useful module, though I have some concerns on it's effect on validation of webforms as subverting the linear progression from page to page may mean users skip required information. I'm going to more thoroughly test your module and get back to you on that point.

My other concerns:

  1. You should add an hook_uninstall() to remove the variables that you set using variable_set.
  2. You may want to reconsider automatically enabling this function on all webforms, and reverse that so it only affects those which have been explicitly enabled. As a site runner with many webforms, it would be extremely frustrating to have to manually go through each and disable this feature if it was not wanted across the board. It's much more user friendly to only have this function where it has been explicitly enabled.
joecorall’s picture

joecorall commented
Status: Needs work » Needs review

Thank you for your feedback! Please let me know if you find any bugs related to your validation concerns. I've been running this module for one of my site's particular use case, and it's been running successfully. Though for that particular webform, the only required field is on the first page of the webform, and the other steps of the webform are optional.

Re concern #1: I added hook_uninstall(). Thanks for pointing this out!

Re concern #2: I debated this on initial release. I ended up defaulting to "enabled" because I thought enabling the module should enable the functionality. Though I understand your point about the annoyance of having to go to multiple webforms in the event you want to enable the functionality for some multi-steps webforms, but not others. So I added a config form to easily enable/disable the module's functionality on all webforms in the system. On the config form, all the webforms are listed, and to disable you can easily uncheck box(es) and then save. The config form is at /admin/config/content/webform/skip. Tomorrow, I'll create a new release for this, and will update the project's main page and README.

joecorall’s picture

joecorall commented
Issue summary: View changes
vuil’s picture

vuil
Location Bulgaria 🇧🇬 🇪🇺 🌍
commented
Issue summary: View changes
Status: Needs review » Needs work

Thank you for the contribution!

Please fix all issues mentioned on Pareview checklist page as the following one:

FILE: ...e1101/web/vendor/drupal/pareviewsh/pareview_temp/webform_skip.module
--------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
--------------------------------------------------------------------------
 158 | WARNING | Do not use the raw $form_state['input'], use
     |         | $form_state['values'] instead where possible
--------------------------------------------------------------------------
joecorall’s picture

joecorall commented
Status: Needs work » Needs review

As mentioned in the initial post, and comment #5 and #7, I believe this is a false positive. Can you please confirm or deny?

joecorall’s picture

joecorall commented

Link to the code in question is here >>> https://git.drupalcode.org/project/webform_skip/blob/7.x-1.x/webform_ski... <<< thank you for reviewing!

vuil’s picture

vuil
Location Bulgaria 🇧🇬 🇪🇺 🌍
commented
Status: Needs review » Needs work

The issue #11 needs to be fixed.

And, please do not change the status if there is no update into the code.
Thank you.

joecorall’s picture

joecorall commented

Can you please tell me why it needs updated? PAReview says on that page the issue could be a false positive, and I believe it is.

joecorall’s picture

joecorall commented

From RandyFay.com

$form_state['input']: The array of values as they were submitted by the user. These are raw and unvalidated, so should not be used without a thorough understanding of security implications. In almost all cases, code should use the data in the 'values' array exclusively. The most common use of this key [input] is for multi-step forms that need to clear some of the user input when setting 'rebuild'

I think PAReview is just not taking this use case into account in its automated reviews of code (hence, the false positive warning it is throwing for this module). This module relies on reseting a hidden form value on a multi-step form, and the only way to do so is to access $form_state['input']. The module is not using the value in $form_state['input'], which is what would pose a security risk, and PAReview is probably simply looking for instances of that string in the code, not taking into account code that is simply setting a value, not using the value.

joecorall’s picture

joecorall commented
Status: Needs work » Needs review
avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented

Since the code is reading from $form_state['values']['skip'] and setting $form_state['input']['skip'], there aren't security issue. I would consider this a false positive from PAReview.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs review » Reviewed & tested by the community

Thanks for you contribution!

"'#value' => 'Save',": all user facing text must run through t() for translation.

Otherwise looks good to me, did not see security issues.

joecorall’s picture

joecorall commented

Excellent! I added t() to that Save button, and pushed to the dev release. Thanks for catching that.

Thanks to everyone for reviewing!

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Assigned: Unassigned » avpaderno
Status: Reviewed & tested by the community » Fixed

Thank you for your contribution! I am going to update your account.

These are some recommended readings to help with excellent maintainership:

You can find more contributors chatting on the IRC #drupal-contribute channel. So, come hang out and stay involved.
Thank you, 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.

I thank all the dedicated reviewers as well.

Status: Fixed » Closed (fixed)

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