Just for testing it would be nice to have the debug options added to the ip2country_get_country() function.

Great module.
Thanks.

CommentFileSizeAuthor
#4 784068-4.patch2.82 KBjoshi.rohit100

Comments

tr’s picture

Version: 6.x-1.x-dev » 7.x-1.x-dev

I will be implementing this first in the 7.x-1.x branch, then possibly backporting it to 6.x-1.x.

tr’s picture

Version: 7.x-1.x-dev » 8.x-1.x-dev
Issue summary: View changes
tr’s picture

Title: Add Debug Mode to ip2country_get_country() » Add debug mode to Ip2CountryManager::getCountry()

This module has a debug configuration setting, but that only affects lookups done through the administrative UI and the country code attached to the User when a user logs in. This debug setting should also affect lookups done by other modules using the ip2country.manager service.

joshi.rohit100’s picture

StatusFileSize
new2.82 KB

Added for D8

joshi.rohit100’s picture

Status: Active » Needs review
tr’s picture

Status: Needs review » Needs work

I'm thinking there should be setDebug()/getDebug() methods as part of the ip2country.manager service so that other modules can manipulate the debug configuration variable. The logger call can be moved to setDebug() so that only changes in debug are logged (not on every call to getCountry() like in your patch), and the Ip2CountrySettingsForm should use the new set/get methods instead of directly accessing the config. Also it looks like you check the test_type but only implement the test_country case, not the test_ip_address case?

Or, perhaps introduce a hook_ip2country_alter() - this is something which I thought of doing a long time ago, and there are even skeleton tests for this hook, though they don't do anything yet. The idea would be to call the hook inside getCountry() - this would allow us to alter the returned country in any way. Then debug mode is simply a hook implementation in ip2country.module which returns the debug country or ip depending on the test type.

I'm about to push some changes to a lot of these files as part of #2921155: Remove usage of deprecated ip2country_get_country(), which will break your patch...

joshi.rohit100’s picture

I am not sure about the setDebug()/getDebug() as semantically not making sense. For having alter hook as option, again the idea of debug mode from UI is that user enables the debug mode and it should work without writing any code. Although in getCountry() we can dispatch event for users to do something.

tr’s picture

Version: 8.x-1.x-dev » 2.0.x-dev

Moving to current development branch.