Fix Referrers.getKeywordsFromSearchEngineId API for invalid subtable ids (#19823)

* Fix Referrers.getKeywordsFromSearchEngineId API for invalid subtable ids

* Add tests

4 files changed, 100 insertions, 24 deletions

diff --git a/plugins/Referrers/API.php b/plugins/Referrers/API.php
index 995062bfca..2b670f73b8 100644
--- a/plugins/Referrers/API.php
+++ b/plugins/Referrers/API.php
@@ -324,16 +324,21 @@ class API extends \Piwik\Plugin\API
     public function getKeywordsFromSearchEngineId($idSite, $period, $date, $idSubtable, $segment = false)
     {
         Piwik::checkUserHasViewAccess($idSite);
+
         $dataTable = $this->getDataTable(Archiver::SEARCH_ENGINES_RECORD_NAME, $idSite, $period, $date, $segment, $expanded = false, $idSubtable);
 
         // get the search engine and create the URL to the search result page
         $searchEngines = $this->getSearchEngines($idSite, $period, $date, $segment);
         $searchEngines->applyQueuedFilters();
-        $searchEngine  = $searchEngines->getRowFromIdSubDataTable($idSubtable)->getColumn('label');
+        $row  = $searchEngines->getRowFromIdSubDataTable($idSubtable);
 
         $dataTable->filter('Piwik\Plugins\Referrers\DataTable\Filter\KeywordsFromSearchEngineId', array($searchEngines, $idSubtable));
-        $dataTable->filter('AddSegmentByLabel', array('referrerKeyword'));
-        $dataTable->queueFilter('PrependSegment', array('referrerName=='.$searchEngine.';referrerType==search;'));
+        $dataTable->filter('AddSegmentByLabel', ['referrerKeyword']);
+
+        if (!empty($row)) {
+            $searchEngine = $row->getColumn('label');
+            $dataTable->queueFilter('PrependSegment', ['referrerName==' . $searchEngine . ';referrerType==search;']);
+        }
 
         return $dataTable;
     }
diff --git a/plugins/Referrers/tests/System/ApiTest.php b/plugins/Referrers/tests/System/ApiTest.php
index 7d6ba4c4e5..10f99c768c 100644
--- a/plugins/Referrers/tests/System/ApiTest.php
+++ b/plugins/Referrers/tests/System/ApiTest.php
@@ -1,4 +1,5 @@
 <?php
+
 /**
  * Matomo - free/libre analytics platform
  *
@@ -37,24 +38,24 @@ class ApiTest extends SystemTestCase
 
     public function getApiForTesting()
     {
-        $api = array(
+        $api = [
             'API.getProcessedReport'
-        );
+        ];
 
-        $apiToTest   = array();
+        $apiToTest   = [];
 
         // we make sure it returns a subtableIds even if a DataTable\Map is requested
-        $apiToTest[] = array($api,
-            array(
+        $apiToTest[] = [$api,
+            [
                 'idSite'     => 1,
                 'apiModule'  => 'Referrers',
                 'apiAction'  => 'getReferrerType',
                 'date'       => '2010-01-01,2010-03-10',
-                'periods'    => array('day'),
+                'periods'    => ['day'],
                 'testSuffix' => 'Referrers_getReferrerType',
-                'otherRequestParameters' => array('expanded' => 0)
-            )
-        );
+                'otherRequestParameters' => ['expanded' => 0]
+            ]
+        ];
 
         $apiToTest[] = [
             'Referrers.getReferrerType',
@@ -68,7 +69,7 @@ class ApiTest extends SystemTestCase
         ];
 
         $apiToTest[] = [
-            array('Referrers.getAll', 'Referrers.getReferrerType'),
+            ['Referrers.getAll', 'Referrers.getReferrerType'],
             [
                 'idSite' => 'all',
                 'date' => '2010-01-01',
@@ -77,6 +78,32 @@ class ApiTest extends SystemTestCase
             ],
         ];
 
+        $apiToTest[] = [
+            ['Referrers.getKeywordsFromSearchEngineId'],
+            [
+                'idSite' => '1',
+                'date' => '2010-01-01',
+                'periods' => 'year',
+                'otherRequestParameters' => [
+                    'idSubtable' => '1',
+                ],
+                'testSuffix' => 'subtableid_valid',
+            ],
+        ];
+
+        $apiToTest[] = [
+            ['Referrers.getKeywordsFromSearchEngineId'],
+            [
+                'idSite' => '1',
+                'date' => '2010-01-01',
+                'periods' => 'year',
+                'otherRequestParameters' => [
+                    'idSubtable' => '99',
+                ],
+                'testSuffix' => 'subtableid_invalid',
+            ],
+        ];
+
         return $apiToTest;
     }
 
@@ -99,7 +126,7 @@ class ApiTest extends SystemTestCase
         $t->doTrackPageView('Page 1');
 
         /** @var DataTable $visits */
-        $visits = Request::processRequest('VisitsSummary.get', array('idSite' => 1, 'period' => 'day', 'date' => $dateTime));
+        $visits = Request::processRequest('VisitsSummary.get', ['idSite' => 1, 'period' => 'day', 'date' => $dateTime]);
 
         $this->assertEquals(1, $visits->getFirstRow()->getColumn('nb_visits'));
         $this->assertEquals(2, $visits->getFirstRow()->getColumn('nb_actions'));
@@ -124,7 +151,7 @@ class ApiTest extends SystemTestCase
         $t->doTrackPageView('Page 1');
 
         /** @var DataTable $visits */
-        $visits = Request::processRequest('VisitsSummary.get', array('idSite' => 1, 'period' => 'day', 'date' => $dateTime));
+        $visits = Request::processRequest('VisitsSummary.get', ['idSite' => 1, 'period' => 'day', 'date' => $dateTime]);
 
         $this->assertEquals(1, $visits->getFirstRow()->getColumn('nb_visits'));
         $this->assertEquals(2, $visits->getFirstRow()->getColumn('nb_actions'));
@@ -149,7 +176,7 @@ class ApiTest extends SystemTestCase
         $t->doTrackPageView('Page 1');
 
         /** @var DataTable $visits */
-        $visits = Request::processRequest('VisitsSummary.get', array('idSite' => 1, 'period' => 'day', 'date' => $dateTime));
+        $visits = Request::processRequest('VisitsSummary.get', ['idSite' => 1, 'period' => 'day', 'date' => $dateTime]);
 
         $this->assertEquals(1, $visits->getFirstRow()->getColumn('nb_visits'));
         $this->assertEquals(2, $visits->getFirstRow()->getColumn('nb_actions'));
@@ -177,13 +204,13 @@ class ApiTest extends SystemTestCase
         $t->doTrackPageView('Page 1');
 
         /** @var DataTable $visits */
-        $visits = Request::processRequest('VisitsSummary.get', array('idSite' => 1, 'period' => 'day', 'date' => $dateTime));
+        $visits = Request::processRequest('VisitsSummary.get', ['idSite' => 1, 'period' => 'day', 'date' => $dateTime]);
 
         $this->assertEquals(1, $visits->getFirstRow()->getColumn('nb_visits'));
         $this->assertEquals(2, $visits->getFirstRow()->getColumn('nb_actions'));
 
         /** @var DataTable $referrers */
-        $referrers = Request::processRequest('Referrers.getCampaigns', array('idSite' => 1, 'period' => 'day', 'date' => $dateTime));
+        $referrers = Request::processRequest('Referrers.getCampaigns', ['idSite' => 1, 'period' => 'day', 'date' => $dateTime]);
         $this->assertEquals(substr($longReferrer, 0, 255), $referrers->getFirstRow()->getColumn('label'));
     }
 
