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

Thomas--F · 2016-09-26T14:41:38Z

Please issue pull request against the 2.x-dev branch

mattab · 2016-09-26T19:42:28Z

core/Tracker/VisitExcluded.php

@@ -89,8 +89,11 @@ public function isExcluded()
         * @param bool &$excluded Whether the request should be excluded or not. Initialized
         *                        to `false`. Event subscribers should set it to `true` in
         *                        order to exclude the request.
+         * @param Request $request Additional Informations for plugins to decide, if the visit is excluded


Can you please write more description ie The request object which contains all of the request's information

mattab · 2016-09-26T19:42:57Z

core/Tracker/VisitExcluded.php

@@ -89,8 +89,11 @@ public function isExcluded()
         * @param bool &$excluded Whether the request should be excluded or not. Initialized
         *                        to `false`. Event subscribers should set it to `true` in
         *                        order to exclude the request.
+         * @param Request $request Additional Informations for plugins to decide, if the visit is excluded
+         * @param bool|string $userAgent Additional Informations for plugins to decide, if the visit is excluded


The User-agent of this visit

mattab · 2016-09-26T19:43:16Z

core/Tracker/VisitExcluded.php

@@ -89,8 +89,11 @@ public function isExcluded()
         * @param bool &$excluded Whether the request should be excluded or not. Initialized
         *                        to `false`. Event subscribers should set it to `true` in
         *                        order to exclude the request.
+         * @param Request $request Additional Informations for plugins to decide, if the visit is excluded
+         * @param bool|string $userAgent Additional Informations for plugins to decide, if the visit is excluded
+         * @param bool|string $ip Additional Informations for plugins to decide, if the visit is excluded       


The IP address of the visitor

mattab · 2016-09-26T19:43:57Z

Please can you issue this pull request against 3.x-dev or 2.x-dev branch?

Thomas--F · 2016-09-26T20:47:29Z

I changed the descriptions, but how can change the pull request do the 2.x-dev branch?

sgiehl · 2016-09-26T20:49:21Z

Should be changeable if you click edit in the top right. If you can't do that, I could do that as well.

Thomas--F · 2016-09-26T20:51:43Z

That was easy. Thanks

mattab · 2016-09-26T21:25:14Z

@Thomas--F you haven't changed it - can you set it to 2.x-dev ?

tsteur · 2016-09-26T22:43:15Z

core/Tracker/VisitExcluded.php

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


This is a bit confusing here because the $this->request contains a method to get the userAgent and the IP as well. So can maybe someone explain (and afterwards add to documentation) when the passed userAgent and the IP is different to the one in the request?

$this->request contains only the IP and user agent, when the hook is called during the real visit. When I use the log-import, the values are empty.
This is the reason, why the IP and the user agent are parameters to the constructor:

public function __construct(Request $request, $ip = false, $userAgent = false)

Thomas--F · 2016-09-27T05:17:38Z

How can I change it to to 2.x-dev?
I edited the text above, what else to do?

sgiehl · 2016-09-27T07:02:24Z

I've changed the base now

tillsteinbach · 2016-09-27T08:03:38Z

It's only a four line change. Is there anything we can do to get that last minute into 2.16.3, or is there a strict policy?

sgiehl · 2016-09-27T08:08:09Z

Guess the branch need to be rebased as it now contains some commits from master, that shouldn't be in this PR

mattab · 2016-09-27T08:10:39Z

we'll merge it for 2.16.3

tillsteinbach · 2016-09-27T08:25:13Z

@mattab This is excellent news
@Thomas--F Can you rebase so that the PR only contains your commit and also add the information tsteur asked for to the inline documentation ($this->request only contains IP and user agent when the hook is called from a real visit. When log-import API is used the values are empty.)?

Thomas--F · 2016-09-27T10:04:21Z

I don't know how to rebase without a local installed git. Is it possible within the webpage?
But I created another patch (Thomas--F:patch-2) based on the dev 2.x-branch. Can you change the PR to this patch?

tillsteinbach · 2016-09-27T10:31:47Z

As far as I know you cannot change the source branch of a PR, only the target branch. And rebase of a branch on the webpage is also not possible. So if there is no way to do that locally the only way I can imagine is a new PR (were we would unfortunately loose our discussion)

Thomas--F · 2016-09-27T13:23:54Z

I cloned the patch-3 to a local machine:
git clone https://github.com/Thomas--F/piwik/ --branch patch-3

How can I now rebase it?

sgiehl · 2016-09-27T13:28:27Z

git rebase -i 2.x-dev
then you need to remove all commits from the list that aren't yours.
After successful rebase you need to to do a force push
git push --force-with-lease

Thomas--F · 2016-09-27T13:56:32Z

git rebase -i 2.x-dev
fatal: Needed a single revision
invalid upstream 2.x-dev

tillsteinbach · 2016-09-27T14:07:19Z

Command should begit rebase -i origin/2.x-dev I guess, or you have to locally create 2.x-dev (what I would not recommend)

…10415)

Thomas--F · 2016-09-27T14:28:10Z

Success! Now only one file is changed an (hopefully) all comments are there.

tsteur · 2016-09-27T19:02:10Z

