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

Give plugins using isExcludedVisit hook more information #10415

Closed
e-dschungel opened this issue Aug 18, 2016 · 9 comments
Closed

Give plugins using isExcludedVisit hook more information #10415

e-dschungel opened this issue Aug 18, 2016 · 9 comments
Labels
Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc.
Milestone

Comments

@e-dschungel
Copy link

It would be nice if the hook isExcludedVisit would give more information the the plugins using it.
For example the Plugin BotTracker has no possibility to get the useragent when the log-import script with the --enable-bots options is used. So the Bottracker tables stay empty. Other plugins could use the additional information to make a better choice if the visit should be excluded or not.

The suggestion is to extend the hook with idSite or request and userAgent like this:

Piwik::postEvent('Tracker.isExcludedVisit', array(&$excluded, $this->idSite, $this->userAgent));
Piwik::postEvent('Tracker.isExcludedVisit', array(&$excluded, $this->request, $this->userAgent));

see the discussion at https://github.com/Thomas--F/BotTracker/issues/38 (in german)

@tillsteinbach
Copy link

I strongly support this change! As the parameters are past as array this should not break any existing code. This is really important as the plugins using the hook cannot reliably access any information related to the request.

@Thomas--F
Copy link
Contributor

The first suggestion is sufficient for my needs but the second one gives far more information to the plugin.
If I use this parameters instead of $_REQUEST and $_SERVER it doesn't matter if the plugin is called live or from the log-import.

@mattab
Copy link
Member

mattab commented Aug 22, 2016

Hi @e-dschungel - @tillsteinbach @Thomas--F

It sounds good to add a new parameter to the event. I propose to add $this->request after $excluded this way you have access to everything. User agent for example is $this->request->getUserAgent()

Could you maybe submit a pull request for this improvement?

@mattab mattab added this to the 2.16.x (LTS) milestone Aug 22, 2016
@mattab mattab added the Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc. label Aug 22, 2016
@mattab
Copy link
Member

mattab commented Aug 23, 2016

Note: If you'd like this change in the next Piwik version 2.16.3 please submit the PR this week!

@Thomas--F
Copy link
Contributor

It's not enough, just to add $this->request. During the log-import, the user agent an the ip-address is passed directly to the constructor and not in the $request-object.
So I prefer
Piwik::postEvent('Tracker.isExcludedVisit', array(&$excluded, $this->request, $this->userAgent));

To pass all the given information, you can also add the parameter $this->ip, but I don't actually need it.

I've made a PR with all informations but there is a syntax error with one of the brackets behind &$excluded. How can I repair this in a pull request?

@sgiehl
Copy link
Member

sgiehl commented Aug 23, 2016

@Thomas--F simply add a new commit to the branch the PR comes from. the PR will be automatically updated.

@Thomas--F
Copy link
Contributor

OK, the second PR ist correct now.

@mattab mattab modified the milestones: 2.16.x (LTS), Mid term Aug 25, 2016
@tillsteinbach
Copy link

Sorry, I must be blind, but I cannot find the PR. What is the current state of this feature?

@Thomas--F
Copy link
Contributor

Sorry, something went wrong. Now there ist finally a PR:
#10564

mattab pushed a commit that referenced this issue Sep 28, 2016
…10415) (#10564)

* Additional informations passed in the hook "isExcludedVisit" (issue #10415)

* Added better description to the new parameters

* Update VisitExcluded.php

* Remove two parameters not needed as better to use the Request object

* Update VisitExcluded.php
mattab added a commit that referenced this issue Sep 29, 2016
* Fix depraction test: use assertDeprecatedMethodIsRemovedInPiwik3

* Fix Scheduled Reports sent one hour late in daylight saving timezones (#10443)

* convert hour to send report to/from UTC, to ensure it isn't affected by daylight savings

* adds update script to change existing scheduled reports to use utc time

* code improvement

* adds missing param

* Added new event Archiving.makeNewArchiverObject to  allow customising plugin archiving  (#10366)

* added hook to alllow plugin archiving prevention

* cr code style notes

* reworked PR to fit CR suggestions

* added PHPDoc for hook

* Event description more consistent

* UI tests: minor changes

* Comment out Visitor Log UI tests refs #10536

* Adds test checking if all screenshots are stored in lfs

* removed screenshots not stored in lfs

* readds screenshots to lfs

* 2.16.3-b4

* Issue translation updates against 2.x-dev

* language update

* Fix bug in widget list remove where the JSON object becomes array

* 2.16.3-rc1

* console command custom-piwik-js:update should work when directory is writable and file does not exist yet (#10576)

* followup #10449

* Fix test
(cherry picked from commit fac3d63)

* Prevent chmod(): No such file or directory

* Automatically update all marketplace plugins when updating Piwik (#10527)

* update plugins and piwik at the same time

* make sure plugins are updated with piwik

* use only one try/catch

* reload plugin information once it has been installed

* make sure to clear caches after an update

* fix ui tests

* make sure to use correct php version without any extras

* Additional informations passed in the hook "isExcludedVisit" (issue #10415) (#10564)

* Additional informations passed in the hook "isExcludedVisit" (issue #10415)

* Added better description to the new parameters

* Update VisitExcluded.php

* Remove two parameters not needed as better to use the Request object

* Update VisitExcluded.php

* remove extra two parameters in VisitExcluded constructor to prevent confusion (#10593)

* Update our libs to latest #10526

* Update composer libraries to latest #10526

* Update log analytics to latest

* When updating the config file failed (or when any other file is not writable...), the Updater (for core or plugins) will now automatically throw an error and cancel the update (#10423)

* When updating the config file failed (or when any other file is not writable...), the Updater (for core or plugins) will now automatically throw an error and cancel the update

* add integration test to check the correct exception is thrown when config not writabel

* New integration test for updater

* Make test better

* When opening the visitor profile, do not apply the segment (#10533)

* When opening the visitor profile, do not apply the segment

* added ui test for profile but does work for me

* next try to make ui test work

* add expected screenshot

* added missing doc
@mattab mattab closed this as completed Oct 3, 2016
@mattab mattab modified the milestones: 2.16.3, Mid term Oct 3, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc.
Projects
None yet
Development

No branches or pull requests

5 participants