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

Get goals through Request::processRequest instead of directly from AP… #13293

Merged
merged 7 commits into from Sep 10, 2018

Conversation

diosmosis
Copy link
Member

…I so events can fire.

@diosmosis diosmosis added not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Needs Review PRs that need a code review labels Aug 14, 2018
@diosmosis diosmosis added this to the 3.7.0 milestone Aug 14, 2018
@mattab mattab modified the milestones: 3.7.0, 3.6.1 Sep 1, 2018
@@ -44,7 +44,7 @@ public function getSparklines()
$goalDefinition['name'] = $this->translator->translate('Goals_Ecommerce');
$goalDefinition['allow_multiple'] = true;
} else {
$goals = GoalsApi::getInstance()->getGoals($this->idSite);
$goals = Request::processRequest('Goals.getGoals', ['idSite' => $this->idSite]);
Copy link
Member

Choose a reason for hiding this comment

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

do we need the filter_limit = -1 here as well to be safe and set default to []? eg the controller might send a filter_limit etc?

@tsteur
Copy link
Member

tsteur commented Sep 8, 2018

Added a comment but otherwise looks good to merge if tests pass. Needs to fix some conflicting files by the looks as well.

@@ -77,7 +77,7 @@ public function getConversionsOverview()
$view = new View('@Ecommerce/conversionOverview');
$idGoal = Common::getRequestVar('idGoal', null, 'string');

$goalMetrics = Request::processRequest('Goals.get', array('idGoal' => $idGoal));
$goalMetrics = Request::processRequest('Goals.get', array('idGoal' => $idGoal, 'filter_limit' => '-1'), $default = []);
Copy link
Member

Choose a reason for hiding this comment

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

Actually, I wonder if we also need to set always filter_offset in case it is sent along the request somehow?

Copy link
Member

Choose a reason for hiding this comment

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

I think by setting default=[] likely not needed, forgot about it.

Copy link
Member Author

Choose a reason for hiding this comment

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

$default = [] changes the default values of the request from being $_GET + $_POST to [], so if filter_limit/filter_offset is sent in the request, it will be ignored. I'm pretty sure filter_limit = -1 isn't necessary w/ default, but like was discussed before, can't hurt.

@diosmosis diosmosis merged commit f72637f into 3.x-dev Sep 10, 2018
@diosmosis diosmosis deleted the goals-api-request branch September 10, 2018 06:23
diosmosis added a commit that referenced this pull request Sep 20, 2018
#13293)

* Get goals through Request::processRequest instead of directly from API so events can fire.

* Do not use Request in tracker requests since renderers are not loaded.

* Add default = [] & filter_limit = -1 to changed Goals.getGoals calls.

* Add empty default request to one Goals.get call.

* fix another test

* Use 3.x-dev file.
InfinityVoid pushed a commit to InfinityVoid/matomo that referenced this pull request Oct 11, 2018
matomo-org#13293)

* Get goals through Request::processRequest instead of directly from API so events can fire.

* Do not use Request in tracker requests since renderers are not loaded.

* Add default = [] & filter_limit = -1 to changed Goals.getGoals calls.

* Add empty default request to one Goals.get call.

* fix another test

* Use 3.x-dev file.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

None yet

3 participants