core/Tracker/VisitExcluded.php

+         * @param bool|string $ip The IP-address of the visitor
+         *
+         * The additional parameters $userAgent and $ip are needed, because the values in the 
+         * object $request are empty when the constructor is called from the log-import.


Sorry for being a bit annoying here. I understand they are empty when there is a log-import but I don't really understand why? Are they supposed to be ignored when doing a log-import? Do I need to treat them differently when they are set / not set? Is it ok to fallback to $this->request? Can I maybe instead always use the ones from $this->request? When IP and user agent is set, are they the same ones as in $this->request? :)

I don't know how the request-object is build, but look at the constructor of VisitExcluded:

public function __construct(Request $request, $ip = false, $userAgent = false) { $this->spamFilter = new ReferrerSpamFilter(); if (false === $ip) { $ip = $request->getIp(); } if (false === $userAgent) { $userAgent = $request->getUserAgent(); } $this->request = $request; $this->idSite = $request->getIdSite(); $this->userAgent = $userAgent; $this->ip = $ip; }

You can see, that ip and user agent are taken from the given paramters when provided. So I think these values are more trustworthy than the content of the request-object.

OK thank you. We had a look and noticed the userAgent is actually never passed and always the same as userAgent from the request. Also the IP seems to be always the same and the Can you remove the userAgent and IP from the postEvent as it is not needed and rather confusing?

Sorry for that. We will likely clean up the code there and remove it from VisitExcluded shortly so it's more clear next time

mattab · 2016-09-28T20:58:48Z

Actually the two extra parameter in the constructor are not needed, so i've removed them here: https://github.com/piwik/piwik/pull/10593/files

therefore we only need to add the Request object to the event.

* 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

Thomas--F · 2016-09-29T12:52:48Z

Thank you for cleaning up the code. I will shortly use the new parameter in the BotTracker-plugin and do some tests with the log-import.

Thomas--F mentioned this pull request Sep 26, 2016

Give plugins using isExcludedVisit hook more information #10415

Closed

mattab reviewed Sep 26, 2016

View reviewed changes

mattab added this to the 3.0.0-b1 milestone Sep 26, 2016

tsteur reviewed Sep 26, 2016

View reviewed changes

sgiehl changed the base branch from master to 2.x-dev September 27, 2016 06:53

mattab modified the milestones: 2.16.3, 3.0.0-b1 Sep 27, 2016

Thomas--F added 3 commits September 27, 2016 14:23

Additional informations passed in the hook "isExcludedVisit" (issue #…

3ef45d8

…10415)

Added better description to the new parameters

44c8776

Update VisitExcluded.php

9878dc1

tsteur reviewed Sep 27, 2016

View reviewed changes

mattab added 2 commits September 29, 2016 10:01

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

f50a1d0

Update VisitExcluded.php

d5c6e11

mattab merged commit 8ef6b29 into matomo-org:2.x-dev Sep 28, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Thomas--F commented Sep 26, 2016 •

edited

mattab Sep 26, 2016

mattab Sep 26, 2016

mattab Sep 26, 2016

mattab commented Sep 26, 2016

Thomas--F commented Sep 26, 2016

sgiehl commented Sep 26, 2016

Thomas--F commented Sep 26, 2016

mattab commented Sep 26, 2016

tsteur Sep 26, 2016

Thomas--F Sep 27, 2016

Thomas--F commented Sep 27, 2016

sgiehl commented Sep 27, 2016

tillsteinbach commented Sep 27, 2016

sgiehl commented Sep 27, 2016

mattab commented Sep 27, 2016

tillsteinbach commented Sep 27, 2016

Thomas--F commented Sep 27, 2016 •

edited

tillsteinbach commented Sep 27, 2016

Thomas--F commented Sep 27, 2016

sgiehl commented Sep 27, 2016

Thomas--F commented Sep 27, 2016

tillsteinbach commented Sep 27, 2016

Thomas--F commented Sep 27, 2016

tsteur Sep 27, 2016

Thomas--F Sep 28, 2016

tsteur Sep 28, 2016

tsteur Sep 28, 2016

mattab commented Sep 28, 2016

Thomas--F commented Sep 29, 2016

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

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

Conversation

Thomas--F commented Sep 26, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattab commented Sep 26, 2016

Thomas--F commented Sep 26, 2016

sgiehl commented Sep 26, 2016

Thomas--F commented Sep 26, 2016

mattab commented Sep 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Thomas--F commented Sep 27, 2016

sgiehl commented Sep 27, 2016

tillsteinbach commented Sep 27, 2016

sgiehl commented Sep 27, 2016

mattab commented Sep 27, 2016

tillsteinbach commented Sep 27, 2016

Thomas--F commented Sep 27, 2016 • edited

tillsteinbach commented Sep 27, 2016

Thomas--F commented Sep 27, 2016

sgiehl commented Sep 27, 2016

Thomas--F commented Sep 27, 2016

tillsteinbach commented Sep 27, 2016

Thomas--F commented Sep 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattab commented Sep 28, 2016

Thomas--F commented Sep 29, 2016

Thomas--F commented Sep 26, 2016 •

edited

Thomas--F commented Sep 27, 2016 •

edited