[D7] Webform Field Repeat

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

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

After installation on clean drupal 7.22, I tried to create new webform and got this error:
Notice: Undefined property: stdClass::$nid in webform_field_repeat_form_alter() (line 70 of /var/www/kristaps/data/www/*****/sites/all/modules/webform_field_repeat/webform_field_repeat.module).

Hello.

I installed the module and it worked as expected.

After reading you code I have some comments :

.info: l.2 : "subimisson" should be "submission"

.module : l.31 You don't need to sanitize contents of node_load()
node_load(check_plain($nid));
l.40 > <a href="' . check_url($GLOBALS['base_url'] . '/node/' . $nid . '?sid=' . $sid) . '">' . t('Go back to the form') . '</a>
All internal links output by modules should be generated by l() function.

I didn't check webform_field_repeat.js.

Hallo qudsiya,

I install the module and I did not find any major Issue. I have only one suggestion. You use drupal_uninstall_schema('webform_field_repeat'); in webform_field_repeat_uninstall(). This is not necessary in Drupal 7.

Hi qudsiya,

I installed the module and tested it out, and it seems to work as expected.
Looking over the code everything looks good - no major issues I can see. In addition code review found no issues. Perhaps something that could be added is a header comment to the webform_field_repeat.js file.

Perfect - looks good now.

Multiple Applications

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

Project 1: https://drupal.org/node/1832562

Project 2: https://drupal.org/node/2024431

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.

Bumping for review again. One of the three applications should be approved.

I'll look at this now in the Project applications sprint

I was a little unclear from reading the project page about whether this provides "multiple values" in webform fields (which I believe is currently lacking, and would be a good feature) - or .. something else that gets triggered when re-editing a form, as the project page seems to imply.
Hopefully that will become clear, but if there is an answer, I'd like to see that on the project page.

• Installed on a clean, current D7 with 'minimal' profile
• enabled module, which pulled in webform dependency.
• Made a webform with some sample fields, including text, fieldset and selects
• Edited the settings and used the 'repeat fields' option. It's not becoming clearer that this means the browser will auto-fill the webform data if I make multiple submissions? Am I getting warmer?
• To test, I use an anonymous browser session and enter some data.
• On submission, I get "go back to this form" as described on the project page.
• Using that, I find that my text field is populated, but the select field I used (and specified in the 'repeat field') is not.
• looking at results, I see that "go back to form" is not actually revisiting the previous submission, but making a new submission (albeit with some cloned data from befpre.) I can (now) understand if this is by design, but please describe the use-case or an example of where this would be useful on the project page.

Functionally, I think that support for webform select fields (at least) is currently missing. This may be because it's a select, or because I chose to nest it in a fieldset.
Are all core webform fields expected to work? If not, please note that as a limitation.

I could not see how to change the text "go back to the form". I'd expect to be able to change it to "Please make another submission" or something through the UI provided. Not least because "Go back" implies that you are going back to what you just did, not starting a new submission, which is actually what happens.

---------------

CODE review.

1. Thanks for the README.
2. I'm a bit worried about the 'anonymous' user data being shared! Thanks for warning about it but .. !
   Seeing as you describe the data as only lasting an hour anyway... wouldn't this function work better (for anon if nothing else) by using $_SESSION ?
   I'm glad you identify the issue. really ... but can't you find a way to eliminate it? EDIT: on code review I see you also filter by remote_addr. So I guess that's not so bad. Perhaps the README could explain that.
