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

Additional events in archiving lifecycle #8738

Merged
merged 3 commits into from Sep 9, 2015

Conversation

Projects
None yet
3 participants
@andrzejewsky
Contributor

andrzejewsky commented Sep 8, 2015

Hello @piwik/core-team,

I created this PR, because sometimes we need to inject into archiving lifecycle, especially we want to detect when archiving was started and finished. I added two additional events (look at changes). Please review this: Maybe should I pass more arguments into those events ? Maybe then it would be more useful ?

@tsteur

This comment has been minimized.

Show comment
Hide comment
@tsteur

tsteur Sep 8, 2015

Member

Maybe we can rename the events to CoreArchive.run.start and CoreArchive.run.finish similar to our existing event CoreArchive.init.finish: http://developer.piwik.org/api-reference/events#cronarchiveinitfinish

CoreArchive.finish should be rather triggered before or while the end() method is called and CoreArchive.start should maybe rather triggered before or while init() method is called. See https://github.com/piwik/piwik/blob/2.15.0-b5/core/CronArchive.php#L271 . Maybe someone else has other thoughts?

Regarding parameters: I'd find it awesome to only post the current instance of CoreArchive there, eg Piwik::postEvent('CronArchive.run.start', array($this));. Posting the timer is a bit inflexible and only useful for this use case. By posting the instance other developers can make use of it for other use cases.

To still have access to the timer you could either create a new Timer in your plugin or maybe we could add a method $cronArchive->getTimer(). It would be actually best to use your own timer in your plugin, ideally by using microtime() yourself since \Piwik\Timer would become a public API otherwise. We can make it API if needed though @mattab ? (In the past I did not want to make it API since it is not really analytics related but could do it though).

A tiny little documentation re this event would be nice see https://github.com/piwik/piwik/blob/2.15.0-b5/core/CronArchive.php#L313-L319 but we can also add it ourselves.

Member

tsteur commented Sep 8, 2015

Maybe we can rename the events to CoreArchive.run.start and CoreArchive.run.finish similar to our existing event CoreArchive.init.finish: http://developer.piwik.org/api-reference/events#cronarchiveinitfinish

CoreArchive.finish should be rather triggered before or while the end() method is called and CoreArchive.start should maybe rather triggered before or while init() method is called. See https://github.com/piwik/piwik/blob/2.15.0-b5/core/CronArchive.php#L271 . Maybe someone else has other thoughts?

Regarding parameters: I'd find it awesome to only post the current instance of CoreArchive there, eg Piwik::postEvent('CronArchive.run.start', array($this));. Posting the timer is a bit inflexible and only useful for this use case. By posting the instance other developers can make use of it for other use cases.

To still have access to the timer you could either create a new Timer in your plugin or maybe we could add a method $cronArchive->getTimer(). It would be actually best to use your own timer in your plugin, ideally by using microtime() yourself since \Piwik\Timer would become a public API otherwise. We can make it API if needed though @mattab ? (In the past I did not want to make it API since it is not really analytics related but could do it though).

A tiny little documentation re this event would be nice see https://github.com/piwik/piwik/blob/2.15.0-b5/core/CronArchive.php#L313-L319 but we can also add it ourselves.

@mattab

This comment has been minimized.

Show comment
Hide comment
@mattab

mattab Sep 8, 2015

Member

We can make it API if needed though @mattab ? (In the past I did not want to make it API since it is not really analytics related but could do it though).

sounds good to me to make Timer API.

Member

mattab commented Sep 8, 2015

We can make it API if needed though @mattab ? (In the past I did not want to make it API since it is not really analytics related but could do it though).

sounds good to me to make Timer API.

@tsteur

This comment has been minimized.

Show comment
Hide comment
@tsteur

tsteur Sep 9, 2015

Member

@andrzejewsky

To still have access to the timer you could either create a new \Piwik\Timer() instance in your plugin and we'd make it a public API

Member

tsteur commented Sep 9, 2015

@andrzejewsky

To still have access to the timer you could either create a new \Piwik\Timer() instance in your plugin and we'd make it a public API

@andrzejewsky

This comment has been minimized.

Show comment
Hide comment
@andrzejewsky

andrzejewsky Sep 9, 2015

Contributor

@tsteur I made your suggestion.
About Timer: It was only example of argument which we can pass to those events. I don't really need this.

I need only inject into start and the end of archiving (sometimes is very useful). Mentioned by you $this as event argument, sounds more flexible.

Contributor

andrzejewsky commented Sep 9, 2015

@tsteur I made your suggestion.
About Timer: It was only example of argument which we can pass to those events. I don't really need this.

I need only inject into start and the end of archiving (sometimes is very useful). Mentioned by you $this as event argument, sounds more flexible.

Show outdated Hide outdated core/CronArchive.php
@andrzejewsky

This comment has been minimized.

Show comment
Hide comment
@andrzejewsky

andrzejewsky Sep 9, 2015

Contributor

maybe write archiving instead of archivization for consistency
Ok @mattab I changed it.

Contributor

andrzejewsky commented Sep 9, 2015

maybe write archiving instead of archivization for consistency
Ok @mattab I changed it.

@tsteur tsteur added this to the 2.15.0 milestone Sep 9, 2015

@tsteur

This comment has been minimized.

Show comment
Hide comment
@tsteur

tsteur Sep 9, 2015

Member

Thx!

Member

tsteur commented Sep 9, 2015

Thx!

tsteur added a commit that referenced this pull request Sep 9, 2015

Merge pull request #8738 from andrzejewsky/master
Additional events in archiving lifecycle

@tsteur tsteur merged commit 05efb38 into matomo-org:master Sep 9, 2015

2 checks passed

Scrutinizer No new issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@mattab mattab removed the Enhancement label Oct 13, 2015

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