Minimal change that allows proxying plugins/HeatmapSessionRecording/config.php. #35
Conversation
|
FYI the file is called |
|
@mattab Tested locally, proxies to heatmap configs.php and query params are present in request. |
|
I just tried if it works to record Sessions and Heatmaps via the proxy and I am running across three issues: I am pretty sure this line: Lines 91 to 93 in bdcb5d1 should read if(isset($_GET['send_image']) && $_GET['send_image'] == 1) {
Otherwise every request without an Now that the plugin has gotten the config it tries to send the clicked position via GET requests similar to this one: But it seems like the part that tries to put the GET parameters back together fails, as PHP creates arrays out of them: A There seems to be another issue: |
|
@Findus23 thanks for testing and catching these issues, I’ll probably look into them today |
…/configs.php and use http_build_query() in piwik.php so array query param values are correctly transformed.
|
@Findus23 fixed the first two issues you mentioned, and wasn't able to reproduce the third (possibly because one of the other two changes fixed that). If you feel so inclined, would appreciate another test :) Note: added a |
|
@matomo-org/core-team could use another code review too |
|
@diosmosis So the HSR plugin is now getting the click data, but the third issue still persists.
I also found a similar issue when testing: #36 |
|
Reproduced the POST issue finally, nice catch! Should be able to fix that + the sendBeacon issue in this PR. And probably #30. |
…tent-type header returned by the actual endpoint (like a proxy).
|
The heatmap POST should work and now the content-type header is simply forwarded from the result. Needs another testing pass & code review. Note: if the sendBeacon() request sends a POST, then it should work w/ this change, too, but I was unable to find a way to test it. |
|
@diosmosis I tested it and everything seems to be working fine. Now we only need to forward the OptOut-iFrame (#29) and I'd say that the proxy is up-to-date. |
|
Forwarding more headers now. Note: #29 will require proxying index.php so I think it should go into a new PR. |
piwik.php
Outdated
|
|
||
| foreach ($_GET as $key => $value) { | ||
| $url .= urlencode($key ). '=' . urlencode($value) . '&'; | ||
| if (!isset($path)) { |
There was a problem hiding this comment.
We may need to init $path initially. I'm thinking of older PHP versions where it is possible to say like ?path=... and then it is available as a variable. It's not a huge risk but it would allow users to potentially call any file which may be for example IP restricted. But because the tracker proxy may be running in the same network etc the request would be allowed. As said, it is not really a big issue, but better be safe. May need to have a another file that is not directly executable which both configs.php and piwik.php can include or so.
There was a problem hiding this comment.
An init.php file sounds good, just in case someone creates an executable config.php or something.
There was a problem hiding this comment.
Setting $path in piwik.php (either directly or indirectly) doesn't work since it would overwrite the value set in, eg, configs.php. Solved this by renaming piwik.php => proxy.php & including that w/ $path set. So this way, it's never uninitialized.
piwik.php
Outdated
| @@ -182,7 +241,7 @@ function getHttpContentAndStatus($url, $timeout, $user_agent) | |||
|
|
|||
| return array( | |||
| $content, | |||
| $httpStatus | |||
| $httpStatus, | |||
There was a problem hiding this comment.
there is a trailing comma, but not important
There was a problem hiding this comment.
Left when undoing a change. Will leave it for the blame.
|
Worked for me 👍 only left a comment otherwise |
…his way `$path` is always defined.
…ure URL is mapped in proxy output.
| * @link http://piwik.org/faq/how-to/#faq_132 | ||
| * @license http://www.gnu.org/licenses/gpl-3.0.html GPL v3 or later | ||
| */ | ||
|
|
There was a problem hiding this comment.
can we maybe in the other two files, define a constant, and then do something like this?
if (!defined( 'MYVARIABLENAME')) {
exit; // this file is not supposed to be accessed directly
}
proxy.php
Outdated
|
|
||
| $content = str_replace($TOKEN_AUTH, '<token>', $content); |
There was a problem hiding this comment.
would this mess up with a set content-length? I wonder if we need to use something like str_pad() to keep the same length or adjust the content-length header?
There was a problem hiding this comment.
Yeah, would mess it up, will make the content-length dynamic.
…dynamic content-length in case php does not autocorrect it.
Changes:
piwik.php.config.phpendpoint that includespiwik.phpw/$pathset.Fixes #26
Fixes #30
Fixes #36