Hi @czolnowski, @mattab asked me to do a review:

architectural issues:

• LocationFetcher & LocationFetcherProvider seem improperly scoped to me:
  • LocationFetcherProvider's only purpose is to create a LocationProvider instance, and the logic to do so is not complex, nor does it need to be overridden. IMO there is not enough reason to create a new class. Instead a LocationProvider can be supplied directly to objects that need it. Please remove this class and move a minimal amount of logic to the LocationFetcher class.
  • LocationFetcher:
    • this class does exactly the same thing as a LocationProvider, it returns a location based on visitor info. The only difference is that it wraps a LocationProvider instance (ie, LocationFetcher is an Adapter). It should, therefore, be part of the LocationProvider hierarchy instead of a separate class. I can think of two ways to accomplish this (maybe you can think of a better one). Either:
      • derive directly from LocationProvider and make sure it doesn't appear in the UserCountry admin page
      • or, create an interface w/ the 'getLocation' method & make LocationProvider & LocationFetcher implement it.
    • after the above change, LocationFetcher should be named to something more meaningful. The name should tell developers that see it that it doesn't just fetch a location (which every other LocationProvider does), but that it uses a cache and will try to use the default provider.
    • getLocationDetail is used in one place and the only thing it does is access an array. this doesn't merit an extra method, please remove it.
• LogsRepository & it's MySQL descendant have the following issues:
  • DAOs in Piwik are not currently named Repository, this is a Doctrine convention and currently not a Piwik convention and so should not appear in core Piwik code. Please rename the class so:
    1. it does not contain the word Repository
    2. it describes more clearly what the class does and is meant for. For example, the class that aggregates log data is called LogAggregator.
  • Currently plugin-specific DAOs are stored in the Model subdirectory of a plugin. Since this class is not specific to UserCountry, it shouldn't be stored there but in core/ somewhere. Piwik core does not have a full Data Access layer, but what it does have is stored in core/DataAccess, so please put the class there.
  • Since there is currently no data access layer in Piwik Core, the interface should be removed. When it is decided what the data access layer will look like, an interface may be added again.
  • The LogsRepository::getVisitsWithDatesLimit method should allow any fields to be selected, not just location fields, and should not select location_ip by default.
  • Some of this code depends on PDO, will it work when MYSQLI is used?
  • Ideally, DAOs in Piwik should not return cursors, since this is a concept that may not be implemented by all data stores. Can you instead return a Traversable instance and iterate over it using a foreach?
  • In updateVisits & updateConversions, there are two parameters when there could be just one. Instead of using one array for column fields & one for values, please use one where the keys are columns and the values are column values. This will make calling code easier to debug (since you won't have to keep to separate arrays in sync).

NOTE: When modifying Piwik core modules, please don't be afraid to modify Piwik core if it improves the architecture of your code or Piwik's. Such changes should be discussed before going into master, but that doesn't mean you can't make them.

code issues:

• AttributeHistoricalDataWithLocations::execute has a lot of code, can you split it up into smaller private methods?
• On AttributeHistoricalDataWithLocations.php, line 237 you return 100 if percentStep < 1. This if statement can be combined w/ the above one, and please add a short comment about what defaulting to 100 will result in.
• Please document all the code that has the possibility to be re-used in the future (either by core devs, pro devs or non-Piwik related plugin devs). This includes public & protected methods of all re-usable classes & all interfaces.

Thank you for this proposed pull request.

Because it was last updated more than one month ago, it is our policy to close pull requests opened for a long time without updates. If you would like to continue work on the pull request, please simply ping us to have it re-opened (after you have pushed a new commit).

We hope you understand this and we look forward to seeing an update from you on this pull request or another one!

Thanks.