Useless code spotted. #4572

anonymous-piwik-user opened this Issue Jan 25, 2014 · 2 comments

3 participants


File: core/Common.php
Line #: On or about 942
Function: getCampaignParameters()
Line: array_walk_recursive($return, 'trim');

This should do nothing. 'trim' does not meet the callback requirements of array_walk_recursive.

callback expects 2 parameters, my_callback(&$value, $index). trim takes trim($str, $charset)
a) $index will mess up charset
b) if you want to change $value it needs to be referenced, trim returns the value

Suggested resolutions:
1) remove offending line, it has never worked and no one noticed.
2) Move the trim function to the loop
From This:
foreach ($return as &$list) {
if (strpos($list, ',') !== false) {
$list = explode(',', $list);
} else {
$list = array($list);
To This:
foreach ($return as &$list) {
if (strpos($list, ',') !== false) {
$list = array_map('trim', explode(',', $list));
} else {
$list = array(trim($list));

3) Create wrapper function for trim:
array_walk_recursive($return, create_function('&$v, $k', '$v = trim($v);');

I am currently playing with getting piwik running on hhvm. I am was getting a warning from the offending line. That is how and why I cam across this.

Code History:
The code was added with regard to ticket #855
with a commit message of:

Fixes #855 Now detecting google style campaign parameters (utm_campaign, utm_term)
git-svn-id: 59fd770c-687e-43c8-a1e3-f5a4ff64c105

Committed By: mattpiwik
On: 2011-04-10 23:36:30

Piwik Open Source Analytics member

In d71d0b1: Fixes #4572 Make code work. Thanks for the report! (please keep feedback coming about HipHop virtual machine! that's super interesting :))

@anonymous-piwik-user anonymous-piwik-user added this to the 2.1 - Piwik 2.1 milestone Jul 8, 2014

This comment is irrelevant to the issue.

Just giving an update on HHVM. This was a while ago, so, I am going on my memory of events. I was successfully able to get the collectors working under HHVM and it did improve performance significantly. The admin interface was still very buggy, however, I didn't need or work on getting that running because we had a standard Apache server which the ad-ops guys used.

A couple of the issues which came up were:
1) There were a few PHP Constants that HHVM didn't have pre-define, so, I just defined them in the root php file.
2) There was an issue with some function (I forget the name), but luckily you had an if statement which switched between the Core PHP defined function and an equivalent User defined function. I don't remember why the if statement was selecting the PHP defined version, but I basically just removed the if statement and forced the User defined function to always be used.

I think those were the only issues and after that the collectors worked perfectly. Of course we only tested the aspects of the collectors which we were using, so, I am not sure if there may be other issues if other features are enabled.

@sabl0r sabl0r pushed a commit to sabl0r/piwik that referenced this issue Sep 23, 2014
@mattab mattab Fixes #4572 Make code work. Thanks for the report! (please keep feedb…
…ack coming about HipHop virtual machine! that's super interesting :))
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment