Move CustomVariables::getNumUsableCustomVariables logic to DI so tests do not need to access database #8806
Conversation
diosmosis
added
not-in-changelog
…he static variable is not needed.
…opes(). Move getNumUsableCustomVariables logic to CustomVariablesMetadataProvider (new service class in DI).
diosmosis
force-pushed
the
8756_cvar_di
branch
from
d74c4b5
to
14a20fe
Compare
…tom variable segments do not have to be added manually when getting all segments.
diosmosis
force-pushed
the
8756_cvar_di
branch
from
14a20fe
to
889a486
Compare
|
Cannot merge w/o refactoring the tracker cache. Moving to 3.0. |
| 'Piwik\Plugins\CustomVariables\Model.conversion' => DI\object('Piwik\Plugins\CustomVariables\Model') | ||
| ->constructor(Model::SCOPE_CONVERSION), | ||
|
|
||
| 'Piwik\Plugins\CustomVariables\Model.all' => array( |
There was a problem hiding this comment.
Is it really needed to have it in DI like this? I know it might be nice to inject it but looks a bit over engineered / complex to me. I don't think one would want to change any thing here, does one?
There was a problem hiding this comment.
It's not about changing/extending the code (not always), it's about decoupling dependencies. Model is a dependency of CustomVariableMetadataProvider, so Model has to be in DI to inject it. If we don't do this everywhere there's application logic, we'll have to keep using StaticContainer in classes that aren't in DI, which will make embedding environments (important for cloud & tests) impossible to do completely.
The reason for the complexity here is due to the design of the Model class. Since it takes a scope as a constructor argument, we have to create multiple instances of it in DI. I can simplify it w/ some refactoring, just didn't want to w/ this PR. Of course, since the PR is broken, it's not as big a deal now.
|
There are some failing tests in https://travis-ci.org/piwik/piwik/jobs/80746350 . Is it related to this? Looks like it. Left a few comments but can be ignored I reckon |
Yeah, that's why I moved it to 3.0. The tracker cache also accesses the database, so replacing this specific logic doesn't work completely. |
|
I'll close this PR for now as it's not really needed right now and the PR would need to be made against |
This PR creates a new class, CustomVariablesMetadataProvider, which contains the main logic for
CustomVariables::getNumUsableCustomVariables. A mock class is defined and used by AutoSuggestAPITest soAPI.getSegmentsMetadatacan be used in the data provider instead of having to define them manually.Additional changes in this PR:
Piwik\Tracker\Cache::$cache. Since the Cache instance comes from DI, we don't need the static variable anymore.CustomVariables\Modelinstances to DI (one per 'scope').Fixes #8756