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.

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

Added Piwik implementation #1702

Merged
merged 2 commits into from
Feb 22, 2016
Merged

Added Piwik implementation #1702

merged 2 commits into from
Feb 22, 2016

Conversation

nicosomb
Copy link
Member

Q A
Bug fix? no
New feature? yes
BC breaks? yes
Deprecations? yes
Tests pass? no
Documentation no
Translation yes
Fixed tickets #1137
License MIT

I don't know if I have to write test for this, because I use PiwikExtension and CraueConfigBundle, which are already tested.

Tests failed, due to new params I think. I don't know really why.

@nicosomb nicosomb added this to the 2.0.0-beta.1 milestone Feb 19, 2016
{
public function __construct(Config $craueConfig)
{
parent::__construct($craueConfig->get('piwik_host'), $craueConfig->get('piwik_site_id'), $craueConfig->get('piwik_enabled'));
Copy link
Member

Choose a reason for hiding this comment

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

Could you split them line by line? I'll more readable.
Also, could you add a comment on the class telling why you are doing that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants