modify SegmentQueryDecorator so extra comment information can be saved (#17337)

* modify SegmentQueryDecorator so extra comment information can be saved

* Update LogAggregator.php

* fix tests

5 files changed, 34 insertions, 20 deletions

diff --git a/core/DataAccess/LogAggregator.php b/core/DataAccess/LogAggregator.php
index 159fdd5b89..e2a19753e9 100644
--- a/core/DataAccess/LogAggregator.php
+++ b/core/DataAccess/LogAggregator.php
@@ -277,7 +277,7 @@ class LogAggregator
         if ($this->doesSegmentTableExist($table)) {
             return; // no need to create the table, it was already created... better to have a select vs unneeded create table
         }
-	    
+
         $engine = '';
         if (defined('PIWIK_TEST_MODE') && PIWIK_TEST_MODE) {
             $engine = 'ENGINE=MEMORY';
@@ -403,11 +403,21 @@ class LogAggregator
             $query['sql'] = 'SELECT /* ' . $this->queryOriginHint . ' */' . substr($query['sql'], strlen($select));
         }
 
+        if (0 === strpos(trim($query['sql']), $select)) {
+            $query['sql'] = trim($query['sql']);
+            $query['sql'] = 'SELECT /* ' . $this->dateStart->toString() . ',' . $this->dateEnd->toString() . ' */' . substr($query['sql'], strlen($select));
+        }
+
+        if ($this->sites && 0 === strpos(trim($query['sql']), $select)) {
+            $query['sql'] = trim($query['sql']);
+            $query['sql'] = 'SELECT /* ' . 'sites ' . implode(',', array_map('intval', $this->sites)) . ' */' . substr($query['sql'], strlen($select));
+        }
+
         if (!$this->getSegment()->isEmpty() && is_array($query) && 0 === strpos(trim($query['sql']), $select)) {
             $query['sql'] = trim($query['sql']);
-            $query['sql'] = 'SELECT /* ' . $this->dateStart->toString() . ',' . $this->dateEnd->toString() . ' sites ' . implode(',', array_map('intval', $this->sites)) . ' segmenthash ' . $this->getSegment()->getHash(). ' */' . substr($query['sql'], strlen($select));
+            $query['sql'] = 'SELECT /* ' . 'segmenthash ' . $this->getSegment()->getHash(). ' */' . substr($query['sql'], strlen($select));
         }
- 
+
         return $query;
     }
 
diff --git a/plugins/SegmentEditor/SegmentQueryDecorator.php b/plugins/SegmentEditor/SegmentQueryDecorator.php
index e268c6febf..cbf4967b8f 100644
--- a/plugins/SegmentEditor/SegmentQueryDecorator.php
+++ b/plugins/SegmentEditor/SegmentQueryDecorator.php
@@ -50,8 +50,10 @@ class SegmentQueryDecorator extends LogQueryBuilder
             $prefixParts[] = "idSegments = [" . implode(', ', $idSegments) . "]";
         }
 
-        if (!empty($prefixParts)) {
-            $result['sql'] = "/* " . implode(', ', $prefixParts) . " */\n" . $result['sql'];
+        $select = 'SELECT';
+        if (!empty($prefixParts) && 0 === strpos(trim($result['sql']), $select)) {
+            $result['sql'] = trim($result['sql']);
+            $result['sql'] = 'SELECT /* ' . implode(', ', $prefixParts) . ' */' . substr($result['sql'], strlen($select));
         }
 
         return $result;
diff --git a/plugins/SegmentEditor/tests/Integration/SegmentQueryDecoratorTest.php b/plugins/SegmentEditor/tests/Integration/SegmentQueryDecoratorTest.php
index 7cf0c46de3..7cc9158cd0 100644
--- a/plugins/SegmentEditor/tests/Integration/SegmentQueryDecoratorTest.php
+++ b/plugins/SegmentEditor/tests/Integration/SegmentQueryDecoratorTest.php
@@ -72,22 +72,23 @@ class SegmentQueryDecoratorTest extends IntegrationTestCase
             $this->assertStringStartsNotWith("/* idSegments", $sql);
         } else {
             $this->assertStringStartsWith($expectedPrefix, $sql);
+            $this->assertEquals(1, substr_count($sql, 'SELECT'));
         }
     }
 
     public function getTestDataForSegmentSqlTest()
     {
         return array(
-            array('countryCode==fr', null, '/* idSegments = [2] */'),
-            array('visitCount<2', null, '/* idSegments = [1, 3, 4, 5] */'),
+            array('countryCode==fr', null, 'SELECT /* idSegments = [2] */'),
+            array('visitCount<2', null, 'SELECT /* idSegments = [1, 3, 4, 5] */'),
             array('', null, null),
-            array('countryCode!=fr', null, '/* idSegments = [6] */'),
+            array('countryCode!=fr', null, 'SELECT /* idSegments = [6] */'),
 
-            array('', 'archivephp', '/* trigger = CronArchive */'),
-            array('countryCode!=fr', 'archivephp', '/* trigger = CronArchive, idSegments = [6] */'),
+            array('', 'archivephp', 'SELECT /* trigger = CronArchive */'),
+            array('countryCode!=fr', 'archivephp', 'SELECT /* trigger = CronArchive, idSegments = [6] */'),
 
             array('', 'garbage', null),
-            array('countryCode!=fr', 'garbage', '/* idSegments = [6] */'),
+            array('countryCode!=fr', 'garbage', 'SELECT /* idSegments = [6] */'),
         );
     }
