Add exception for invalid limit before grouping (#19881)

* add exception for invalid limit before grouping

add exception for invalid limit before grouping

* update int to numeric

update int to numeric

* update getTransitionsForAction limitBeforeGrouping default to int

update getTransitionsForAction limitBeforeGrouping default to int

* Update test name

* remove translation

remove translation

* update all other place from false to 0

update all other place from false to 0

* fix other places using limitBeforeGrouping is false

* update ranking Query from false to 0

update ranking Query from false to 0

* add more tests

add more tests

* update tests

update tests

* remove throw errors

remove throw errors

* remove exception for other places

remove exception for other places

* add throw and tests back

add throw and tests back

* update test case

* update intval to int and false condition

update intval to int and false condition

* revert ranking query from false to 0

revert ranking query from false to 0

Co-authored-by: Ben <ben.burgess@innocraft.com>

5 files changed, 49 insertions, 18 deletions

diff --git a/core/RankingQuery.php b/core/RankingQuery.php
index 162b24cd19..0ee5ee5b54 100644
--- a/core/RankingQuery.php
+++ b/core/RankingQuery.php
@@ -96,7 +96,7 @@ class RankingQuery
      */
     public function __construct($limit = false)
     {
-        if ($limit !== false) {
+        if ($limit !==false) {
             $this->setLimit($limit);
         }
     }
diff --git a/plugins/Transitions/API.php b/plugins/Transitions/API.php
index d082013af6..dc419858e7 100644
--- a/plugins/Transitions/API.php
+++ b/plugins/Transitions/API.php
@@ -36,12 +36,12 @@ use Piwik\Tracker\TableLogAction;
  */
 class API extends \Piwik\Plugin\API
 {
-    public function getTransitionsForPageTitle($pageTitle, $idSite, $period, $date, $segment = false, $limitBeforeGrouping = false)
+    public function getTransitionsForPageTitle($pageTitle, $idSite, $period, $date, $segment = false, $limitBeforeGrouping = 0)
     {
         return $this->getTransitionsForAction($pageTitle, 'title', $idSite, $period, $date, $segment, $limitBeforeGrouping);
     }
 
-    public function getTransitionsForPageUrl($pageUrl, $idSite, $period, $date, $segment = false, $limitBeforeGrouping = false)
+    public function getTransitionsForPageUrl($pageUrl, $idSite, $period, $date, $segment = false, $limitBeforeGrouping = 0)
     {
         return $this->getTransitionsForAction($pageUrl, 'url', $idSite, $period, $date, $segment, $limitBeforeGrouping);
     }
@@ -55,13 +55,13 @@ class API extends \Piwik\Plugin\API
      * @param $period
      * @param $date
      * @param bool $segment
-     * @param bool $limitBeforeGrouping
+     * @param int $limitBeforeGrouping
      * @param string $parts
      * @return array
      * @throws Exception
      */
     public function getTransitionsForAction($actionName, $actionType, $idSite, $period, $date,
-                                            $segment = false, $limitBeforeGrouping = false, $parts = 'all')
+                                            $segment = false, $limitBeforeGrouping = 0, $parts = 'all')
     {
         Piwik::checkUserHasViewAccess($idSite);
 
@@ -69,6 +69,12 @@ class API extends \Piwik\Plugin\API
             throw new Exception('PeriodNotAllowed');
         }
 
+        if ($limitBeforeGrouping && !is_numeric($limitBeforeGrouping)) {
+            throw new Exception('limitBeforeGrouping has to be an integer.');
+        }
+        //convert string to int
+        $limitBeforeGrouping = (int)$limitBeforeGrouping;
+
         // get idaction of the requested action
         $idaction = $this->deriveIdAction($actionName, $actionType);
         if ($idaction < 0) {
@@ -212,11 +218,12 @@ class API extends \Piwik\Plugin\API
      * @param $report
      * @param $idaction
      * @param string $actionType
-     * @param $limitBeforeGrouping
+     * @param int $limitBeforeGrouping
      * @param boolean $includeLoops
      */
-    private function addFollowingActions($logAggregator, &$report, $idaction, $actionType, $limitBeforeGrouping, $includeLoops = false)
+    private function addFollowingActions($logAggregator, &$report, $idaction, $actionType, $limitBeforeGrouping = 0, $includeLoops = false)
     {
+
         $data = $this->queryFollowingActions(
             $idaction, $actionType, $logAggregator, $limitBeforeGrouping, $includeLoops);
 
@@ -231,12 +238,12 @@ class API extends \Piwik\Plugin\API
      * @param $idaction
      * @param $actionType
      * @param LogAggregator $logAggregator
-     * @param $limitBeforeGrouping
+     * @param  $limitBeforeGrouping
      * @param $includeLoops
      * @return array(followingPages:DataTable, outlinks:DataTable, downloads:DataTable)
      */
     protected function queryFollowingActions($idaction, $actionType, LogAggregator $logAggregator,
-                                          $limitBeforeGrouping = false, $includeLoops = false)
+                                          $limitBeforeGrouping = 0, $includeLoops = false)
     {
         $types = array();
 
@@ -291,7 +298,7 @@ class API extends \Piwik\Plugin\API
         $types[Action::TYPE_OUTLINK] = 'outlinks';
         $types[Action::TYPE_DOWNLOAD] = 'downloads';
 
-        $rankingQuery = new RankingQuery($limitBeforeGrouping ? $limitBeforeGrouping : $this->limitBeforeGrouping);
+        $rankingQuery = new RankingQuery($limitBeforeGrouping ? $limitBeforeGrouping: $this->limitBeforeGrouping);
         $rankingQuery->setOthersLabel('Others');
         $rankingQuery->addLabelColumn(array('name', 'url_prefix'));
         $rankingQuery->partitionResultIntoMultipleGroups('type', array_keys($types));
@@ -326,12 +333,14 @@ class API extends \Piwik\Plugin\API
      * @param $idaction
      * @param $actionType
      * @param LogAggregator $logAggregator
-     * @param $limitBeforeGrouping
+     * @param int $limitBeforeGrouping
      * @return DataTable
+     * @throws Exception
      */
-    protected function queryExternalReferrers($idaction, $actionType, $logAggregator, $limitBeforeGrouping = false)
+    protected function queryExternalReferrers($idaction, $actionType, $logAggregator, $limitBeforeGrouping = 0)
     {
-        $rankingQuery = new RankingQuery($limitBeforeGrouping ? $limitBeforeGrouping : $this->limitBeforeGrouping);
+
+        $rankingQuery = new RankingQuery($limitBeforeGrouping ? $limitBeforeGrouping: $this->limitBeforeGrouping);
         $rankingQuery->setOthersLabel('Others');
 
         // we generate a single column that contains the interesting data for each referrer.
@@ -401,16 +410,16 @@ class API extends \Piwik\Plugin\API
      * @param $idaction
      * @param $actionType
      * @param LogAggregator $logAggregator
-     * @param $limitBeforeGrouping
+     * @param int $limitBeforeGrouping
      * @return array(previousPages:DataTable, loops:integer)
      */
-    protected function queryInternalReferrers($idaction, $actionType, $logAggregator, $limitBeforeGrouping = false)
+    protected function queryInternalReferrers($idaction, $actionType, $logAggregator, $limitBeforeGrouping = 0)
     {
         $keyIsOther = 0;
         $keyIsPageUrlAction = 1;
         $keyIsSiteSearchAction = 2;
 
-        $rankingQuery = new RankingQuery($limitBeforeGrouping ? $limitBeforeGrouping : $this->limitBeforeGrouping);
+        $rankingQuery = new RankingQuery($limitBeforeGrouping ? $limitBeforeGrouping: $this->limitBeforeGrouping);
         $rankingQuery->setOthersLabel('Others');
         $rankingQuery->addLabelColumn(array('name', 'url_prefix'));
         $rankingQuery->setColumnToMarkExcludedRows('is_self');
diff --git a/plugins/Transitions/tests/Integration/TransitionsMaxAllowedPeriodTest.php b/plugins/Transitions/tests/Integration/TransitionsMaxAllowedPeriodTest.php
index b074e7d3ab..be39a0629f 100644
--- a/plugins/Transitions/tests/Integration/TransitionsMaxAllowedPeriodTest.php
+++ b/plugins/Transitions/tests/Integration/TransitionsMaxAllowedPeriodTest.php
@@ -82,6 +82,21 @@ class TransitionsMaxAllowedPeriodTest extends IntegrationTestCase
         }
     }
 
+    public function test_ShouldThrowException_IfInvalidLimitBeforeGroup()
+    {
+        $this->expectException(\Exception::class);
+        $this->expectExceptionMessage('limitBeforeGrouping has to be an integer.');
+        $this->api->getTransitionsForAction('http://example.org/page/one.html', 'url', 1,
+            'range', '2012-08-09,2012-08-10', false, 'all');
+    }
+
+    public function test_ShouldPass_IfLimitBeforeGroupPassingIntAsString()
+    {
+        $report = $this->api->getTransitionsForAction('http://example.org/page/one.html', 'url', 1,
+            'range', '2012-08-09,2012-08-10', false, '100');
+        $this->assertIsArray($report);
+    }
+
     public function test_ShouldThrowException_IfRangeDayCountIsLargerThanDayPeriod()
     {
         Config::setSetting('Transitions_1', 'max_period_allowed', 'day');
diff --git a/plugins/Transitions/tests/System/TransitionsTest.php b/plugins/Transitions/tests/System/TransitionsTest.php
index c5adc135c0..7329d917fb 100644
--- a/plugins/Transitions/tests/System/TransitionsTest.php
+++ b/plugins/Transitions/tests/System/TransitionsTest.php
@@ -17,7 +17,7 @@ use Piwik\Tests\Fixtures\SomeVisitsManyPageviewsWithTransitions;
  * @group Plugins
  */
 class TransitionsTest extends SystemTestCase
-{   
+{
     public static $fixture = null; // initialized below class definition
 
     /**
@@ -114,4 +114,4 @@ class TransitionsTest extends SystemTestCase
     }
 }
 
-TransitionsTest::$fixture = new SomeVisitsManyPageviewsWithTransitions();
\ No newline at end of file
+TransitionsTest::$fixture = new SomeVisitsManyPageviewsWithTransitions();
diff --git a/tests/PHPUnit/Unit/RankingQueryTest.php b/tests/PHPUnit/Unit/RankingQueryTest.php
index f170bacd9f..4320055986 100644
--- a/tests/PHPUnit/Unit/RankingQueryTest.php
+++ b/tests/PHPUnit/Unit/RankingQueryTest.php
@@ -58,6 +58,7 @@ class RankingQueryTest extends \PHPUnit\Framework\TestCase
      */
     public function testExcludeRows()
     {
+
         $query = new RankingQuery(20);
         $query->setOthersLabel('Others');
         $query->addLabelColumn('label');
@@ -89,6 +90,12 @@ class RankingQueryTest extends \PHPUnit\Framework\TestCase
 		";
 
         $this->checkQuery($query, $innerQuery, $expected);
+
+        $query = new RankingQuery('20');
+        $query->setOthersLabel('Others');
+        $query->addLabelColumn('label');
+        $query->setColumnToMarkExcludedRows('exclude_marker');
+        $this->checkQuery($query, $innerQuery, $expected);
     }
 
     /**