[D7] IP Locator with Splash

Fixed the git clone URL in the issue summary for non-maintainer users.

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.

I did a manual review of this project, and ran it through the "coder" module - here are my comments:

Individual user account
Yes: Follows the guidelines for individual user accounts.

No duplication
No: Causes module duplication and/or fragmentation.

(*) There is an existing module (Smart IP - https://www.drupal.org/project/smart_ip) which provides country lookup and geolocation based on the user’s IP address. There are many modules that use it for geolocation functionality. Module duplication and fragmentation is a huge problem on drupal.org and we prefer collaboration over competition. Consider depending on that module to provide geolocation functionality that’s implemented in this module

Master Branch
Yes: Follows the guidelines for master branch.

Licensing
Yes: Follows the licensing requirements.

3rd party assets/code
(-) I’m not sure why the third-party code needs to be added as a library, and can’t be included in the module. The license is compatible with being included as part of the Drupla module

README.txt/README.md
No: Does not follow the guidelines for in-project documentation and/or theREADME Template.

(-) There is no need to add hyphens and break up words to make each line exactly 80 characters. It’s OK to have lines in the README that are a bit shorter than 80 characters if it means that words are not broken up, and it is easier to read.

(+) Clearly identify the dependency on the “libraries” module and the steps to install the library, including renaming the library folder

(-) ip_locator_with_splash.module - Line 373: Comment doesn’t match the function

Code long/complex enough for review
Yes: Follows the guidelines for project length and complexity.

Secure code
Some issues identified by the “coder” module:

ip_locator_with_splash.install
(+) Line 73: Potential problem: drupal_set_message() only accepts filtered text, be sure to use check_plain(), filter_xss() or similar to ensure your $variable is fully sanitized.
drupal_set_message($msg, 'warning');

ip_locator_with_splash.module
(+) Line 295: Potential problem: FAPI elements '#title' and '#description' only accept filtered text, be sure to use check_plain(), filter_xss() or similar to ensure your $variable is fully sanitized.
'#title' => $val->country_label,

Coding style & Drupal API usage

General Comments
(-) The main configuration page (admin/config) should provide a single link to the configuration page for your module, not links to each of the individual configuration settings available through the module. The four options (“Add new website URL”, “Flush All”, “IP Locator Configuration”, and “Splash Message Configuration”) should be accessed through a single “IP Locator Configuration” page.
(+) When viewing one of the individual configuration pages, clicking on “IP Locator Configuration” in the breadcrumb generates an error message: Fatal error: require_once(): Failed opening required ‘/PATH_TO_SITE/sites/all/modules/ip_locator_with_splash/system.admin.inc' (include_path='.:') in /PATH_TO_SITE/cayl_drupal/includes/menu.inc on line 517

ip_locator_with_splash.module
(*) Lines 7-10: There should be no need to separately bootstrap Drupal from within this module
(-) Line 81: Truncating the table should be sufficient here, there’s no need to delete it as well.
(+) Line 291: table names should be enclosed in {curly_brackets} [sql_curly]
$sql = "Select * from country_url_data";
(-) Line 358: It looks like both minimized and development versions of the sweet alert javascript files are added on the site. Only one of them should be necessary.
(*) Line 419: It looks like the module calls out to http://ipinfo.io on every page load, but only uses the results of that call on the home page. For performance reasons, better to first check if you’re on the home page, and only then do the IP lookup.
(+) Line 475: table names should be enclosed in {curly_brackets} [sql_curly]
$result = db_query("Select * from country_url_data");
(+) Line 495: table names should be enclosed in {curly_brackets} [sql_curly]
$result = db_query("Select * from country_url_data");

ip_locator_with_splash.install
(+) Line 69, Line 82 - There is no need to call drupal_install_schema() or drupal_uninstall_schema() - see https://www.drupal.org/update/modules/6/7#install-schema. On Line 82, there’s also no need to truncate the country_url_data table, since it will have been removed.
(-) Line 72: The $msg variable should be translated using t(), not just the text of link.

Default variable set on admin form and other things working good.

Thanks

Default variable set on admin form and other things working good.

Thanks

Set back to "Needs Work" based on my review (which was delayed in posting due to spam filter issues)

It's not that the modules are the same, it's that some of the functionality that's implemented in your module (specifically, the identification of country by IP address) duplicates functionality that's provided by that module. You could require a dependency on that module, and then use their API to retrieve the country.

It's possible that relying on that module introduces some constraints (for example, see "Configuration / Cache Performance Note" on the Smart IP home page) that precludes using it in this module, but it's worth assessing. If it's not a good fit, I'd suggest explaining why in the project documentation. (On that note, it's also probably worthwhile to give some information in the project documentation about the IP geolocation service that you're using).

Hope this helps.

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxlhuria942560365git

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

Automated Review :
Coder found no warnings, issues with the project.

Manual Review:

Individual user account
Yes, Follows
No duplication
Yes,Follows
Master Branch
Yes: Follows
Licensing
Yes: Follows.
3rd party assets/code :
Yes, follows
Code long/complex enough for review
Yes, follows
Security Code:
Yes,follows

A few recommendations
1. If I do not have the library, it should give me an error message on the configuration page.
2. I did not find a way to remove a country once I set that up.
3. Getting an error because there was no sweetalert-dev.js file in the folder I downloaded from github. You can remove it if its not required.
4. Since you have two configuration pages , you should set them up as menu tabs so its easier to navigate

Last changes looks good. Just did a manual review of this module.

Though I had to setup IP addresses of different countries to test this module. In future, you could create something to test this in a easy way. As far as the complexity of this module is, it looks okay for such lengthy code.

So This looks good to go!!

Thanks

Up next in my queue.

Automated Review

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

• 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

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Maybe: Causes module duplication and/or fragmentation. Partial overlap w/ similar geolocation modules.

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

Yes: Follows the guidelines for in-project documentation and/or the README Template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

(*) There are two security problems with this module. Assigning to @th_tushar for PAR security training.

Coding style & Drupal API usage

ip_locator_with_splash_schema(), you don't need the !db_table_exists('country_url_data'); in fact this may cause weird things to happen.

ip_locator_with_splash_install(), don't build translatable links that wat. See https://www.drupal.org/node/322774

ip_locator_with_splash_enable() is not needed; just use default values (plus, this gets called each time the module is enabled).

ip_locator_with_splash_disable() should be in a hook_uninstall().

ip_locator_with_splash_admin_setting_form_delete_submit() should be able use a $form_state['#redirect'] instead of drupal_goto().

You have a "Implements hook_form()." These are form builder functions. See https://www.drupal.org/node/1354#forms

Try to be consistent with single quotes vs double quotes; single quotes are preferred when possible.

ip_locator_with_splash_splash_setting_form() has a few untranslated strings (Yes, No).

ip_locator_with_splash_admin_setting_form_delete() should be able to get the parameter from the argument and not have to use arg()?

ip_locator_with_splash_preprocess_page() alters the global $base_url? Bad things may happen here.

PhpStorm() is showing some unused variables in ip_locator_with_splash_preprocess_page();

ip_locator_with_splash_preprocess_page(), all variable_get() calls should have default values.

The inline JS ip_locator_with_splash_preprocess_page() should really be in a different file.

ip_locator_with_splash_get_location_info_by_ip(), file_get_contents() of a remote URL may not work on all systems. Use drupal_http_request().

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.

Automated Review

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

• 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

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Maybe: Causes module duplication and/or fragmentation.

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

Yes: Follows the guidelines for in-project documentation and/or the README Template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

(*) There are two security problems with this module.

Coding style & Drupal API usage

* Remove double quotes for package in ip_locator_with_splash.info file.

* After module installation, got the below message, HTML for the link to configuration page is displayed.

Please make sure website urls according to country codes are entered on our module's configuration page for the correct functioning of Locator. Click <a href="/admin/config/system/ip-locator-configuration">here</a> to visit configuration page.

* Access callback set to TRUE for admin path "admin/config/system/ip-locator-configuration/%/delete" in ip_locator_with_splash.module file. Please add proper permissions for the admin paths.

* All user facing text should be passed through translate t() function. Below are the findings,

Line #51 in ip_locator_with_splash.admin.inc file, "No result found." should be passed through translate function.

'#markup' => '<div class="ip-locator-help">No result found.</div>',

Line #55 in ip_locator_with_splash.admin.inc file, "Update" should be passed through translate function.

$form['actions']['submit'] = array('#type' => 'submit', '#value' => 'Update');

Line #140 in ip_locator_with_splash.admin.inc file, "Are you sure want to flush all urls?" should be passed through translate function.

return confirm_form($form, 'Are you sure want to flush all urls?', '/admin/config/system/ip-locator-configuration');

* The select list option's array values should be passed through translate function.

$option_at = array(
    'pop' => 'pop',
    'slide-from-top' => 'slide-from-top',
    'slide-from-bottom' => 'slide-from-bottom',
  );

$option_sl = array(
    'true' => 'Yes',
    'false' => 'No',
  );

$option_sc = array(
    'true' => 'Yes',
    'false' => 'No',
  );

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.

In "How to configure:" section on project page,

Points #1,#2 and Point #6 means the same, Either keep Points #1,#2 or keep Point #6.

* In ip_locator_with_splash.install file, use placeholder in translate function to replace link in ip_locator_with_splash_install().

* For SQL select statements, use db_select() instead of db_query().

Check below code, in ip_locator_with_splash_admin_setting_form() fucntion,

$count_nodes = db_query('SELECT id FROM {country_url_data}')->rowCount();

$sql = "Select * from {country_url_data}";
  $result = db_query($sql);

Check below code, in ip_locator_with_splash_check_if_website_available() function,

$result = db_query("Select * from {country_url_data}");

* Use drupal_http_header() to fetch the data. Check below code,

In ip_locator_with_splash_get_location_info_by_ip() function,

$ip_data = drupal_json_decode(file_get_contents("http://ipinfo.io/" . $ip_addr));

* The module is not showing any warnings/errors to user even if the library is not downloaded and placed in libraries folder. sweetalert.min.js file not found error is displaying in browser console.

Please fix the above findings and change the status back to "Needs review".

In ip_locator_with_splash.install file, use placeholder in translate function to replace link in ip_locator_with_splash_install() as it is implemented in below code.

$t('IP Locator with Splash requires Sweetalert library. Please download the Sweetalert library from !link.', array('!link' => l($t('Sweetalert download homepage'), 'https://github.com/lhuria94/sweetalert/archive/master.zip')));

You can ignore the db_query statements.

Its fine that you are showing a warning on reports page, but then the library should also not be loaded which is causing a sweetalert.js file not found error.

None of the current issues raised are blockers. I will look at the security issue resolutions tonight, and post what I had as my notes on what they were.

Automated Review

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

• Remove all .DS_Store files from your repository.
• ESLint has found some issues with your code (please check the JavaScript coding standards).
  

  /Users/matt/PAR/pareview_temp/js/locator.js: line 6, col 1, Error - Definition for rule 'keyword-spacing' was not found (keyword-spacing)
  
  1 problem
  

• 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

Secure code

From #24, my findings were:

Looks like in ip_locator_with_splash_menu(), $items['admin/config/system/ip-locator-configuration/%/delete'] has 'access callback' => TRUE, which means that anyone can delete an entry. Needs a permission.

Your inline JS in ip_locator_with_splash_preprocess_page() is XSS vulnerable b/c the variables are used rirectly with string concatenation. Move these values to settings and use a separate file.

These look like they are addressed. The menu path has a permission, and the variables are added onto the page via settings (which get escaped in the process).

My security issues were the blocking issues. This is RTBC. Assigning to @naveenvalecha for a second look if he has time. If this is still RTBC in seven days, I will approve.

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.

Removing review bonus tag, the first review link was a review by another user? Can you do one more review and add that?

OK, a week at RTBC w/o any objections.

Thanks for your contribution, lhuria94!

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.