3. Thanks for allowing a proper schema uninstall, though your textual descriptions are not very descriptive. boilerplate text doesn't help anyone a lot.
   

     $schema['webform_field_repeat'] = array(
       'description' => 'The table for repeated webform fields.',
   

4. I don't believe the DB index on cid is used anywhere in your code. Is there a reason it's there?
5. I'm not sure about webform_field_repeat.js ? I can see the UI help it gives ... but if the changes you make to the browser form client-side are important, you probably also want to #validate the form on submission too. Maybe.
6. AND - do not use raw
   

   jQuery(document).ready(function(){
   

   You must use Drupal.behaviors to add event handlers otherwise your code will clash with overlay, AJAX and other js widgets on the page in weird and wonderful and frustrating ways.
   You must also use the funky

   (function ($) {
     // All your code here
   })(jQuery);
   

   for jquery namespacing. See the examples in core /misc folder or any other Drupal module.

7. $url = $GLOBALS['base_url'] . '/node/' . $nid;
   

   Please use url() for this. $GLOBALS is always a warning sign.
   .. for that matter, it seems you use l() later anyway, so that is supposed to take care of that for you.

8.   $content['raw_markup'] = array(
       '#type' => 'markup',
       '#markup' => '<p>' . $confirmation . '</p><p>' . l($text, $url, $options) . '</p>',
     );
   

   looks a bit awkward (it's unthemable) ... but I'll let it slide.

9. I'm happy to see you caught hook_webform_component_delete() etc. Cleanup is good.

10. /**
     * Implements hook_form_alter().
     */
    function webform_field_repeat_form_alter(&$form, &$form_state, $form_id) {
    

    Needs better description. What does it actually do?
    _alter hooks are one of the hardest things to trace when debugging Drupal. PLEASE tell us what you are actually doing to our forms! The code inside there does not clarify what it's doing.

11. Using direct sql lookups to retrieve a submission form something as well-established as webform seems suspicious. Are you sure there is no API call that webform gives you for this? You really should use that.

            $access = db_query("SELECT nid, uid, submitted, remote_addr FROM {webform_submissions} WHERE sid = :sid", array(
              ':sid' => $_GET['sid'],
            ));
    ...
                  $data = db_query("SELECT * FROM {webform_submitted_data} WHERE sid = :sid", array(
                    ':sid' => $_GET['sid'],
                  ));
    
    

12. /**
     * Implements hook_form_FORM_ID_alter().
     */
    function webform_field_repeat_form_webform_configure_form_alter(&$form, &$form_state, $form_id) {
      drupal_add_js('/' . drupal_get_path('module', 'webform_field_repeat') . '/webform_field_repeat.js', 'external');
    

    You should normally instead use FAPI #attached
    You will find that your js does not load if you submit this form and validation fails - you js will not turn up on the second page. This is almost trivial in this case, but useful to know when building forms.

13. Several places could do with better inline documentation. webform_field_repeat_form_submit() for example is just not very clear about what's really happening. And, due to its sql stuff it does, troublesome.

With all this, (though the only real blocker is the drupal-incompatible jquery) I find it hard to agree with the RTBC tag ... I'm opening it up for a third opinion.

I'll leave this open - anyone can promote. I don't see blockers, but I'm not confident about the non-use of webform APIs either.

You have a couple of errors regarding unused variables here: http://pareview.sh/pareview/httpgitdrupalorgsandboxqudsiaaziz2023513git. You should also remove the .gitignore file. We don't use logical "and", use && instead.

In webform_field_repeat_form_submit() don't delete and then insert rows, you can use db_merge() instead. Because of if ($field['#type'] == "checkbox" and $field['#checked'] == TRUE) { ... } does that mean this module only works for checkboxes?

You could use a more detailed project page - when would I want to use this module? Can you attach screenshots?

I think you need a security warning about IP addresses on the project page as well - it's possible that 2 different users share the same IP address, and they might see each others' results.

Edit: I just read dman's review, many excellent points there as well.

----
Top Shelf Modules - Crafted, Curated, Contributed.

Project page

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

You can remove .gitignore.
In webform_field_repeat_form_alter(), the sql query does not need to select nid.

Looks great! I'll move it along to RTBC, but the project page really needs some help.

----
Top Shelf Modules - Crafted, Curated, Contributed.

It's been a month without any problems reported, so I'm promoting this myself as per https://drupal.org/node/1125818.

Thanks for your contribution, qudsiya!

I updated your account to let you 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 get 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.

----
Top Shelf Modules - Crafted, Curated, Contributed.