use union of segments for custom variables

8 files changed, 44 insertions, 73 deletions

diff --git a/core/Segment/SegmentExpression.php b/core/Segment/SegmentExpression.php
index cd2e0038bd..5f1a7fea27 100644
--- a/core/Segment/SegmentExpression.php
+++ b/core/Segment/SegmentExpression.php
@@ -158,11 +158,14 @@ class SegmentExpression
             $operator = $leaf[self::INDEX_BOOL_OPERATOR];
             $operandDefinition = $leaf[self::INDEX_OPERAND];
 
-
             $operand = $this->getSqlMatchFromDefinition($operandDefinition, $availableTables);
 
             if ($operand[self::INDEX_OPERAND_OPERATOR] !== null) {
-                $this->valuesBind = array_merge($this->valuesBind, $operand[1]);
+                if (is_array($operand[self::INDEX_OPERAND_OPERATOR])) {
+                    $this->valuesBind = array_merge($this->valuesBind, $operand[self::INDEX_OPERAND_OPERATOR]);
+                } else {
+                    $this->valuesBind[] = $operand[self::INDEX_OPERAND_OPERATOR];
+                }
             }
 
             $operand = $operand[self::INDEX_OPERAND_NAME];
@@ -189,12 +192,12 @@ class SegmentExpression
      */
     protected function getSqlMatchFromDefinition($def, &$availableTables)
     {
-        $fields    = $def[0];
+        $field     = $def[0];
         $matchType = $def[1];
         $value     = $def[2];
 
         // Segment::getCleanedExpression() may return array(null, $matchType, null)
-        $operandWillNotMatchAnyRow = empty($fields) && is_null($value);
+        $operandWillNotMatchAnyRow = empty($field) && is_null($value);
         if($operandWillNotMatchAnyRow) {
             if($matchType == self::MATCH_EQUAL) {
                 // eg. pageUrl==DoesNotExist
@@ -221,10 +224,6 @@ class SegmentExpression
             return array($sqlExpression, $value = null);
         }
 
-        if (!is_array($fields)) {
-            $fields = array($fields);
-        }
-
         $alsoMatchNULLValues = false;
         switch ($matchType) {
             case self::MATCH_EQUAL:
@@ -289,44 +288,23 @@ class SegmentExpression
 
         // We match NULL values when rows are excluded only when we are not doing a
         $alsoMatchNULLValues = $alsoMatchNULLValues && !empty($value);
+        $sqlMatch = str_replace('%s', $field, $sqlMatch);
 
-        $sqlExpressions = array();
-        $values = array();
-        foreach ($fields as $field) {
-            $sqlMatchReplaced = str_replace('%s', $field, $sqlMatch);
-
-            if ($matchType === self::MATCH_ACTIONS_CONTAINS
-                || is_null($value)
-            ) {
-                $sqlExpression = "( $sqlMatchReplaced )";
+        if ($matchType === self::MATCH_ACTIONS_CONTAINS
+            || is_null($value)
+        ) {
+            $sqlExpression = "( $sqlMatch )";
+        } else {
+            if ($alsoMatchNULLValues) {
+                $sqlExpression = "( $field IS NULL OR $sqlMatch ? )";
             } else {
-                if ($alsoMatchNULLValues) {
-                    $sqlExpression = "( $field IS NULL OR $sqlMatchReplaced ? )";
-                } else {
-                    $sqlExpression = "$sqlMatchReplaced ?";
-                }
+                $sqlExpression = "$sqlMatch ?";
             }
-
-            $sqlExpressions[] = $sqlExpression;
-
-            if ($value !== null) {
-                if(is_array($value)) {
-                    $values = array_merge($values, $value);
-                } else {
-                    $values[] = $value;
-                }
-            }
-
-            $this->checkFieldIsAvailable($field, $availableTables);
         }
 
-        if (count($fields) == 1) {
-            $sqlExpression = reset($sqlExpressions);
-        } else {
-            $sqlExpression = '((' . implode(") OR (", $sqlExpressions) . '))';
-        }
+        $this->checkFieldIsAvailable($field, $availableTables);
 
-        return array($sqlExpression, $values);
+        return array($sqlExpression, $value);
     }
 
     /**
diff --git a/plugins/CustomVariables/Columns/Base.php b/plugins/CustomVariables/Columns/Base.php
index 1818ca3145..4c6a4ea542 100644
--- a/plugins/CustomVariables/Columns/Base.php
+++ b/plugins/CustomVariables/Columns/Base.php
@@ -16,7 +16,7 @@ use Piwik\Plugins\CustomVariables\CustomVariables;
 
 class Base extends VisitDimension
 {
-    protected function configureSegmentsFor($fieldPrefix, $segmentNameSuffix)
+    protected function configureSegmentsFor($segmentNameSuffix)
     {
         $numCustomVariables = CustomVariables::getNumUsableCustomVariables();
 
@@ -25,10 +25,7 @@ class Base extends VisitDimension
         $segment->setSegment('customVariable' . $segmentNameSuffix);
         $segment->setName($this->getName() . ' (' . Piwik::translate('CustomVariables_ScopeVisit') . ')');
         $segment->setCategory('CustomVariables_CustomVariables');
-        $segment->setSqlSegment($this->getSegmentColumns('log_visit.' . $fieldPrefix, $numCustomVariables));
-        $segment->setSuggestedValuesCallback(function ($idSite, $ignore, DataTable $table) use ($segmentNameSuffix) {
-            return $table->getColumnsStartingWith('customVariable' . $segmentNameSuffix);
-        });
+        $segment->setUnionOfSegments($this->getSegmentColumns('customVariable' . $segmentNameSuffix, $numCustomVariables));
         $this->addSegment($segment);
 
         $segment = new Segment();
@@ -36,10 +33,7 @@ class Base extends VisitDimension
         $segment->setSegment('customVariablePage' . $segmentNameSuffix);
         $segment->setName($this->getName() . ' (' . Piwik::translate('CustomVariables_ScopePage') . ')');
         $segment->setCategory('CustomVariables_CustomVariables');
-        $segment->setSqlSegment($this->getSegmentColumns('log_link_visit_action.' . $fieldPrefix, $numCustomVariables));
-        $segment->setSuggestedValuesCallback(function ($idSite, $ignore, DataTable $table) use ($segmentNameSuffix) {
-            return $table->getColumnsStartingWith('customVariablePage' . $segmentNameSuffix);
-        });
+        $segment->setUnionOfSegments($this->getSegmentColumns('customVariablePage' . $segmentNameSuffix, $numCustomVariables));
         $this->addSegment($segment);
     }
 
diff --git a/plugins/CustomVariables/Columns/CustomVariableName.php b/plugins/CustomVariables/Columns/CustomVariableName.php
index 85aa46436f..b42f2e8ae7 100644
--- a/plugins/CustomVariables/Columns/CustomVariableName.php
+++ b/plugins/CustomVariables/Columns/CustomVariableName.php
@@ -14,7 +14,7 @@ class CustomVariableName extends Base
 {
     protected function configureSegments()
     {
-        $this->configureSegmentsFor('custom_var_k', 'Name');
+        $this->configureSegmentsFor('Name');
     }
 
     public function getName()
diff --git a/plugins/CustomVariables/Columns/CustomVariableValue.php b/plugins/CustomVariables/Columns/CustomVariableValue.php
index 03046758cf..a565a8136c 100644
--- a/plugins/CustomVariables/Columns/CustomVariableValue.php
+++ b/plugins/CustomVariables/Columns/CustomVariableValue.php
@@ -14,7 +14,7 @@ class CustomVariableValue extends Base
 {
     protected function configureSegments()
     {
-        $this->configureSegmentsFor('custom_var_v', 'Value');
+        $this->configureSegmentsFor('Value');
     }
 
     public function getName()
diff --git a/tests/PHPUnit/Integration/SegmentTest.php b/tests/PHPUnit/Integration/SegmentTest.php
index f88d1248f6..483f047433 100644
--- a/tests/PHPUnit/Integration/SegmentTest.php
+++ b/tests/PHPUnit/Integration/SegmentTest.php
@@ -94,8 +94,7 @@ class SegmentTest extends IntegrationTestCase
 
             // test multiple column segments
             array('customVariableName==abc;customVariableValue==def', array(
-                'where' => ' ((log_visit.custom_var_k1 = ?) OR (log_visit.custom_var_k2 = ?) OR (log_visit.custom_var_k3 = ?) OR (log_visit.custom_var_k4 = ?) OR (log_visit.custom_var_k5 = ?))'
-                         . ' AND ((log_visit.custom_var_v1 = ?) OR (log_visit.custom_var_v2 = ?) OR (log_visit.custom_var_v3 = ?) OR (log_visit.custom_var_v4 = ?) OR (log_visit.custom_var_v5 = ?)) ',
+                'where' => ' (log_visit.custom_var_k1 = ? OR log_visit.custom_var_k2 = ? OR log_visit.custom_var_k3 = ? OR log_visit.custom_var_k4 = ? OR log_visit.custom_var_k5 = ?) AND (log_visit.custom_var_v1 = ? OR log_visit.custom_var_v2 = ? OR log_visit.custom_var_v3 = ? OR log_visit.custom_var_v4 = ? OR log_visit.custom_var_v5 = ? )',
                 'bind' => array(
                     'abc', 'abc', 'abc', 'abc', 'abc',
                     'def', 'def', 'def', 'def', 'def',
diff --git a/tests/PHPUnit/System/expected/test_AutoSuggestAPITest_customVariableName__API.getSuggestedValuesForSegment.xml b/tests/PHPUnit/System/expected/test_AutoSuggestAPITest_customVariableName__API.getSuggestedValuesForSegment.xml
index b3e5836d32..9c10ae8b37 100644
--- a/tests/PHPUnit/System/expected/test_AutoSuggestAPITest_customVariableName__API.getSuggestedValuesForSegment.xml
+++ b/tests/PHPUnit/System/expected/test_AutoSuggestAPITest_customVariableName__API.getSuggestedValuesForSegment.xml
@@ -1,5 +1,5 @@
 <?xml version="1.0" encoding="utf-8" ?>
 <result>
-	<row>Cvar 5 name</row>
 	<row>Cvar 1 name</row>
+	<row>Cvar 5 name</row>
 </result>
\ No newline at end of file
diff --git a/tests/PHPUnit/System/expected/test_AutoSuggestAPITest_customVariablePageValue__API.getSuggestedValuesForSegment.xml b/tests/PHPUnit/System/expected/test_AutoSuggestAPITest_customVariablePageValue__API.getSuggestedValuesForSegment.xml
index fb3a6b6413..0ab1e3a74c 100644
--- a/tests/PHPUnit/System/expected/test_AutoSuggestAPITest_customVariablePageValue__API.getSuggestedValuesForSegment.xml
+++ b/tests/PHPUnit/System/expected/test_AutoSuggestAPITest_customVariablePageValue__API.getSuggestedValuesForSegment.xml
@@ -1,22 +1,22 @@
 <?xml version="1.0" encoding="utf-8" ?>
 <result>
 	<row>CAT</row>
+	<row>Cvar5 PAGE value is 0</row>
 	<row>Cvar5 PAGE value is 1</row>
 	<row>Cvar2 PAGE value is 1</row>
 	<row>Cvar2 PAGE value is 0</row>
-	<row>Cvar5 PAGE value is 0</row>
-	<row>Cvar5 PAGE value is 3</row>
 	<row>Cvar2 PAGE value is 3</row>
-	<row>Cvar2 PAGE value is 2</row>
 	<row>Cvar5 PAGE value is 2</row>
-	<row>Cvar5 PAGE value is 4</row>
+	<row>Cvar5 PAGE value is 3</row>
+	<row>Cvar2 PAGE value is 2</row>
+	<row>Cvar2 PAGE value is 5</row>
 	<row>Cvar2 PAGE value is 4</row>
-	<row>Cvar2 PAGE value is 7</row>
-	<row>Cvar5 PAGE value is 8</row>
-	<row>Cvar2 PAGE value is 8</row>
-	<row>Cvar5 PAGE value is 7</row>
 	<row>Cvar2 PAGE value is 6</row>
-	<row>Cvar2 PAGE value is 5</row>
-	<row>Cvar5 PAGE value is 6</row>
+	<row>Cvar5 PAGE value is 8</row>
 	<row>Cvar5 PAGE value is 5</row>
+	<row>Cvar5 PAGE value is 6</row>
+	<row>Cvar5 PAGE value is 4</row>
+	<row>Cvar5 PAGE value is 7</row>
+	<row>Cvar2 PAGE value is 8</row>
+	<row>Cvar2 PAGE value is 7</row>
 </result>
\ No newline at end of file
diff --git a/tests/PHPUnit/System/expected/test_AutoSuggestAPITest_customVariableValue__API.getSuggestedValuesForSegment.xml b/tests/PHPUnit/System/expected/test_AutoSuggestAPITest_customVariableValue__API.getSuggestedValuesForSegment.xml
index 662ed617b1..6be328a177 100644
--- a/tests/PHPUnit/System/expected/test_AutoSuggestAPITest_customVariableValue__API.getSuggestedValuesForSegment.xml
+++ b/tests/PHPUnit/System/expected/test_AutoSuggestAPITest_customVariableValue__API.getSuggestedValuesForSegment.xml
@@ -1,21 +1,21 @@
 <?xml version="1.0" encoding="utf-8" ?>
 <result>
-	<row>Cvar5 value is 1</row>
-	<row>Cvar1 value is 0</row>
 	<row>Cvar1 value is 1</row>
+	<row>Cvar1 value is 0</row>
+	<row>Cvar5 value is 1</row>
 	<row>Cvar5 value is 0</row>
 	<row>Cvar1 value is 3</row>
-	<row>Cvar5 value is 3</row>
-	<row>Cvar5 value is 2</row>
 	<row>Cvar1 value is 2</row>
-	<row>Cvar5 value is 4</row>
+	<row>Cvar5 value is 2</row>
+	<row>Cvar5 value is 3</row>
 	<row>Cvar1 value is 4</row>
 	<row>Cvar5 value is 7</row>
-	<row>Cvar1 value is 7</row>
-	<row>Cvar5 value is 8</row>
-	<row>Cvar1 value is 6</row>
+	<row>Cvar1 value is 5</row>
 	<row>Cvar5 value is 6</row>
+	<row>Cvar1 value is 7</row>
 	<row>Cvar5 value is 5</row>
-	<row>Cvar1 value is 5</row>
+	<row>Cvar5 value is 8</row>
 	<row>Cvar1 value is 8</row>
+	<row>Cvar5 value is 4</row>
+	<row>Cvar1 value is 6</row>
 </result>
\ No newline at end of file