-}
\ No newline at end of file
+}
diff --git a/plugins/SegmentEditor/tests/Unit/SegmentQueryDecoratorTest.php b/plugins/SegmentEditor/tests/Unit/SegmentQueryDecoratorTest.php
index 1f7b6a29c1..593bac5bed 100644
--- a/plugins/SegmentEditor/tests/Unit/SegmentQueryDecoratorTest.php
+++ b/plugins/SegmentEditor/tests/Unit/SegmentQueryDecoratorTest.php
@@ -68,7 +68,8 @@ class SegmentQueryDecoratorTest extends \PHPUnit\Framework\TestCase
 
         $query = $this->decorator->getSelectQueryString($expression, '*', 'log_visit', '', array(), '', '', '');
 
-        $this->assertStringStartsWith('/* idSegments = [3] */', $query['sql']);
+        $this->assertStringStartsWith('SELECT /* idSegments = [3] */', $query['sql']);
+        $this->assertEquals(1, substr_count($query['sql'], 'SELECT'));
     }
 
     public function test_getSelectQueryString_DecoratesSql_WhenMultipleStoredSegmentsMatchUsedSegment()
@@ -78,7 +79,8 @@ class SegmentQueryDecoratorTest extends \PHPUnit\Framework\TestCase
 
         $query = $this->decorator->getSelectQueryString($expression, '*', 'log_visit', '', array(), '', '', '');
 
-        $this->assertStringStartsWith('/* idSegments = [2, 4] */', $query['sql']);
+        $this->assertStringStartsWith('SELECT /* idSegments = [2, 4] */', $query['sql']);
+        $this->assertEquals(1, substr_count($query['sql'], 'SELECT'));
     }
 
     private function getMockSegmentEditorService()
diff --git a/tests/PHPUnit/Integration/DataAccess/LogAggregatorTest.php b/tests/PHPUnit/Integration/DataAccess/LogAggregatorTest.php
index ff2b33906b..1929352479 100644
--- a/tests/PHPUnit/Integration/DataAccess/LogAggregatorTest.php
+++ b/tests/PHPUnit/Integration/DataAccess/LogAggregatorTest.php
@@ -73,8 +73,7 @@ class LogAggregatorTest extends IntegrationTestCase
         $query = $this->logAggregator->generateQuery('test, test2', 'log_visit', '1=1', false, '5');
 
         $expected = array(
-            'sql' => '
-			SELECT
+            'sql' => 'SELECT /* sites 1 */ /* 2010-03-01,2010-03-31 */
 				test, test2
 			FROM
 				log_visit AS log_visit
@@ -101,7 +100,7 @@ class LogAggregatorTest extends IntegrationTestCase
         $query = $this->logAggregator->generateQuery('test, test2', 'log_visit', '1=1', false, '5');
 
         $expected = array(
-            'sql' => 'SELECT /* 2010-03-01,2010-03-31 sites 1 segmenthash 4eaf469650796451c610972d0ca1e9e8 */
+            'sql' => 'SELECT /* segmenthash 4eaf469650796451c610972d0ca1e9e8 */ /* sites 1 */ /* 2010-03-01,2010-03-31 */
 				test, test2
 			FROM
 				log_visit AS log_visit
@@ -132,7 +131,7 @@ class LogAggregatorTest extends IntegrationTestCase
         $query = $this->logAggregator->generateQuery('test, test2', 'log_visit', '1=1', false, '5');
 
         $expected = array(
-            'sql' => 'SELECT /* 2010-03-01,2010-03-31 sites 1 segmenthash 4eaf469650796451c610972d0ca1e9e8 */
+            'sql' => 'SELECT /* segmenthash 4eaf469650796451c610972d0ca1e9e8 */ /* sites 1 */ /* 2010-03-01,2010-03-31 */
 				test, test2
 			FROM
 				logtmpsegment0e053be69df974017fba4276a0d4347d AS logtmpsegment0e053be69df974017fba4276a0d4347d INNER JOIN log_visit AS log_visit ON log_visit.idvisit = logtmpsegment0e053be69df974017fba4276a0d4347d.idvisit
@@ -203,7 +202,7 @@ class LogAggregatorTest extends IntegrationTestCase
 
         $this->setSqlRequirePrimaryKeySetting(0);// reset variable
         $expected = array(
-            'sql' => 'SELECT /* 2010-03-01,2010-03-31 sites 1 segmenthash 4a4d16d6897e7fed2d5d151016a5a19c */
+            'sql' => 'SELECT /* segmenthash 4a4d16d6897e7fed2d5d151016a5a19c */ /* sites 1 */ /* 2010-03-01,2010-03-31 */
 				test, test2
 			FROM
 				logtmpsegment4ef74412006a3160b17ca5fe99a5f866 AS logtmpsegment4ef74412006a3160b17ca5fe99a5f866 INNER JOIN log_visit AS log_visit ON log_visit.idvisit = logtmpsegment4ef74412006a3160b17ca5fe99a5f866.idvisit
@@ -237,7 +236,7 @@ class LogAggregatorTest extends IntegrationTestCase
         $query = $this->logAggregator->generateQuery('test, test2', 'log_visit', '1=1', false, '5');
 
         $expected = array(
-            'sql' => 'SELECT /* MyPluginName */
+            'sql' => 'SELECT /* sites 1 */ /* 2010-03-01,2010-03-31 */ /* MyPluginName */
 				test, test2
 			FROM
 				log_visit AS log_visit