Add parameters to getJavascriptTag API call #127

This pull request brings the getJavascriptTag API call up to parity with the Piwik Administrative interface (Tracking Code section) by adding three boolean parameters:

• mergeSubdomains: Track visitors across all subdomains of [site name]
• prependDomain: Prepend the site domain to the page title when tracking
• hideAlias: In the "Outlinks" report, hide clicks to known alias URLs of [site name]

Please note that the new code makes database calls. I'm not certain how you normally sanitize user input so I have applied Common::sanitizeInputValue() to the variables that could be used to inject SQL. Please provide guidance if you prefer a different strategy.

Thank you for the pull request. Can I ask why you made such change?

WP-Piwik (WordPress plugin) uses this API call to get the JS tracking code for placement on WordPress sites. I want to "Track visitors across all subdomains of..." my site, but couldn't upgrade WP-Piwik (in a scalable way at least) unless the Piwik API supported the same options. While I only needed the one option (mergeSubdomains), I figured I'd implement all three while I was messing with that part of the codebase.

If it isn't immediately obvious, this dovetails with improvements I made to the PiwikTracker API. I use PiwikTracker to set the cookie in my ShortURL processor and then pick the user back up in WP-Piwik (once I add support for wildcard domain cookies).

+1 Good one!

review

• instead of Db::get()->fetchRow("SELECT main_url... ) you can use the API directly such as: \Piwik\Plugins\SitesManager\API::getInstance()->getSiteUrlsFromId( $idSite )
• you dont need to check for "valid urls" since the URL are already validated
• after these changes the code should be much simpler

Excellent. One clarification...

• Site URLs can have a path attached.
• Do you know (off hand) if _paq.push(["setDomains", ... can accept paths?
• Otherwise, I'll still run parse_url to isolate the host from the path

• reuse existing variable name as used in the javascript for groupPageTitlesByDomain
• you dont need to write out the comment if the url is invalid, it should not happen as we validate URLs
• setDomains does not accept path so indeed you need write path only
• API needs to be called only once as it returns Main URL (index 0) and then alias URLs

OK. I've applied the requested changes and (manually) tested the code.

I might as well add the 4 advanced parameters from the Tracking Code page while I'm in this part of the code. From your viewpoint, is the best approach to continue to expand the function's parameters:

public function getJavascriptTag($idSite, $piwikUrl = '', $mergeSubdomains = false, $groupPageTitlesByDomain = false, $mergeAliasUrls = false, $newParam1 = false, $newParam2 = false, $newParam3 = false, $newParam4 = false);

Alternatively, the function could be defined more like:

public function getJavascriptTag($idSite, $piwikUrl = '', $options = array());

...where $options is an array built by the API client that includes keys like mergeSubdomains, groupPageTitlesByDomain, and mergeAliasUrls.

Better use named parameters because the web API will accept named parameters in any order and because they are all optional it looks good!

Updated:

• Rebased to current version -- let me know if I should be doing something else to minimize the impact on your end.
• Added code for the 4 remaining advanced options. I actually adjusted the logic to more closely parallel jsTrackingGenerator.js and put comments on both ends to note that changes should be mirrored in the other location.
• I assumed that API parameters will be formatted like the params variable of jsTrackingGenerator.js (line 190). In theory, it should be possible to substitute an API call for lines 216 to 270. That would also be the easiest way to thoroughly test the new code, but I don't know how you normally make API calls in JS.