@@ -207,7 +234,7 @@ class ApiTest extends SystemTestCase
         $t->doTrackPageView('Page 1');
 
         /** @var DataTable $visits */
-        $visits = Request::processRequest('VisitsSummary.get', array('idSite' => 1, 'period' => 'day', 'date' => $dateTime));
+        $visits = Request::processRequest('VisitsSummary.get', ['idSite' => 1, 'period' => 'day', 'date' => $dateTime]);
 
         $this->assertEquals(1, $visits->getFirstRow()->getColumn('nb_visits'));
         $this->assertEquals(2, $visits->getFirstRow()->getColumn('nb_actions'));
@@ -233,7 +260,7 @@ class ApiTest extends SystemTestCase
         /** @var DataTable $visits */
         $visits = Request::processRequest(
             'Referrers.getWebsites',
-            array('idSite' => $idSite, 'period' => 'day', 'date' => $dateTime, 'flat' => 1)
+            ['idSite' => $idSite, 'period' => 'day', 'date' => $dateTime, 'flat' => 1]
         );
 
         $firstRow = $visits->getFirstRow();
@@ -261,14 +288,14 @@ class ApiTest extends SystemTestCase
         /** @var DataTable $visits */
         $visits = Request::processRequest(
             'Referrers.getWebsites',
-            array('idSite' => $idSite, 'period' => 'day', 'date' => $dateTime)
+            ['idSite' => $idSite, 'period' => 'day', 'date' => $dateTime]
         );
 
         $idSubtable = $visits->getFirstRow()->getIdSubDataTable();
 
         $visits = Request::processRequest(
             'Referrers.getUrlsFromWebsiteId',
-            array('idSite' => $idSite, 'period' => 'day', 'date' => $dateTime, 'idSubtable' => $idSubtable)
+            ['idSite' => $idSite, 'period' => 'day', 'date' => $dateTime, 'idSubtable' => $idSubtable]
         );
 
         $firstRow = $visits->getFirstRow();
@@ -288,7 +315,7 @@ class ApiTest extends SystemTestCase
         $t->doTrackPageView('Page 1');
 
         /** @var DataTable $visits */
-        $visits = Request::processRequest('Referrers.getSearchEngines', array('idSite' => 1, 'period' => 'day', 'date' => $dateTime));
+        $visits = Request::processRequest('Referrers.getSearchEngines', ['idSite' => 1, 'period' => 'day', 'date' => $dateTime]);
 
         $this->assertEquals('Looksmart', $visits->getFirstRow()->getColumn('label'));
         $this->assertEquals(1, $visits->getFirstRow()->getColumn('nb_visits'));
@@ -315,4 +342,4 @@ class ApiTest extends SystemTestCase
     }
 }
 
-ApiTest::$fixture = new TwoSitesManyVisitsOverSeveralDaysWithSearchEngineReferrers();
\ No newline at end of file
+ApiTest::$fixture = new TwoSitesManyVisitsOverSeveralDaysWithSearchEngineReferrers();
diff --git a/plugins/Referrers/tests/System/expected/test_subtableid_invalid__Referrers.getKeywordsFromSearchEngineId_year.xml b/plugins/Referrers/tests/System/expected/test_subtableid_invalid__Referrers.getKeywordsFromSearchEngineId_year.xml
new file mode 100644
index 0000000000..c234bed59e
--- /dev/null
+++ b/plugins/Referrers/tests/System/expected/test_subtableid_invalid__Referrers.getKeywordsFromSearchEngineId_year.xml
@@ -0,0 +1,2 @@
+<?xml version="1.0" encoding="utf-8" ?>
+<result />
\ No newline at end of file
diff --git a/plugins/Referrers/tests/System/expected/test_subtableid_valid__Referrers.getKeywordsFromSearchEngineId_year.xml b/plugins/Referrers/tests/System/expected/test_subtableid_valid__Referrers.getKeywordsFromSearchEngineId_year.xml
new file mode 100644
index 0000000000..280be5979c
--- /dev/null
+++ b/plugins/Referrers/tests/System/expected/test_subtableid_valid__Referrers.getKeywordsFromSearchEngineId_year.xml
@@ -0,0 +1,42 @@
+<?xml version="1.0" encoding="utf-8" ?>
+<result>
+	<row>
+		<label>free &gt; proprietary</label>
+		<nb_visits>11</nb_visits>
+		<nb_actions>11</nb_actions>
+		<max_actions>1</max_actions>
+		<sum_visit_length>0</sum_visit_length>
+		<bounce_count>11</bounce_count>
+		<nb_visits_converted>0</nb_visits_converted>
+		<sum_daily_nb_uniq_visitors>11</sum_daily_nb_uniq_visitors>
+		<sum_daily_nb_users>0</sum_daily_nb_users>
+		<segment>referrerName==Google;referrerType==search;referrerKeyword==free+%3E+proprietary</segment>
+		<url>http://google.com/search?q=free+%3E+proprietary</url>
+	</row>
+	<row>
+		<label>justice )(&amp;^#%$ not '&quot; corruption!</label>
+		<nb_visits>10</nb_visits>
+		<nb_actions>10</nb_actions>
+		<max_actions>1</max_actions>
+		<sum_visit_length>0</sum_visit_length>
+		<bounce_count>10</bounce_count>
+		<nb_visits_converted>0</nb_visits_converted>
+		<sum_daily_nb_uniq_visitors>10</sum_daily_nb_uniq_visitors>
+		<sum_daily_nb_users>0</sum_daily_nb_users>
+		<segment>referrerName==Google;referrerType==search;referrerKeyword==justice+%29%28%26%5E%23%25%24+not+%27%22+corruption%21</segment>
+		<url>http://google.com/search?q=justice+%29%28%26%5E%23%25%24+not+%27%22+corruption%21</url>
+	</row>
+	<row>
+		<label>peace &quot;,&quot; not war</label>
+		<nb_visits>10</nb_visits>
+		<nb_actions>10</nb_actions>
+		<max_actions>1</max_actions>
+		<sum_visit_length>0</sum_visit_length>
+		<bounce_count>10</bounce_count>
+		<nb_visits_converted>0</nb_visits_converted>
+		<sum_daily_nb_uniq_visitors>10</sum_daily_nb_uniq_visitors>
+		<sum_daily_nb_users>0</sum_daily_nb_users>
+		<segment>referrerName==Google;referrerType==search;referrerKeyword==peace+%22%2C%22+not+war</segment>
+		<url>http://google.com/search?q=peace+%22%2C%22+not+war</url>
+	</row>
+</result>
\ No newline at end of file