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.

Sign up for GitHub

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

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

Merged
merged 5 commits into from
Sep 28, 2016
Merged

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

Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3ef45d8
Additional informations passed in the hook "isExcludedVisit" (issue #…
Thomas--F Sep 26, 2016
44c8776
Added better description to the new parameters
Thomas--F Sep 26, 2016
9878dc1
Update VisitExcluded.php
Thomas--F Sep 27, 2016
f50a1d0
Remove two parameters not needed as better to use the Request object
mattab Sep 28, 2016
d5c6e11
Update VisitExcluded.php
mattab Sep 28, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion core/Tracker/VisitExcluded.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,8 +89,14 @@ 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 The request object which contains all of the request's information
* @param bool|string $userAgent The user-agent used by the visitor
* @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.
Copy link
Member

Choose a reason for hiding this comment

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

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? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

$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)


/*
* Following exclude operations happen after the hook.
Expand Down