@nstCactus opened this Pull Request on October 25th 2013 Contributor

I added two options to the Privacy settings page:

  • Allow using non-anonymized IP address to determine the visitor's location
  • Allow using non-anonymized IP address to determine the visitor's ISP

When these are activated, the visitor's original IP address is used to determine location and/or ISP, in order to get a more accurate value.

The IP address saved in the log_visit table will honor the ip_address_mask_length setting.

@mattab commented on October 27th 2013 Member

Thanks for the PR, very nice feature idea which was suggested few times!

Code review:

  • I propose to KISS and have only one setting for all plugins. Called use_anonymized_ip_for_visit_enrichment = 1 by default
  • plugins can read this setting and decide to use the request->getIp or the anonymized one.
  • I modified the hooks so they pass the $request object which you get the original IP by $request->getIp() or the anon ip in $visitor['visitor_ip']
    • (Calling getIpFromHeader() does not work as it would skip the enforced IP use case)
  • there are white space differences and code inconsistencies (" instead of ' used in lines around etc)
@nstCactus commented on October 27th 2013 Contributor

I'm working on it. Any idea for the documentation text for the use_anonymized_ip_for_visit_enrichment setting in global.ini?

@mattab commented on October 27th 2013 Member

If you have enabled the Anonymize IP feature, by default, Geo Location via geoip and Provider reverse name lookups will use the anonymized IP address. You can set this setting to let plugins use the full IP address when enriching visitor information. Note: the IP will always be stored anonymized in the database.

@nstCactus commented on October 28th 2013 Contributor

Thank you for your suggestions.

@mattab commented on October 28th 2013 Member

de rien ├ža fait plaisir de voir de telles contributions ! :)

@nstCactus commented on November 1st 2013 Contributor

Requested changes done and commits squashed into one. Ready to merge.
I've been testing it on my running piwik instance. Everything's OK so far.

@mattab commented on November 3rd 2013 Member
This Pull Request was closed on November 3rd 2013
Powered by GitHub Issue Mirror