diff options
author | Zoltan Flamis <zoltan@innocraft.com> | 2021-04-21 22:49:07 +0300 |
---|---|---|
committer | GitHub <noreply@github.com> | 2021-04-21 22:49:07 +0300 |
commit | 647ed543aecd9b1d92ecfb95acd5f9ac1c7d58f7 (patch) | |
tree | aa3fe46f10b6f049e5c52d069719bd012cf556e3 | |
parent | 49f0499b480f96cd5e3a9e2abfd286c3c2d1d690 (diff) |
Remove urlencode for segment definitions (#17475)
* remove urlencode for segment definitions
* use hash from db in model
-rw-r--r-- | core/Archive.php | 2 | ||||
-rw-r--r-- | core/CronArchive.php | 8 | ||||
-rw-r--r-- | core/CronArchive/QueueConsumer.php | 2 | ||||
-rw-r--r-- | core/CronArchive/SegmentArchiving.php | 4 | ||||
-rw-r--r-- | core/DataAccess/Model.php | 6 | ||||
-rw-r--r-- | plugins/SegmentEditor/API.php | 2 | ||||
-rw-r--r-- | tests/PHPUnit/Integration/Archive/ArchivePurgerTest.php | 17 | ||||
-rw-r--r-- | tests/PHPUnit/Integration/CronArchiveTest.php | 8 |
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, ), |