Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow disabling the usage of the default geolocation provider as fallback #18634

Merged
merged 6 commits into from Jan 26, 2022

Conversation

sgiehl
Copy link
Member

@sgiehl sgiehl commented Jan 17, 2022

Description:

fixes #18625

Review

@sgiehl sgiehl added not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Needs Review PRs that need a code review labels Jan 17, 2022
config/global.ini.php Outdated Show resolved Hide resolved
Copy link
Member

@tsteur tsteur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we create an FAQ for this and explain the use case when someone might do this?

BTW can this also be used to disable geolocation? We've had few people asking for this in the past and if so, we should create a separate FAQ

@sgiehl
Copy link
Member Author

sgiehl commented Jan 20, 2022

BTW can this also be used to disable geolocation? We've had few people asking for this in the past and if so, we should create a separate FAQ

Actually this only disables the fallback. So if no geolocation provider is configured -> the default is used guessing the country by the language will still be done. But if we want to make that possible, we could simply create some kind of Dummy provider, which always returns an empty result set.

@tsteur
Copy link
Member

tsteur commented Jan 20, 2022

@sgiehl getting bit confused. So if we disable enabled_geolocation_guess_country_fallback, then there's still some guessing and a fallback happening within the default provider? That might be getting bit confusing as there's then also a different guessing happening?

Also I see there's a setting [Tracker]enable_language_to_country_guess. Would that mean this config should be in Tracker category too?

@sgiehl
Copy link
Member Author

sgiehl commented Jan 20, 2022

Guess could make sense to move it to Tracker. The default provider is doing the language to country guess.
If you have configured e.g. GeoIP2 and this provider doesn't return any results then the GeoLocator falls back to the default provider. And that fallback can be disabled with the config. By default Matomo uses the default provider and that currently can't be disabled.
Hope that makes it a bit clearer.

@tsteur
Copy link
Member

tsteur commented Jan 20, 2022

Does it make sense in that case to name the setting like enable_default_location_provider?

When disabled, we use a Dummy provider. This way we can tackle this specific use case, plus we can also handle the use case for disabling geolocation altogether if I see this right? Also the name would be a bit more clear maybe and it be easier to explain/understand.

Would also need some FAQs then.

@sgiehl sgiehl force-pushed the m18625 branch 2 times, most recently from f9880bb to c5aed09 Compare January 21, 2022 10:41
@sgiehl
Copy link
Member Author

sgiehl commented Jan 21, 2022

@tsteur I've now adjusted everything a bit again. We now have a new Disabled provider. If that one is chosen geolocation is disabled. The current default provider is still used as default and fallback unless the Tracker option enable_default_location_provider is disabled. In that case the Disabled provider will be used as default and fallback.
Hope that makes everything a bit easier to understand.

Copy link
Member

@tsteur tsteur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left one more comment, otherwise looks good. Feel free to merge after the FAQs have been created.

CHANGELOG.md Outdated Show resolved Hide resolved
@sgiehl sgiehl added this to the 4.8.0 milestone Jan 26, 2022
@sgiehl sgiehl merged commit 2ba0f7a into 4.x-dev Jan 26, 2022
@sgiehl sgiehl deleted the m18625 branch January 26, 2022 19:50
@sgiehl
Copy link
Member Author

sgiehl commented Jan 26, 2022

As of Matomo 4.8. there will be a new config setting [Tracker] enable_default_location_provider = 0, which can be used to disable the default geolocation provider. That way the fallback to this provider can also be disabled.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Geolocation wrong on internal network even with enable_language_to_country_guess=0
2 participants