Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

strtolower(): Argument #1 ($string) must be of type string, array given in Date.php #18639

Closed
tsteur opened this issue Jan 18, 2022 · 6 comments · Fixed by #18653 or #18686
Closed

strtolower(): Argument #1 ($string) must be of type string, array given in Date.php #18639

tsteur opened this issue Jan 18, 2022 · 6 comments · Fixed by #18653 or #18686
Assignees
Labels
Bug For errors / faults / flaws / inconsistencies etc. Help wanted Beginner friendly issues or issues where we'd highly appreciate community's help and involvement. not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Milestone

Comments

@tsteur
Copy link
Member

tsteur commented Jan 18, 2022

First problem:

Error: {"message":"strtolower(): Argument #1 ($string) must be of type string, array given","file":"/core/Date.php","line":1079,"request_id":"ba369","backtrace":" on /core/Date.php(1079)\n#0 /core/Date.php(1079): strtolower(Array)\n#1 /core/Period/Range.php(255): Piwik\Date->addPeriod(-29, Array)\n#2 /core/Period.php(131): Piwik\Period\Range->generate()\n#3 /core/Period/Range.php(152): Piwik\Period->getDateStart()\n#4 /core/Period/Range.php(533): Piwik\Period\Range->getDateStart()\n#5 /core/Plugin/Controller.php(502): Piwik\Period\Range::getRelativeToEndDate(Array, 'last30', Object(Piwik\Date), Object(Piwik\Site))\n#6 /plugins/MultiSites/Controller.php(97): Piwik\Plugin\Controller->getGraphParamsModified()\n#7 /plugins/MultiSites/Controller.php(37): Piwik\Plugins\MultiSites\Controller->getSitesInfo(false)\n#8 [internal function]: Piwik\Plugins\MultiSites\Controller->index()\n#9 /core/FrontController.php(619): call_user_func_array(Array, Array)\n#10 /core/FrontController.php(168): Piwik\FrontController->doDispatch('MultiSites', 'index', Array)\n#11 /core/dispatch.php(32): Piwik\FrontController->dispatch()\n#12

Happens on

  • /index.php?action=index&date=yesterday&idSite=1&module=MultiSites&period[]=day

Second problem

Error: {"message":"urldecode(): Argument #1 ($string) must be of type string, array given","file":"/core/Date.php","line":138,"request_id":"83d50","backtrace":" on /core/Date.php(138)\n#0 /core/Date.php(138): urldecode(Array)\n#1 /core/Period.php(121): Piwik\Date::factory(Array)\n#2 /core/Plugin/Controller.php(644): Piwik\Period::checkDateFormat(Array)\n#3 /core/Plugin/Controller.php(616): Piwik\Plugin\Controller->setGeneralVariablesViewAs(Object(Piwik\View), 'basic')\n#4 /plugins/MultiSites/Controller.php(100): Piwik\Plugin\Controller->setGeneralVariablesView(Object(Piwik\View))\n#5 /plugins/MultiSites/Controller.php(37): Piwik\Plugins\MultiSites\Controller->getSitesInfo(false)\n#6 [internal function]: Piwik\Plugins\MultiSites\Controller->index()\n#7 /core/FrontController.php(619):

Happens on

  • index.php?action=index&date[]=yesterday&idSite=1&module=MultiSites&period=day

Expected outcome

No fatal error

@tsteur tsteur added Bug For errors / faults / flaws / inconsistencies etc. Help wanted Beginner friendly issues or issues where we'd highly appreciate community's help and involvement. labels Jan 18, 2022
@tsteur tsteur added this to the 4.8.0 milestone Jan 18, 2022
@sgiehl
Copy link
Member

sgiehl commented Jan 18, 2022

@tsteur I would assume we have that problem at multiple other places as well. It's nowhere expected that date or period is given as an array in the URL. Wondering if we should try to have some kind of global solution, that maybe already throws an exception when parsing the request array or something like that.

@tsteur
Copy link
Member Author

tsteur commented Jan 18, 2022

I would first check if it's needed. If it can be done eg in checkDateFormat which is called as part of setGeneralVariablesViewAs then it be this way already covered in most places and we can avoid any global solution?

I suppose the problem might be period where it's more complicated.

If we can avoid a global solution that be good as it gives us more flexibility and it could in theory cause regressions etc (although I doubt it would). It's hard to say though without looking too much into it. A global solution could potentially work as well though, it will I suppose only restrict from using period[] etc which might not be used so far (but hard to say what plugins do). Guess even a non-global fix could break things.

@sgiehl
Copy link
Member

sgiehl commented Jan 19, 2022

I would first check if it's needed. If it can be done eg in checkDateFormat which is called as part of setGeneralVariablesViewAs then it be this way already covered in most places and we can avoid any global solution?

The problem is, that there are many places where we call Common::getRequestVar('period') without specifying the expected type. And those are often called before the view is rendered, so changing checkDateFormat wouldn't have an effect.

We could go through all usages of Common::getRequestVar('period') and Common::getRequestVar('date') and ensure the variable type 'string' is provided. This would cause an exception to be thrown as soon as an array would be provided.

@tsteur
Copy link
Member Author

tsteur commented Jan 19, 2022

@sgiehl not 100% sure but from a quick test below change would probably already cover 90% of the cases with very little effort (the API already handles things well by the looks and does not result in fatal error):

diff --git a/core/Period.php b/core/Period.php
index 4059a80155..00bb717607 100644
--- a/core/Period.php
+++ b/core/Period.php
@@ -118,6 +118,10 @@ abstract class Period
             return;
         }
 
+        if (!is_string($dateString)) {
+            throw new \Exception('Date parameter needs to be a string');
+        }
+
         Date::factory($dateString);
     }

Period might be bit different and might require 2 or 3 changes to cover most of the errors like below (although are likely quite a few more):

diff --git a/core/Plugin/Controller.php b/core/Plugin/Controller.php
index 241ef0632b..e19ccda1c8 100644
--- a/core/Plugin/Controller.php
+++ b/core/Plugin/Controller.php
@@ -477,7 +477,7 @@ abstract class Controller
     protected function getGraphParamsModified($paramsToSet = array())
     {
         if (!isset($paramsToSet['period'])) {
-            $period = Common::getRequestVar('period');
+            $period = Common::getRequestVar('period', 'day', 'string');
         } else {
             $period = $paramsToSet['period'];
         }
@@ -924,6 +924,9 @@ abstract class Controller
         $periodValidator = new PeriodValidator();
 
         $currentPeriod = Common::getRequestVar('period');
+        if (!is_string($currentPeriod)) {
+            throw new Exception('Expected period to be a string');
+        }
         $view->displayUniqueVisitors = SettingsPiwik::isUniqueVisitorsEnabled($currentPeriod);
         $availablePeriods = $periodValidator->getPeriodsAllowedForUI();
 
diff --git a/plugins/MultiSites/Controller.php b/plugins/MultiSites/Controller.php
index 86792958f6..62382b9fd4 100644
--- a/plugins/MultiSites/Controller.php
+++ b/plugins/MultiSites/Controller.php
@@ -75,7 +75,7 @@ class Controller extends \Piwik\Plugin\Controller
         Piwik::checkUserHasSomeViewAccess();
 
         $date   = Common::getRequestVar('date', 'today');
-        $period = Common::getRequestVar('period', 'day');
+        $period = Common::getRequestVar('period', 'day', 'string');
 
         $view = new View("@MultiSites/getSitesInfo");

Generally sounds good though to add a Piwik::getDate() and Piwik::getPeriod similar to getModule and getAction. We just want to avoid spending too much time on this edge case

@peterhashair
Copy link
Contributor

peterhashair commented Jan 19, 2022

right, so we want to add 2 functions Piwik::getDate() and Piwik::getPeriod() then replace those once.

$date   = Common::getRequestVar('date', 'today');
$period = Common::getRequestVar('period', 'day');

Also just checking do we have a param validator sth like this

$validatedData = $request->validate([
   'title' => ['required', 'unique:posts', 'max:255'],
   'body' => ['required'],
]);

@tsteur
Copy link
Member Author

tsteur commented Jan 19, 2022

Also just checking do we have a param validator s

We don't @peterhashair

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For errors / faults / flaws / inconsistencies etc. Help wanted Beginner friendly issues or issues where we'd highly appreciate community's help and involvement. not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
4 participants