A webform component that lets the user select a country out of a list that you can configure.

To configure, you choose a set of countries (out of the complete list of all countries) that can be selected. You can also change the order in which they appear in the select box.

Project page

webform_country_list

Sandbox

git clone --branch 7.x-1.x http://git.drupal.org/sandbox/matthias_mo/2294933.git webform_country_list
cd webform_country_list

PAReview

http://pareview.sh/pareview/httpgitdrupalorgsandboxmatthiasmo2294933git

Other projects

Since more than a year I'm the maintainer of the webform_confirm_email module.

Reviews of other project applications

mollie_payment
ckeditor_less
custom_stackdriver_metrics

Comments

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/httpgitdrupalorgsandboxmatthias_mo2294933git

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.

phaer’s picture

Hi,

Nice work! As far as I know, there's no other country list component for webform and it's a simple and readable module.

  • webform_options_countries() seems to be just an alias for two lines using drupals core country list (source. Cores country list is editable with https://www.drupal.org/project/countries, you could mention that in your README.txt
  • There are a few conflicts with Drupals coding guidelines (http://pareview.sh/pareview/httpgitdrupalorgsandboxmatthiasmo2294933git ). I'd say you can ignore the warning about function prefixes because prefixing them with an extra _ does no harm, but please fix the CodeSniffer issues.
  • You use some renderable arrays mixed with some hardcored HTML in theme_country_list_form(). Is there a reason for this? I think exclusive usage of renderable arrays would be the drupal-way.
  • You could simply use if (empty($available_countries)) { instead of if (count($available_countries) == 0) {
  • You could simplify isset($component['weight']) == TRUE ? $component['weight'] : 0, to isset($component['weight']) ? $component['weight'] : 0,, == TRUE is redunant here.
  • If you add a & to line 36 of the .module file $avail_countries = $component['extra']['available_countries'];, you should be able to reuse it in line 49 ( '#default_value' => !empty($component['extra']... which is a bit too long at the moment. Please keep lines below 80 characters if possible.
  • I guess its a matter of personal preference, but I'd think that _webform_edit_country_list() would be even more readable if you'd alias $form['country_list'][$delta] to something shorter like $country_form
  • Does it support webform-3.x and/or webform-4.x? You could clarify that in README.txt
matthias_mo’s picture

Hi!

Thank you @phaer for reviewing. I incorporated most of your suggestions.

Regarding your 3rd point - "You use some renderable arrays mixed with some hardcored HTML":

  • I copied (and changed) the code from Drupal core field module
  • According to theme table documentation one is supposed to provide HTML for the table headers

Thank you for taking the time!

matthias_mo’s picture

Status: Needs work » Needs review
matthias_mo’s picture

Just for the record, all issues from pareview have been addressed except those that I think should be left as is:

  • the _webform_* functions use the webform API name schema and have to be named like this
  • I think the table header array structure is indented correctly
  • I left the </lable> tag intentionally on a separate line for better readability
matthias_mo’s picture

Issue summary: View changes
matthias_mo’s picture

Issue summary: View changes
matthias_mo’s picture

Issue summary: View changes
matthias_mo’s picture

Issue summary: View changes
matthias_mo’s picture

Issue summary: View changes
matthias_mo’s picture

Issue tags: +PAReview: review bonus
er.pushpinderrana’s picture

@matthias_mo, first of all thanks for your contribution!

Nice Module!

Manual Reviews:
Few things I noticed in your module.
1. You are adding webform_country_list.css using .info file, it would be better to add css using #attached.
2. You are adding '@' with following function to avoid warnings, it would be better add a single line comment above to this code for better understanding.

$geoip_ctry_code = @geoip_country_code_by_name(ip_address()); 

3. Add hook_help() in your module and also describe about geoip_country_code_by_name function there for better understanding.

Rest looks good to me! I am not moving this to "Need Works" as there is not any blocker.

Thanks Again!

matthias_mo’s picture

Thank you er.pushpinderrana,

I think I fixed all issues that you reported.

Thank you for taking the time to look at my code.

best

matthias_mo’s picture

Issue summary: View changes
vanyamtv’s picture

Status: Needs review » Needs work
FileSize
594.41 KB

1. "Country list" field description is not shown at all.
2. While editing the "Country list" field, after it was already saved, you'll get an error messages like "Weight for row n must be an integer." (see the screenshot)
3. Would be great to add possibility to don't select any of the countries. For example to have functionality to add a label, like "- Please select a country -", for example.
4. Is the country names translatable?

Thanks:)

matthias_mo’s picture

Thanx for testing out my module @vanyamtv!

@ 1.) is now fixed in the git repo
@ 2.) I cannot reproduce it, I'd need more detail instructions how you got there
@ 3.) hmm. I have to think about that, not shure whether I want that behavior. But thanx for pointing out.
@ 4.) yes they are. They're defined in core, includes/iso.inc using the t() function

Thank you for your review!

matthias_mo’s picture

Status: Needs work » Needs review
vanyamtv’s picture

Status: Needs review » Needs work

2) Well: just add new filed of type "Country list", don't rearrange the order of the countries, then click save and you'll get 250 error messages.

