-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Auto generated glossary of all Metrics & Report definitions in Piwik #8793
Conversation
{ | ||
Piwik::checkUserHasSomeViewAccess(); | ||
|
||
$glossary = new Glossary($this->idSite); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see this PR is still a work in progress, but I want to mention that classes that encapsulate application logic should be stateless or immutable service classes and should be obtained through DI. Instead of creating an instance of this type manually here, you should get it through StaticContainer.
To do this, you must:
- Remove the idSite as a constructor parameter and move to individual methods.
- Add constructor arguments for all dependencies in
Glossary
class. Dependencies that aren't set via DI (ie, singletons that are not yet in DI) should be set in the constructor, but not as constructor arguments. (These I'll change manually when getting rid of Singleton/StaticContainer).
|
||
namespace Piwik\Plugins\API; | ||
|
||
class Glossary |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add tests for this class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Auto generated glossary of all Metrics & Report definitions in Piwik
#6773
Todo: