[D7] webform_country_list

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.

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, 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!

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:)

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

https://www.youtube.com/watch?v=uwr4aBnw-84

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.

You're welcome:)

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, Getting another review bonus will help speed up the process and make sure it gets on the review admins radar.

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!

Hi matthias_mo,

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

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

@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

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) {

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

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

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.

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.

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

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

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

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.

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.

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:

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