fix for array value handling in sumRowArray() #14917
Conversation
|
CC @tsteur as this relates to AbTesting |
|
The PR is likely fine to merge but need to look further into it. AFAIK from what I see for Maybe it changed which test is applied. Like it used to use a TTest because a goal allowed multiple conversions in one version and this is no longer the case. Then old reports would maybe still have that metric even though it's maybe no longer used. |
|
could you maybe try this patch in a/b testing instead? diff --git a/Archiver.php b/Archiver.php
index d0cc250..67d5629 100644
--- a/Archiver.php
+++ b/Archiver.php
@@ -348,11 +348,7 @@ class Archiver extends \Piwik\Plugin\Archiver
$ttestMetrics = array();
foreach ($experimentMetrics as $metric) {
- $bestStrategy = $this->strategy->getBestStrategyForMetric($metric, $experiment['idexperiment'], $experiment['idsite']);
-
- if ($bestStrategy === Strategy::TTEST) {
- $ttestMetrics[] = $metric;
- }
+ $ttestMetrics[] = $metric;
}
$unique = $this->aggregator->getUniqueVisitors($experiment, $onlyEntered = false);I've been looking at this PR for quite a while and not sure if we should merge or not. In theory it looks good. Bit scared it may sum wrong values when there is eg a zero index in |
|
Hi @MichaelHeerklotz - Thanks for creating the PR. |
|
@MichaelHeerklotz thanks for confirming. We will release an update for A/B Testing and I reckon we can close this PR for now? |
Since some time the archive job crashes in my setup with
What I found out after some debuging:
The AbTesting plugin causes sumRowArray($thisColumnValue, $columnToSumValue) to be called
a) with a float value in $thisColumnValue and an array in $columnToSumValue
In this case the
if (!isset($newValue[$arrayIndex])) {check is triggered, however, because $newValue is a float, PHP will not convert $newValue to an array, and the $newValue[$arrayIndex] will pass a value of "null" to the recursive sumRowArray() call. This will trigger the above exception.Because the documentation of sumRowArray() allows $columnToSumValue to be an array, this is most likely a bug.
b) with an array $thisColumnValue and a float value in $columnToSumValue
This case is not allowed by the function documentation, but maybe it makes sense to handle this case, too?
These changes should be reviewed with care, as I do not yet fully understand why the AbTesting plugin is causing these problems.