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

Fixed bug with plugin status on install/uninstall #29

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ppalex7
Copy link
Contributor

@ppalex7 ppalex7 commented Jan 25, 2014

At 95ac85c plugin installation system was broken, and saves status of installation/deinstallation as 'success' in all cases. I fixed it.

Saving plugin status as 'installed'/'uninstalled' only if corresponding action finished properly
Also added messages for failed plugin installation.

Saving plugin status as 'installed'/'uninstalled' only if corresponding action finished properly
Also added messages for successful and failed plugin installation.
@ErikMinekus
Copy link
Member

I don't think this is necessary, as you can see in the Community Bans plugin, the new way is to throw an exception. The error message is then returned via AJAX. The documentation in SBPlugin just wasn't updated.

@ppalex7
Copy link
Contributor Author

ppalex7 commented Jan 28, 2014

Maybe, but why if I return false from runInstall - plugin will marked as installed?

@ppalex7
Copy link
Contributor Author

ppalex7 commented Jan 28, 2014

Why break something that already works?

Or if you think, that throwing exception - is only right way - you should edit documentation of SBPlugin model in the part of "return value of runInstall or runUninstall functions"

@ErikMinekus
Copy link
Member

Maybe, but why if I return false from runInstall - plugin will marked as installed?

Because it doesn't check for false anymore, it checks whether an exception was thrown.

Why break something that already works?

Sure, returning false worked, except that you had no idea what went wrong. Now you actually get an error message.

Or if you think, that throwing exception - is only right way - you should edit documentation of SBPlugin model in the part of "return value of runInstall or runUninstall functions"

I don't think it's the only right way, but for now it's the easiest way to stop the installation and return an error message. Especially since Yii also throws exceptions for SQL errors and such. Maybe in the future we'll want to return multiple error messages, then we can reconsider how it's implemented.

I updated the documentation in commit 7b145fb.

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.

2 participants