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

Add parameters to getJavascriptTag API call #127

Merged
merged 9 commits into from Oct 28, 2013

Conversation

claytondaley
Copy link
Contributor

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.

@mattab
Copy link
Member

mattab commented Oct 16, 2013

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

@claytondaley
Copy link
Contributor Author

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.

@claytondaley
Copy link
Contributor Author

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).

@halfdan
Copy link
Member

halfdan commented Oct 16, 2013

+1 Good one!

@mattab
Copy link
Member

mattab commented Oct 22, 2013

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

@claytondaley
Copy link
Contributor Author

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

@mattab
Copy link
Member

mattab commented Oct 23, 2013

  • 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

@claytondaley
Copy link
Contributor Author

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.

…e 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>
…verification, but leaves parsing to separate the host (required) from the path (unwanted). Renames several variables to more closely mirror jsTrackingGenerator.js.
…ManagerAPI. Added "use" SitesManagerAPI to Piwik.php.
@mattab
Copy link
Member

mattab commented Oct 24, 2013

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

@claytondaley
Copy link
Contributor Author

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.

mattab pushed a commit that referenced this pull request Oct 28, 2013
Add parameters to getJavascriptTag API call Fixes #4247
@mattab mattab merged commit de56af3 into matomo-org:master Oct 28, 2013
@claytondaley claytondaley deleted the api-javascript-tag-options branch October 28, 2013 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants