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

Jump to bottom

[Hotfix]add date, period params into common function #18686

Merged
merged 18 commits into from Feb 14, 2022
Merged

[Hotfix]add date, period params into common function #18686

merged 18 commits into from Feb 14, 2022

Conversation

peterhashair
Copy link
Contributor

Description:

Fixes: #18639
Accidently click merge on original PR #18653, revert it.

Review

Peter and others added 9 commits January 19, 2022 16:34
Update Common.php
01efbc9 
add array check in common request
Update Common.php
edfd5df 
test
Merge branch '4.x-dev' into m-18639-array-request-params
71956ef
add common period and date function
faf6a07 
add common period and date function
@sgiehl
Update core/Piwik.php
eaa8665 
Co-authored-by: Stefan Giehl <stefan@matomo.org>
@sgiehl
Update core/Piwik.php
5b633a7 
Co-authored-by: Stefan Giehl <stefan@matomo.org>
update function
83ed55c 
update function
@sgiehl
Update core/Piwik.php
171e921 
Co-authored-by: Stefan Giehl <stefan@matomo.org>
Update Controller.php
44f3c09 
update isset check
@peterhashair peterhashair added the Needs Review PRs that need a code review label Jan 26, 2022
@peterhashair peterhashair added this to the 4.8.0 milestone Jan 26, 2022
@peterhashair peterhashair added the not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. label Jan 26, 2022
sgiehl
sgiehl requested changes Jan 31, 2022
View reviewed changes
core/Piwik.php Show resolved Hide resolved
core/Piwik.php Show resolved Hide resolved
core/Plugin/Controller.php Outdated Show resolved Hide resolved
core/Plugin/Controller.php Outdated Show resolved Hide resolved
core/Plugin/Controller.php Outdated Show resolved Hide resolved
@sgiehl sgiehl removed the Needs Review PRs that need a code review label Jan 31, 2022
Peter Zhang and others added 5 commits February 1, 2022 02:18
@sgiehl
Update core/Plugin/Controller.php
2a5ec8f 
Co-authored-by: Stefan Giehl <stefan@matomo.org>
@sgiehl
Update core/Piwik.php
db282ad 
Co-authored-by: Stefan Giehl <stefan@matomo.org>
update isset check
928c3bb 
update isset check
Merge branch 'm-18639-period-request' of github.com:matomo-org/matomo…
2a03613 
… into m-18639-period-request
Update Piwik.php
eb1bf42 
update phpdocs
@sgiehl
Copy link
Member

sgiehl commented Feb 1, 2022

Overall the code looks fine, but the errors described in the original issue are actually still showing up as warnings in the log:

WARNING MultiSites[2022-02-01 14:30:24 UTC] [09f2b] /srv/matomo/core/Date.php(1079): Deprecated - strtolower(): Passing null to parameter #1 ($string) of type string is deprecated - Matomo 4.7.0-rc1 - Please report this message in the Matomo forums: https://forum.matomo.org (please do a search first as it might have been reported already) [internal function]: Piwik\ErrorHandler::errorHandler(),#1/core/Date.php(1079),#2/core/Period/Range.php(255),#3/core/Period.php(131),#4/core/Period/Range.php(152),#5/core/Period/Range.php(543),#6/core/Plugin/Controller.php(491),#7/plugins/MultiSites/Controller.php(97),#8/plugins/MultiSites/Controller.php(37),[internal function]: Piwik\Plugins\MultiSites\Controller->index()

I guess to solve that you may need to change this:

matomo/plugins/MultiSites/Controller.php

Line 97 in 83ed55c

$params = $this->getGraphParamsModified();

and supply ['period' => $period] as parameter for the method call.

Peter Zhang and others added 3 commits February 2, 2022 09:27
@peterhashair peterhashair added the Needs Review PRs that need a code review label Feb 8, 2022
@peterhashair peterhashair changed the title add date, period params into common function [Bug]add date, period params into common function Feb 8, 2022
@peterhashair peterhashair changed the title [Bug]add date, period params into common function [Hotfix]add date, period params into common function Feb 8, 2022
sgiehl
sgiehl requested changes Feb 14, 2022
View reviewed changes
Copy link
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

left a comment, which I guess causes the UI test failures.

core/Plugin/Controller.php Outdated Show resolved Hide resolved
@sgiehl
Update core/Plugin/Controller.php
ed0eebf 
Co-authored-by: Stefan Giehl <stefan@matomo.org>
sgiehl
sgiehl approved these changes Feb 14, 2022
View reviewed changes
Copy link
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems tests are passing now

@sgiehl sgiehl merged commit 65383e5 into 4.x-dev Feb 14, 2022
@sgiehl sgiehl deleted the m-18639-period-request branch February 14, 2022 11:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sgiehl sgiehl sgiehl approved these changes

Assignees
No one assigned
Labels
Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Milestone
4.8.0
Development

Successfully merging this pull request may close these issues.

strtolower(): Argument #1 ($string) must be of type string, array given in Date.php
2 participants
@peterhashair @sgiehl