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

Useless code spotted. #4572

Closed
anonymous-matomo-user opened this issue Jan 25, 2014 · 2 comments
Closed

Useless code spotted. #4572

anonymous-matomo-user opened this issue Jan 25, 2014 · 2 comments
Assignees
Labels
Bug For errors / faults / flaws / inconsistencies etc.
Milestone

Comments

@anonymous-matomo-user
Copy link

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);');

Backstory:
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: http://dev.piwik.org/svn/trunk@4389 59fd770c-687e-43c8-a1e3-f5a4ff64c105

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

@mattab
Copy link
Member

mattab commented Jan 27, 2014

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

@amcnea
Copy link

amcnea commented Jul 9, 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 pushed a commit to sabl0r/piwik that referenced this issue Sep 23, 2014
…keep feedback 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
Labels
Bug For errors / faults / flaws / inconsistencies etc.
Projects
None yet
Development

No branches or pull requests

3 participants