Remove urlencode for segment definitions (#17475)

* remove urlencode for segment definitions

* use hash from db in model

8 files changed, 25 insertions, 24 deletions

diff --git a/core/Archive.php b/core/Archive.php
index 8338d19ec3..38f8d63b0d 100644
--- a/core/Archive.php
+++ b/core/Archive.php
@@ -822,7 +822,7 @@ class Archive implements ArchiveQuery
             $this->initializeArchiveIdCache($doneFlag);
 
             $prepareResult = $coreAdminHomeApi->archiveReports(
-                $site->getId(), $period->getLabel(), $periodDateStr, urlencode($this->params->getSegment()->getString()),
+                $site->getId(), $period->getLabel(), $periodDateStr, $this->params->getSegment()->getString(),
                 $plugin, $requestedReport);
 
             if (!empty($prepareResult)
diff --git a/core/CronArchive.php b/core/CronArchive.php
index ddbc74e743..85419be50f 100644
--- a/core/CronArchive.php
+++ b/core/CronArchive.php
@@ -466,9 +466,9 @@ class CronArchive
         $cliMulti->timeRequests();
 
         $responses = $cliMulti->request($urls);
-        
+
         $this->disconnectDb();
-        
+
         $timers = $cliMulti->getTimers();
         $successCount = 0;
 
@@ -890,11 +890,11 @@ class CronArchive
             }
 
             foreach ($this->segmentArchiving->getAllSegmentsToArchive($idSite) as $segmentDefinition) {
-                $params = new Parameters(new Site($idSite), $periodObj, new Segment(urlencode($segmentDefinition), [$idSite], $periodObj->getDateStart(), $periodObj->getDateEnd()));
+                $params = new Parameters(new Site($idSite), $periodObj, new Segment($segmentDefinition, [$idSite], $periodObj->getDateStart(), $periodObj->getDateEnd()));
                 if ($this->canWeSkipInvalidatingBecauseThereIsAUsablePeriod($params, $doNotIncludeTtlInExistingArchiveCheck)) {
                     $this->logger->debug('  Found usable archive for {archive}, skipping invalidation.', ['archive' => $params]);
                 } else {
-                    $this->getApiToInvalidateArchivedReport()->invalidateArchivedReports($idSite, $date, $period, urlencode($segmentDefinition),
+                    $this->getApiToInvalidateArchivedReport()->invalidateArchivedReports($idSite, $date, $period, $segmentDefinition,
                         $cascadeDown = false, $_forceInvalidateNonexistant);
                 }
             }
diff --git a/core/CronArchive/QueueConsumer.php b/core/CronArchive/QueueConsumer.php
index 966e9ea6ca..8e05cc80c8 100644
--- a/core/CronArchive/QueueConsumer.php
+++ b/core/CronArchive/QueueConsumer.php
@@ -575,7 +575,7 @@ class QueueConsumer
         $dateStr = $periodLabel == 'range' ? ($invalidatedArchive['date1'] . ',' . $invalidatedArchive['date2']) : $invalidatedArchive['date1'];
         $period = PeriodFactory::build($periodLabel, $dateStr);
 
-        $segment = new Segment(urlencode($invalidatedArchive['segment']), [$invalidatedArchive['idsite']]);
+        $segment = new Segment($invalidatedArchive['segment'], [$invalidatedArchive['idsite']]);
 
         $params = new Parameters($site, $period, $segment);
 
diff --git a/core/CronArchive/SegmentArchiving.php b/core/CronArchive/SegmentArchiving.php
index 5d0041f54f..664f4d4242 100644
--- a/core/CronArchive/SegmentArchiving.php
+++ b/core/CronArchive/SegmentArchiving.php
@@ -90,7 +90,7 @@ class SegmentArchiving
             }
 
             try {
-                $segmentObj = new Segment(urlencode($segment['definition']), [$idSite]);
+                $segmentObj = new Segment($segment['definition'], [$idSite]);
             } catch (\Exception $ex) {
                 $this->logger->debug("Could not process segment {$segment['definition']} for site {$idSite}. Segment should not exist for the site, but does.");
                 continue;
@@ -236,7 +236,7 @@ class SegmentArchiving
             return;
         }
 
-        $definition = urlencode($segmentInfo['definition']);
+        $definition = $segmentInfo['definition'];
         $idSite = !empty($segmentInfo['enable_only_idsite']) ? $segmentInfo['enable_only_idsite'] : 'all';
 
         $idSites = Access::doAsSuperUser(function () use ($idSite) {
diff --git a/core/DataAccess/Model.php b/core/DataAccess/Model.php
index a04b0db7a7..ca4cdac9c2 100644
--- a/core/DataAccess/Model.php
+++ b/core/DataAccess/Model.php
@@ -585,7 +585,7 @@ class Model
     }
 
     /**
-     * Get a list of IDs of archives with segments that no longer exist in the DB. Excludes temporary archives that 
+     * Get a list of IDs of archives with segments that no longer exist in the DB. Excludes temporary archives that
      * may still be in use, as specified by the $oldestToKeep passed in.
      * @param string $archiveTableName
      * @param array $segments  List of segments to match against
@@ -619,7 +619,7 @@ class Model
     private function getDeletedSegmentWhereClause(array $segment)
     {
         $idSite = (int)$segment['enable_only_idsite'];
-        $segmentHash = Segment::getSegmentHash(urlencode($segment['definition']));
+        $segmentHash = $segment['hash'];
         // Valid segment hashes are md5 strings - just confirm that it is so it's safe for SQL injection
         if (!ctype_xdigit($segmentHash)) {
             throw new Exception($segment . ' expected to be an md5 hash');
@@ -827,7 +827,7 @@ class Model
         $inProgressInvalidation = Db::fetchOne($sql, $bind);
         return $inProgressInvalidation;
     }
-  
+
     /**
      * Returns true if there is an archive that exists that can be used when aggregating an archive for $period.
      *
diff --git a/plugins/SegmentEditor/API.php b/plugins/SegmentEditor/API.php
index 49b436c3d2..2a732a995b 100644
--- a/plugins/SegmentEditor/API.php
+++ b/plugins/SegmentEditor/API.php
@@ -404,7 +404,7 @@ class API extends \Piwik\Plugin\API
         foreach ($segments as &$segmentInfo) {
             $idSites = !empty($segmentInfo['enable_only_idsite']) ? [(int) $segmentInfo['enable_only_idsite']] : $allIdSites;
             try {
-                $segmentObj = new Segment(urlencode($segmentInfo['definition']), $idSites);
+                $segmentObj = new Segment($segmentInfo['definition'], $idSites);
                 $segmentInfo['hash'] = $segmentObj->getHash();
             } catch (\Exception $ex) {
                 $segmentInfo['hash'] = 'INVALID SEGMENT';
diff --git a/tests/PHPUnit/Integration/Archive/ArchivePurgerTest.php b/tests/PHPUnit/Integration/Archive/ArchivePurgerTest.php
index c73feeb7b1..9cc6283498 100644
--- a/tests/PHPUnit/Integration/Archive/ArchivePurgerTest.php
+++ b/tests/PHPUnit/Integration/Archive/ArchivePurgerTest.php
@@ -15,6 +15,7 @@ use Piwik\Db;
 use Piwik\Tests\Fixtures\RawArchiveDataWithTempAndInvalidated;
 use Piwik\Tests\Framework\Fixture;
 use Piwik\Tests\Framework\TestCase\IntegrationTestCase;
+use Piwik\Segment;
 
 /**
  * @group ArchivePurgerTest
@@ -133,12 +134,12 @@ class ArchivePurgerTest extends IntegrationTestCase
         //Extra data set with segment and plugin archives
         self::$fixture->insertSegmentArchives($this->january);
 
-        $segmentsToDelete = array(
-            array('definition' => '9876fedc5432abcd', 'enable_only_idsite' => 0),
-            array('definition' => 'hash3', 'enable_only_idsite' => 0),
+        $segmentsToDelete = [
+            ['definition' => '9876fedc5432abcd', 'enable_only_idsite' => 0, 'hash' => Segment::getSegmentHash('9876fedc5432abcd')],
+            ['definition' => 'hash3', 'enable_only_idsite' => 0, 'hash' => Segment::getSegmentHash('hash3')],
             // This segment also has archives for idsite = 1, which will be retained
-            array('definition' => 'abcd1234abcd5678', 'enable_only_idsite' => 2)
-        );
+            ['definition' => 'abcd1234abcd5678', 'enable_only_idsite' => 2, 'hash' => Segment::getSegmentHash('abcd1234abcd5678')]
+        ];
 
         //Archive #29 also has a deleted segment but it's before the purge threshold so it stays for now.
         $deletedRowCount = $this->archivePurger->purgeDeletedSegmentArchives($this->january, $segmentsToDelete);
@@ -151,10 +152,10 @@ class ArchivePurgerTest extends IntegrationTestCase
         // Extra data set with segment and plugin archives
         self::$fixture->insertSegmentArchives($this->january);
 
-        $segmentsToDelete = array(
+        $segmentsToDelete = [
             // This segment also has archives for idsite = 1, which will be retained
-            array('definition' => 'abcd1234abcd5678', 'enable_only_idsite' => 0, 'idsites_to_preserve' => array(2))
-        );
+            ['definition' => 'abcd1234abcd5678', 'enable_only_idsite' => 0, 'idsites_to_preserve' => [2], 'hash' => Segment::getSegmentHash('abcd1234abcd5678')]
+        ];
 
         // Archives for idsite=1 should be purged, but those for idsite=2 can stay
         $deletedRowCount = $this->archivePurger->purgeDeletedSegmentArchives($this->january, $segmentsToDelete);
diff --git a/tests/PHPUnit/Integration/CronArchiveTest.php b/tests/PHPUnit/Integration/CronArchiveTest.php
index 9ffe5e0b83..643908b973 100644
--- a/tests/PHPUnit/Integration/CronArchiveTest.php
+++ b/tests/PHPUnit/Integration/CronArchiveTest.php
@@ -362,7 +362,7 @@ class CronArchiveTest extends IntegrationTestCase
                         1,
                         '2020-02-03',
                         'day',
-                        'visitCount%3E5',
+                        'visitCount>5',
                         false,
                         false,
                     ),
@@ -370,7 +370,7 @@ class CronArchiveTest extends IntegrationTestCase
                         1,
                         '2020-02-03',
                         'day',
-                        'browserCode%3D%3DIE',
+                        'browserCode==IE',
                         false,
                         false,
                     ),
@@ -392,7 +392,7 @@ class CronArchiveTest extends IntegrationTestCase
                         1,
                         '2020-02-02',
                         'day',
-                        'visitCount%3E5',
+                        'visitCount>5',
                         false,
                         false,
                     ),
@@ -400,7 +400,7 @@ class CronArchiveTest extends IntegrationTestCase
                         1,
                         '2020-02-02',
                         'day',
-                        'browserCode%3D%3DIE',
+                        'browserCode==IE',
                         false,
                         false,
                     ),