Environment: PHP Version 5.3.28, Drupal 7.30, Webform 7.x-3.20

matthias_mo’s picture

Status: Needs work » Needs review

@vanyamtv Tried it again, and no, no errors

matthias_mo’s picture

@vanyamtv I just mad a complete new minimal drupal 7.30 installation and tested it in every way I could think of. I never got error messages.

Please provide detailed step by step instructions how to reproduce this problem.

vanyamtv’s picture

Status: Needs review » Needs work
matthias_mo’s picture

Status: Needs work » Needs review

@vanyamtv thank you for the screen cast, but I believed you also before ;-)

As I told you already I cannot reproduce your problem. I tried it on another machine, followed exactly your steps and still get no errors.

I also searched the error strings and I can't find them neither in drupal core nor in the webform module. For me the look like some validation function output, so do have some special client side form validation running on your installation?

So can I ask you to make a new, clean installation of Drupal and try it there?

And if it still persists, can you debug it some more to give me some pointers where to look for?

And do you have the latest version of the module? I did some updates during the last 2 weeks ...

I also suggest to leave this application at "Needs review" as sofar none else than you has this problem and I'd like to have a report from someone else on whether he/she can reproduce the problem you reported.

vanyamtv’s picture

Status: Needs review » Needs work

All right:

1) The error message comes from:
File /sites/all/modules/webform/components/number.inc
Line number: 594
form_error($element, t('%name field value of @value must be an integer.', array('%name' => $element['#title'], '@value' => $value)));

2) In order to fix the bug:
File: /sites/all/modules/webform_country_list/webform_country_list.module
Line number: 76
'#default_value' => isset($country['_weight']) ? $country['_weight'] : $delta,

Should be changed to:

'#default_value' => (is_array($country) && isset($country['_weight'])) ? $country['_weight'] : $delta,

Because:
if $country is string isset($country['_weight']) returns true in my case. Could be explained like (int)'_weight' = 0. So default value in that case will be the first letter of the country name.

matthias_mo’s picture

@vanyamtv

You were right, the code was wrong. I copied it from field.form.inc and didn't rework it completely. The whole conditional is useless.

I'm still surprised that I didn't see the warnings you got. I checked whether they were turned of in /admin/config/development/logging but no, all error messages and warnings are switched on. So the only thing the remains is the PHP version, I'm using 5.5.12

Thanx for finding this bug and being persistend ;-)

vanyamtv’s picture

You're welcome:)

matthias_mo’s picture

Status: Needs work » Needs review
klausi’s picture

Status: Needs review » Needs work
Issue tags: -PAReview: review bonus +PAReview: security

