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

Introduce new events Updater.componentUpdated, PluginManager.pluginInstalled, PluginManager.pluginUninstalled #10468

Merged
merged 5 commits into from Sep 14, 2016

Conversation

Projects
None yet
3 participants
@sgiehl
Copy link
Member

sgiehl commented Sep 4, 2016

Introduces some new events:

  • Updater.componentUpdated triggered after core or a plugin has been updated
  • PluginManager.pluginInstalled triggered after a plugin was installed
  • PluginManager.pluginUninstalled triggered after a plugin was uninstalled

@sgiehl sgiehl added this to the 3.0.0-b1 milestone Sep 4, 2016

*
* @param string $pluginName The plugin that has been installed.
*/
Piwik::postEvent('PluginManager.pluginInstalled', array($plugin));

This comment has been minimized.

Copy link
@tsteur

tsteur Sep 8, 2016

Member

For consistency with pluginunistalled, does it make sense to use $pluginName here as well? if $plugin is needed, could use it as a second param.

The docs need to be adjusted as it is currently Plugin $plugin not string $pluginName.

Maybe we could also add an example to all the new events with an example use case? We have this for many other events and it will be possible on the developer site.

Should we mention that this event may be triggered several times when the config file is not writable?

This comment has been minimized.

Copy link
@sgiehl

sgiehl Sep 14, 2016

Author Member

I'll change that. plugin name should be fine. Will also mention that it could be triggered multiple times.
The other events for de-/activating plugins don't have use case comments. Aren't those events self explaining?

This comment has been minimized.

Copy link
@tsteur

tsteur Sep 14, 2016

Member

I reckon they are self explaining 👍

* @param string $updatedVersion version updated to
* @param array $warningMessages warnings occurred during update
*/
Piwik::postEvent('Updater.componentUpdated', array($componentName, $updatedVersion, $warningMessages));

This comment has been minimized.

Copy link
@tsteur

tsteur Sep 8, 2016

Member

An example on how to use it might be useful. Also an example maybe for $updatedVersion and $warningMessages could be helpful. I wonder if we could refactor the $this->executeListenerHook('onComponentUpdateFinished', array($componentName, $updatedVersion, $warningMessages)); to use this event now. would make things simpler to have only one of them

This comment has been minimized.

Copy link
@sgiehl

sgiehl Sep 14, 2016

Author Member

the current implementation doesn't use the onComponentUpdateFinished hook at all. As far as I have seen we only have the CLI observer that uses some hooks. But that one is unused. Simply removing it would maybe make the observer kind of incomplete for possible new use cases. Completely removing the observer and replacing it using events, might not be the best solution, as well.

This comment has been minimized.

Copy link
@tsteur

tsteur Sep 14, 2016

Member

I'd be tempted to remove the observer completely and to replace it. It's not really "the Piwik way" and seems bit unneeded/over-engineered. However, there might be some benefits of having it so let's just leave it.

I was then wondering if we could use the observer/hook instead of the event but I think it's better to have the event as this is API 👍

@tsteur

This comment has been minimized.

Copy link
Member

tsteur commented Sep 8, 2016

The events will be also useful for CustomPiwikJs plugin. Can you maybe listen to them in that plugin for all 3 of them and call updateTracker similar to the other plugin deactivated events etc.?

sgiehl added some commits Sep 13, 2016

@tsteur

This comment has been minimized.

Copy link
Member

tsteur commented Sep 14, 2016

LGTM

@sgiehl

This comment has been minimized.

Copy link
Member Author

sgiehl commented Sep 14, 2016

Ok. I'd merge that. Should we backport the events to 2.x?

@tsteur

This comment has been minimized.

Copy link
Member

tsteur commented Sep 14, 2016

Yep that would be great.

@sgiehl

This comment has been minimized.

Copy link
Member Author

sgiehl commented Sep 14, 2016

Ok. I'll push that on the 2.x branch directly later...

@sgiehl sgiehl merged commit 2cd22a9 into 3.x-dev Sep 14, 2016

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/travis-ci/push The Travis CI build failed
Details

@sgiehl sgiehl deleted the additionalevents branch Sep 14, 2016

sgiehl added a commit that referenced this pull request Sep 14, 2016

@mattab mattab changed the title Introduce new events Introduce new events Updater.componentUpdated, PluginManager.pluginInstalled, PluginManager.pluginUninstalled Sep 26, 2016

@mattab

This comment has been minimized.

Copy link
Member

mattab commented Sep 26, 2016

Removed not-in-changelog label as it's an enhancement and can be noted in changelog :

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.