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

Add console plugin:install command #22640

Open
wants to merge 16 commits into
base: 5.x-dev
Choose a base branch
from

Conversation

jsantos42
Copy link

Description:

Fixes #21003

Review

@sgiehl
Copy link
Member

sgiehl commented Oct 7, 2024

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 jsantos42 force-pushed the add-console-plugin-install branch 2 times, most recently from c081c25 to f78f05c Compare October 9, 2024 09:49
@jsantos42
Copy link
Author

jsantos42 commented Oct 9, 2024

@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
Copy link
Contributor

@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
Copy link
Author

@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!

Copy link
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

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

Left some minor comments. Otherwise the command generally works as expected now.

plugins/CorePluginsAdmin/Commands/InstallPlugin.php Outdated Show resolved Hide resolved
plugins/CorePluginsAdmin/Commands/InstallPlugin.php Outdated Show resolved Hide resolved
plugins/CorePluginsAdmin/Commands/InstallPlugin.php Outdated Show resolved Hide resolved
@jsantos42
Copy link
Author

jsantos42 commented Oct 10, 2024

@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
Copy link
Author

jsantos42 commented Oct 11, 2024

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

  • added the config writability check
  • added DI to get the PluginInstaller
  • updated the changelog

As for the name of the command:

  • the way you implemented I believe it will always give positive feedback to the user, outputting 'successfully installed' even if it didn't do a thing (i.e. if the plugin was already installed and up to date)
  • the way had I implemented, it checks if the plugin is installed and informs the user that it is already installed. The con: it doesn't update the plugin because it moves to the next loop iteration as soon as it finds it in the filesystem

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!

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

Successfully merging this pull request may close these issues.

Add console plugin:install
3 participants