matomo-org / matomo

Empowering People Ethically with the leading open source alternative to Google Analytics that gives you full control over your data. Matomo lets you easily collect data from websites & apps and visualise this data and extract insights. Privacy is built-in. Liberating Web Analytics. Star us on Github? +1. And we love Pull Requests!
https://matomo.org/
GNU General Public License v3.0
19.9k stars 2.65k forks source link

Add console plugin:install command #22640

Closed jsantos42 closed 2 weeks ago

jsantos42 commented 1 month ago

Description:

Fixes #21003

Review

sgiehl commented 1 month ago

Hey @jsantos42. Thanks for providing this Pull Request. I had a rough look through the code and by the looks the new command actually doesn't fully do what would be expected. It only seems to create the plugin.json file(s), but doesn't fetch the actual plugin content.

jsantos42 commented 1 month ago

@sgiehl you were right, thank you for your feedback! Just refactored the code in order to add the remaining files of the plugin. Also added some PHPDoc notations. I do have one question: would you still keep the specific exception on line 53, given that I have to catch the no connection exception with a general exception?

michalkleiner commented 1 month ago

@jsantos42 thanks for putting the effort into this, it's very appreciated. Can I ask why do you think we need to write the plugin.json file first? Wouldn't the installation take care of that anyway?

jsantos42 commented 1 month ago

@michalkleiner my pleasure! For one simple reason: I had created a local bogus plugin with incompatible requirements to test if it would install. But, even though it would not install it, it was not outputting any error message; hence me fetching the json first and checking for missing requirements. But now I looked for an incompatible plugin on the marketplace (I used GoalConversionExport), and when trying to install it, I do get the error message with the PluginInstaller code, so I cleaned the redundant part. Thanks for the feedback!

jsantos42 commented 1 month ago

@sgiehl I fixed what you suggested. Also, I found out that with commit b90fd94 it stopped installing the plugin (or, at least, it would no longer show up in ./console plugin:list). I step-debugged the installation through the GUI marketplace and I found out that it also calls the pluginManager methods that I had removed on that commit. So I reintroduced them in 53e42d5.

jsantos42 commented 1 month ago

@tsteur I pushed the new changes that I took from your commits:

As for the name of the command:

Hence, I removed the check isPluginInstalled() and renamed the command to install-or-update. This outputs 'successfully installed' even if the plugin was already installed and up to date, which seems to be the behavior of other commands as well (e.g. if you try to uninstall a given plugin twice, you will get the same 'uninstalled successfully' message twice). Not ideal, I guess, but consistent with the existent codebase. And this way the command can also be used to update, as you had on your implementation!

tsteur commented 1 month ago

Awesome, thanks @jsantos42