@JE4GLE opened this Pull Request on June 8th 2021 Contributor

Description:

This Pull Request solves the issue mentioned in the following issue: https://github.com/matomo-org/matomo/issues/16942

It would allow for a combination of the PHP tracker (to track 100% of the PageViews) and the JavaScript tracker (to track screen size, plugins, …) by first generating a page view id on the PHP side and setting it after page generation using JavaScript.

Review

  • [ ] Functional review done
  • [ ] Potential edge cases thought about (behavior of the code with strange input, with strange internal state or possible interactions with other Matomo subsystems)
  • [ ] Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
  • [ ] Security review done see checklist
  • [ ] Code review done
  • [ ] Tests were added if useful/possible
  • [ ] Reviewed for breaking changes
  • [ ] Developer changelog updated if needed
  • [ ] Documentation added if needed
  • [ ] Existing documentation updated if needed
@diosmosis commented on June 9th 2021 Member

Hi @JE4GLE, thanks for taking the time to create this pull request! Currently the jslint tests are failing, I believe because the new method is not currently in the list here: https://github.com/matomo-org/matomo/blob/4.x-dev/js/piwik.js#L66. Would you be able to add it?

@diosmosis commented on June 9th 2021 Member

build js

@diosmosis commented on June 9th 2021 Member

Hmm, looks like the tracker javascript building workflow isn't working on this PR. I'll build it manually before merging.

@diosmosis commented on June 9th 2021 Member

@JE4GLE Looks like this change would create a regression, actually. logPageview() generates a pageview id, and is supposed to generate a new one on every subsequent call so new pageviews have new IDs. Now, however, the pageview ID is only set on the first call, then since it's already set at the second call, nothing changes.

This could be fixed pretty easily by having another variable like configIdPageViewSetManually that is set to true in setPageViewId(). Then we check this in logPageview(). However I'm not sure if this causes problems for your usecase. Would you be able to change the pageview ID and still have it determined by the PHP tracker code you're using (assuming I understand the use case correctly)?

@JE4GLE commented on June 9th 2021 Contributor

@diosmosis My use case would be:

  1. Create a pageview request with a pageview ID in PHP and place it in the HTML code, before page generation
  2. After the page has loaded, the JavaScript code will then read this ID in the HTML code and set it manually
  3. Send the pageview again using JavaScript, but this time including additional data from JavaScript

Unfortunately I don't quite understand, why the pageview ID would change during one page visit. In my understanding, the user visits a page and a pageview ID is generated, which will stay the same, until the user reloads the page or goes to another one.

Anyways, here's your proposed change:

@diosmosis commented on June 9th 2021 Member

@JE4GLE

Unfortunately I don't quite understand, why the pageview ID would change during one page visit. In my understanding, the user visits a page and a pageview ID is generated, which will stay the same, until the user reloads the page or goes to another one.

After logPageview() is called we expect the pageview to be over, and subsequent calls to use a different ID. In a normal website where you click links that load new pages, this wouldn't be needed, but for single page apps where the page never reloads, and the app calls logPageview() itself multiple times without the page reloading, this change would break things.

I guess if you're expecting only one pageview before a new page is loaded or the current page is reloaded, then I suppose you wouldn't be affected by this.

@diosmosis commented on June 9th 2021 Member

The change looks good now and the build passes. Thanks again for taking the trouble to make the PR and apply fixes @JE4GLE!

This Pull Request was closed on June 9th 2021
Powered by GitHub Issue Mirror