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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 5 commits into from
Sep 14, 2016
Merged

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
82d811b
trigger events after a plugin was installed / uninstalled
sgiehl Sep 2, 2016
b300a6d
post event after component has been updated
sgiehl Sep 2, 2016
b616357
add new events to changelog
sgiehl Sep 2, 2016
27078cb
use new events in CustomPiwikJs plugin
sgiehl Sep 13, 2016
11e27e6
improve comments
sgiehl Sep 14, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,9 @@ Read more about migrating a plugin from Piwik 2.X to Piwik 3 on our [Migration g
* `Dimension.filterDimension` let's you filter any dimensions
* `Report.addReports` let's you add dynamically created reports
* `Report.filterReports` let's you filter any report
* `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

### New features
* New "Sparklines" visualization that let's you create a widget showing multiple sparklines
Expand Down
15 changes: 15 additions & 0 deletions core/Plugin/Manager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -419,6 +419,14 @@ public function uninstallPlugin($pluginName)
if ($this->isPluginInFilesystem($pluginName)) {
return false;
}

/**
* Event triggered after a plugin has been uninstalled.
*
* @param string $pluginName The plugin that has been uninstalled.
*/
Piwik::postEvent('PluginManager.pluginUninstalled', array($pluginName));

return true;
}

Expand Down Expand Up @@ -1080,6 +1088,13 @@ private function installPluginIfNecessary(Plugin $plugin)
$updater = new Updater();
$updater->markComponentSuccessfullyUpdated($plugin->getPluginName(), $plugin->getVersion());
$saveConfig = true;

/**
* Event triggered after a new plugin has been installed.
*
* @param string $pluginName The plugin that has been installed.
*/
Piwik::postEvent('PluginManager.pluginInstalled', array($plugin));
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

I reckon they are self explaining 👍

}

if ($saveConfig) {
Expand Down
9 changes: 9 additions & 0 deletions core/Updater.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -279,6 +279,15 @@ public function update($componentName)

$this->executeListenerHook('onComponentUpdateFinished', array($componentName, $updatedVersion, $warningMessages));

/**
* Event triggered after a component has been updated.
*
* @param string $componentName 'core', or plugin name
* @param string $updatedVersion version updated to
* @param array $warningMessages warnings occurred during update
*/
Piwik::postEvent('Updater.componentUpdated', array($componentName, $updatedVersion, $warningMessages));
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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 👍


return $warningMessages;
}

Expand Down