Review of the 7.x-1.x branch (commit 7bcadec):

  • No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

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. _webform_render_country_list(): this is vulnerable to XSS exploits. If I enter <script>alert('XSS');</script> as description for the webform field there will be a nasty javascript popup when the webform is displayed. You need to sanitize #description before printing, make sure to read https://www.drupal.org/node/28984 again. And please don't remove the security tag, we keep that for statistics and to show examples of security problems.
  2. webform_country_list_help(): all user facing text must run through t() for translation.

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

matthias_mo’s picture

Status: Needs work » Needs review

thank you @klausi for the review and the insight.

I fixed the issues you reported.

matthias_mo’s picture

Priority: Normal » Major

This project application has now seen 4 reviews, all required changes have been made.

According to How to review Full Project applications I changed the priority to "Major".

It seems that this project is ready to become a full project, hence I kindly ask to give it a go.

matthias_mo’s picture

Priority: Major » Critical

It's now been 4 weeks since the last review, I kindly ask again to approve my project application.

er.pushpinderrana’s picture

@matthias_mo, Getting another review bonus will help speed up the process and make sure it gets on the review admins radar.

naveenvalecha’s picture

Hi @matthias_mo,
thanks for your contribution!
Nice Module!

Automated Review

Best practice issues identified by pareview.sh / drupalcs / coder.Some minor nutpicks please fix but regarding the functions naming conventions is fine because functions provided by the webform module. http://pareview.sh/pareview/httpgitdrupalorgsandboxmatthiasmo2294933git

Manual Review

Individual user account
Yes: Follows the guidelines for individual user accounts.
No duplication
Yes: Does not cause module duplication and fragmentation.
Master Branch
Yes: Follows the guidelines for master branch.
Licensing
Yes: Follows the licensing requirements
3rd party code
Yes: Follows the guidelines for 3rd party code.
README.txt/README.md
Yes: Follows the guidelines for in-project documentation and the README Template.
Code long/complex enough for review
Yes: Follows the guidelines for project length and complexity.
Secure code
Yes. If "no", list security issues identified.
Coding style & Drupal API usage
  1. webform_country_list.info : Add the package name webform in the file because this module provides the functionality related to the webform.

This review uses the Project Application Review Template.

As I am not a git admin, please help to review other project applications to get another review bonus. This will put you on the high priority list, then git administrators will take a look at your project right away :-)

I have not seen any release blocker. So +1 for RTBC.

Thanks Again!

robin.ingelbrecht’s picture

Status: Needs review » Needs work
FileSize
22.35 KB

Hi matthias_mo,

I Installed the module and configured a webform. When going to the webform I got following notices:
Notices

Also, wouldn't it be nice to have a button check/uncheck all?

naveenvalecha’s picture

@robin.ingelbrecht,
Thanks for checking.I did not see this error while review.Also I cannot reproduce this now.Please offer more details.
Thanks Again!

Also, wouldn't it be nice to have a button check/uncheck all?

As this is not the release blocker so I have added a feature request in the module issues https://www.drupal.org/node/2346821

matthias_mo’s picture

Priority: Critical » Major
Status: Needs work » Needs review

yes that was a bug @robin.ingelbrecht, thanks for reporting

davidam’s picture

Status: Needs review » Needs work

In this moment I can see this problems in the pareview:

./webform_country_list.module: all functions should be prefixed with your module/theme name to avoid name clashes. See https://www.drupal.org/node/318#naming

function _webform_defaults_country_list() {
function _webform_edit_country_list($component) {
function _webform_render_country_list($component, $value = NULL, $filter = TRUE) {
klausi’s picture

Status: Needs work » Needs review

Those are webform coding conventions and not application blockers, please do a real manual review of the source code.

sumitmadan’s picture

FileSize
22.04 KB
10.79 KB

Hi @matthias_mo,

I have added the "country list" component and its working fine on webform page. But why no result/mail is for country selected?

See attached screenshot.

matthias_mo’s picture

@sumitmadan : because the hooks _webform_table_country_list and _webform_display_country_list have not been implemented yet, please add a feature request. I'm still waiting for my project application to be accepted, after that I'll continue to add features (like this).

sumitmadan’s picture

@matthias_mo result is an important thing for webform component, otherwise no use of this module. I think you have to add this feature and make it useful for others.

matthias_mo’s picture

@sumitmadan I added the feature request.

druliner’s picture

Hey @matthias_mo:

This sounds like a great module. I installed it and created a couple of webforms and it works as advertised. However I would have to agree with @sumitmadan. This module is probably of little use to anyone if the component's data is not saved.

matthias_mo’s picture

@druliner the setup of the webform component (that is - which countries are available for selecting) as well as the webform submission data (which country the user selected) *IS* saved in the database. The only thing that's missing is the implementation of the hook that displays the user submitted data for this component in the webform results. So I do think the modul is useful already.

As I've already noted I'm happy to implement these hooks for the webform results, actually I have several other ideas of how to make this module more pleasant to use. But I already put more time into this "full project rights application" as is justified for such a simple and small module so for now I put everything on hold and wait.

Watergate’s picture

Assigned: Unassigned » Watergate
Priority: Major » Normal
Status: Needs review » Active
Watergate’s picture

Assigned: Watergate » Unassigned
Status: Active » Needs work

Hi matthias_mo,

I've read the previous comments and see that you are eager to start deploying your project. However, I do agree with sumitmadan and druliner that submission results should be easily visible for site administrators (i.e., project application reviewers). I don't consider this as functionality that could/should be added later, because it involves 'security-vulnerable' code (i.e., outputting user entered content). If you ask me, having extra pair of eyes looking at this specific code should be considered as more than welcome, and that is what reviewers do ;) Since I like to help you move on with the project, I have created a patch that implements this functionality, see #2360777: Basic output of webform results

To speed up the review proces, I encourage you to get another review bonus. This will get your project request on the high priority list, and it will stay there!

Below you can find (the rest of) my review using the Project Application Review Template.

Let me know if you need some clarification on my feedback.

Manual Review

Individual user account
Yes: Follows the guidelines for individual user accounts.
No duplication
Yes: Does not cause module duplication and/or fragmentation. However, I encourage you to write a few paragraphs (on the project page) explaining the difference(s) of your module compared to Countries and the list of countries that is available within Webform itself. I think this would help convincing site administrators to use your module.
Master Branch
Yes: Follows the guidelines for master branch.
Licensing
Yes: Follows the licensing requirements.
3rd party assets/code
Yes: Follows the guidelines for 3rd party assets/code.
README.txt/README.md
No: Does not follow the guidelines for in-project documentation and/or the README Template completely.
  1. I encourage you to use the prefered format that is explained on the bottom of the README Template (i.e., Headings in all caps, Two lines prior to headings (except the first one), ...).
  2. I've tested your module with Webform 7.x-4.1 and it works fine. I would therefore change the compatibility text in the readme file, that your module is tested with 7.x-3.x (if you have done yourself) and 7.x-4.x.
  3. Although trivial, but you could mention that when no country is selected all will be available.
  4. I encourage you to add a section with required modules, and also list the dependencies of Webform: ctools and Views.
  5. In the sections with installation instructions, I recommend you to add a link to https://drupal.org/documentation/install/modules-themes/modules-7
  6. I recommend to add the (part of the) contents of the readme file (i.e., requirements, installation instructions) and a section of similar modules (see notes on duplication) on your project page.
  7. Since I'm unfamiliar with the PHP GeoIP package, are there any alternatives (I read something on GeoIP2, GeoLite Legacy)? I think instruction on how to install the package wouldn't be superfluous. Maybe you could also provide a link to an overview page or provide a (small) description explaining the functionality (compared to other packages?) yourself?
Code long/complex enough for review
Yes: Follows the guidelines for project length and complexity.
Secure code
Yes: Meets the security requirements.
Coding style & Drupal API usage
  1. In the .info file you could add package = Webform, so that the module is listed along with the other Webform modules (when installing).
  2. (+) Marking the component required, has no effect on the rendered form. In webform_country_list_webform_component_info() you specify that the component can be required (i.e., that a visitor has to select a value). So I think you either should remove the ability that your component can be marked required or implement (correct) #empty_option behavior (maybe with the $geoip_ctry_code as the first option to select).
  3. (+) Changing the 'Label display' property on the component edit page has no effect. Adding '#title_display' => $component['extra']['title_display'] ? $component['extra']['title_display'] : 'before', in the array of the form element in _webform_render_country_list() solves this problem.
  4. (+) Value(s) entered in the 'Wrapper CSS classes' aren't outputted to HTML. Adding '#theme_wrappers' => array('webform_element'), into the array of the form element in _webform_render_country_list() solves this problem.
  5. It would have been useful for me if you had provided inline comments, especially in the section where you generate the list of all/available countries (_webform_edit_country_list()), or apply usort() in _webform_country_list_edit_validate().
  6. Please apply the API documentation and comment standards, for instance _webform_country_list_cmp_weights(), theme_country_list_form() (use @param and @return tag).
  7. (*) Webform's hook _webform_theme_component() should be used instead of hook_theme(), see _webform_theme_component documentation. The patch in #2360777: Basic output of webform results already fixes this!

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.

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This review uses the Project Application Review Template.

matthias_mo’s picture

Status: Needs work » Needs review

Hi Niels,

first of all, thank you for the extended amount of effort you put in the review, it was very much appreciated! Thank you also for the insight! For shure I commited your patch.

To speed up the review proces, I encourage you to get another review bonus. This will get your project request on the high priority list, and it will stay there!

I know, I was reminded many times. But my previous experience with it was so unpleasand that I lost interest in the "review bonus".

Does not cause module duplication and/or fragmentation. However, I encourage you to write a few paragraphs (on the project page) explaining the difference(s) of your module compared to Countries and the list of countries that is available within Webform itself.

I added documentation in the README and the project site.

Does not follow the guidelines for in-project documentation and/or the README Template completely ....

I went through your list and applied the changes as you suggested.

Since I'm unfamiliar with the PHP GeoIP package, are there any alternatives (I read something on GeoIP2, GeoLite Legacy)? I think instruction on how to install the package wouldn't be superfluous. Maybe you could also provide a link to an overview page or provide a (small) description explaining the functionality (compared to other packages?) yourself?

I use the GeoIP package for the simple reason that it comes packaged for several Linux distributions and hence is widely used (and hence tested).

In the .info file you could add package = Webform, so that the module is listed along with the other Webform modules (when installing).

Done.

Marking the component required, has no effect on the rendered form.

I don't need the required feature, I removed it.

Changing the 'Label display' property on the component edit page has no effect. Adding '#title_display' => $component['extra']['title_display'] ? $component['extra']['title_display'] : 'before', in the array of the form element in _webform_render_country_list() solves this problem.

Done.

Value(s) entered in the 'Wrapper CSS classes' aren't outputted to HTML. Adding '#theme_wrappers' => array('webform_element'), into the array of the form element in _webform_render_country_list() solves this problem.

Done.

It would have been useful for me if you had provided inline comments, especially in the section where you generate the list of all/available countries (_webform_edit_country_list()), or apply usort() in _webform_country_list_edit_validate(). Please apply the API documentation and comment standards, for instance _webform_country_list_cmp_weights(), theme_country_list_form() (use @param and @return tag).

I went through the code and enhanced the documentation.

Webform's hook _webform_theme_component() should be used instead of hook_theme(), see _webform_theme_component documentation. The patch in #2360777: Basic output of webform results already fixes this!

I applied your patch, thanx again for the work!

Once again, thank you for this helpful review.

matthias_mo’s picture

Priority: Normal » Major
naveenvalecha’s picture

Awesome module!.Thanks for ur contributions.
+1 for RTBC.
Few more minors

  1. file : webform_country_list.component.inc : function _webform_edit_country_list : Use module_load_include to load specific inc file instead of include_once DRUPAL_ROOT . '/includes/locale.inc';
  2. file : webform_country_list.component.inc : function _webform_render_country_list : Use module_load_include to load specific inc file instead of include_once DRUPAL_ROOT . '/includes/locale.inc';
  3. if this function _webform_country_list_edit_validate its not seems to be a hook,if not then remove its _ prefix from function name..
klausi’s picture

@naveenvalecha: module_load_include() should only be used for modules, but that file is not part of a module, so this looks correct as is.

Also: did you forget to set this to RTBC?

naveenvalecha’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
229.21 KB

@kalusi,
Have not seen any release blocker to the module.Rest seems good to me.Setting this RTBC :)
Some Codesniffer issues :

  1. webform_country_list.module:52:9: error - Array indentation error, expected 6 spaces but found 8
  2. webform_country_list.module:53:9: error - Array indentation error, expected 6 spaces but found 8
  3. webform_country_list.module:53:20: error - String concat is not required here; use a single string instead
  4. webform_country_list.module:54:9: error - Array indentation error, expected 6 spaces but found 8
  5. webform_country_list.module:55:9: error - Array indentation error, expected 6 spaces but found 8
  6. webform_country_list.module:55:20: error - String concat is not required here; use a single string instead
  7. webform_country_list.module:56:9: error - Array indentation error, expected 6 spaces but found 8
  8. webform_country_list.module:57:9: error - Array indentation error, expected 6 spaces but found 8

Suggestions

  1. Display the country Name instead of country code on the result submission view page.See the attached screenshot below
  2. I suggest to use the tableselect to select all countries in single shot and if needed remove selected one.Its upto you its just a feature request that I think it should be in the module.
  3. function webform_country_list_help($path, array $arg) { : No need to specify the array before $arg. you can remove it.Seems similarly in theme_country_list_form, theme_webform_display_country_list functions because you are passing the arrays and I don't think you need to change the type of $variables to array.
naveenvalecha’s picture

Webform component edit form.The checkboxes should be aligned with the table header.I would suggest to use the different table columns in theme_country_list_form and remove the column borders using css.That seems to be a better approach.
See the problem in the attached screenshot.
This is not a release blocker.So the status will remain same.So you do the changes accordingly to make it looks good.

matthias_mo’s picture

@naveenvalecha thank you for reviewing my module!

Some Codesniffer issues

As mentioned in comment #5 I respectfully disagree with pareview.sh about formating these lines.

Display the country Name instead of country code on the result submission view page.See the attached screenshot below

Sounds reasonable, I put it on my TODO list.

I suggest to use the tableselect to select all countries in single shot and if needed remove selected one.Its upto you its just a feature request that I think it should be in the module.

Yes, there is the feature request for this. I'll implement that.

function webform_country_list_help($path, array $arg) { : No need to specify the array before $arg. you can remove it.Seems similarly in theme_country_list_form, theme_webform_display_country_list functions because you are passing the arrays and I don't think you need to change the type of $variables to array.

This was suggested by pareview.sh. It seemed consistent with the other parameter type declarations to me.

Thanx again for your review!

naveenvalecha’s picture

These are not release blockers.

As mentioned in comment #5 I respectfully disagree with pareview.sh about formating these lines.

I think if you will address the #51 This will also fix these errors as well.

matthias_mo’s picture

Priority: Major » Critical
klausi’s picture

Status: Reviewed & tested by the community » Fixed

Sorry for the delay. Reviewing more project applications and a new review bonus will get applications finished faster.

manual review:

  • _webform_render_country_list(): why do you suppress PHP errors with the "@" operator here? This will make debugging harder, please add a comment.

But otherwise looks good to me.

Thanks for your contribution, matthias_mo!

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.

matthias_mo’s picture

Hi @klausi,

thank you for promoting my account.

There is no reason why PHP errors from geoip should be suppressed so I changed it.

Status: Fixed » Closed (fixed)

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