This PR heavily refactors the tracking code to make it easy for plugins to tweak, change and extend tracking behavior. It moves a big chunk of the code currently in Visit::handle() to other plugins, like Goals, Actions, CustomVariables, etc.
Note: Though this PR implements enough changes for the plugin I'm working on, it doesn't completely close #8071, there's still more logic that can be moved out of core.
This PR introduces the concept of the RequestProcessor. RequestProcessors handle and tweak tracker behavior. To learn more, read the class docs for the RequestProcessor class.
isTrackerPlugin(). Plugins that don't implement dimensions or use tracker events can override this method to make sure they are loaded during tracking (if they need it).
This TODO list can be added to #8071 or to a new issue:
afterRequestProcessed()is used for both defining metadata that depends on other plugins' metadata and for manipulating other plugins' metadata. This means metadata that uses other plugins' metadata, can't be manipulated. Order of plugins will be come a problem eventually if many plugins get involved together...
<a class='mention' href='https://github.com/api'>@api</a>it should be made as easy to use as possible.
[ ] Do not use multi-dimensional array of properties for request metadata, instead use array of classes defined by plugins, so:
/** <a class='mention' href='https://github.com/var'>@var</a> $data MyPluginRequestMetadata */ $data = $request->getMetadata('MyPlugin');
(moved to 2.15.0)
FYI, this PR is done now. There may be more TODO after it is merged though.
could you do here a little risk analysis and find out the list of plugin names that may break, because of this change? (from our open source and/or premium and/or client custom plugins)
It is acceptable breaking plugins that have a class extending Tracker\Visit in 2.15.0 (because Tracker\Visit is not tagged with
<a class='mention' href='https://github.com/api'>@api</a>) but it will help to know list of plugins we break here. (I write here about
Tracker\Visit as you mentioned it on Slack, and maybe others parts have changed too - didn't check)
FYI searched with https://piwikcodesearch.herokuapp.com and couldn't find community plugins extending the Visit class. Found 3 Piwik Pro plugins though: https://github.com/search?q=user%3Apiwikpro+%22Piwik%5CTracker%5CVisit%22&type=Code
Good to know! an issue was created in each of these plugin tracker.
In general can we add unit / integration tests for the new files in
Also some of my comments depend on whether this becomes API or not. Will it be API?
FYI, moved the following exchange out of comments since it is now part of an outdated diff:
I find this concept a bit "weird". Would it make sense to Request::setMetadata() on the request object? Visitor and request metadata doesn't sound like it belongs together maybe? Also maybe we can remove one dimension instead of $pluginName and $key just have $name if possible? Plugin name would be acceptable for me only if it was set by the platform automatically, if possible.
Is it possible to not have metadata but clear objects and what can be done on the Visitor object? Eg $visitor->setIsVisitorKnown($isKnown)? One problem that I have is that eg the core knows about plugins. Eg within core (/core) we do getRequestMetadata('CoreHome', 'isVisitorKnown'), but core should not know anything about the plugins.
If the metadata is stored in a core object, plugins cannot define their own metadata that can be changed by other plugins. Also the core object would have to have 'setters', since the logic to set them is accomplished through RequestProcessors, not externally.
Here is an alternative approach:
// in a RequestProcessor ... /** <a class='mention' href='https://github.com/var'>@var</a> ActionsRequestMetadata $data */ $data = $request->getMetadata('Goals'); $data->goalsConverted = array();
$data = $request->getMetadata('Piwik\Goals\Tracker\GoalsRequestMetadata'); // PHPStorm will be configured to set the type of $data to the string argument
To avoid naming collisions and strange errors, the plugin name should be used. Setting it automatically is possible, the plugin name can be detected from the namespace.
Eg within core (/core) we do getRequestMetadata('CoreHome', 'isVisitorKnown'), but core should not know anything about the plugins.
This is because I only moved what was necessary for the new plugin. There is a Future TODO item about moving the rest of the logic.
At least some are expected to exist by core like CoreHome, isVisitKnown. Something like this is not metadata and would be better to have in classes/methods of core if possible (including setters). Once all is done in that plugin it could be maybe a metadata again.
Why should it be in core? Would it be unexpected for a plugin to change it? The logic that sets it isn't in core.
It can exist in a class instead of as an element in an array, see above. W/o support for generics, it will never look perfect.
Tried moving properties from VisitProperties to Visitor, can't be done currently since it is required to initialize Visitor w/ whether the visitor is known or not, and VisitProperties are needed before this point.
Okay, I reckon we can have a look at it again when it becomes public API and hopefully by then we have maybe a different naming anyway (eg User and Session) etc
Did quick benchmark, Visit::handle() does not appear to benchmark. Will merge shortly so new plugin can be demo-d.