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 ability to disable plugins #453

Merged
merged 3 commits into from Dec 5, 2016
Merged

Add ability to disable plugins #453

merged 3 commits into from Dec 5, 2016

Conversation

pupi1985
Copy link
Contributor

@pupi1985 pupi1985 commented Oct 17, 2016

I think the subject says it all. There are a few things to bear in mind, though.

  • The code is a bit dirty and undocumented (writing the doc takes almost the same time as developing this so I'm waiting for some feedback before doing that).
  • Plugins that are disabled should not even register because if they do so they'd execute the qa-plugin.php code and the main idea is not to execute that file. This generates a possibly undesired behaviour when showing plugin specific options as the "options" link comes from executing the plugin's code.
  • Additionally, some code had to be slightly reordered so that the init_queries function doesn't get called either.
  • In order to avoid the object creation overhead in qa_admin_plugin_directory_hash, the getHashesForPlugins method could be made static. But as this complicates testing and doesn't go along dependency injection I kept it that way. It could also be made singleton but still I didn't like the idea, at least until we have an IoC container (in Q2a 3.0, of course) :)
  • 'qa_opt' does not allow to check for the presence of a value without creating the key/title (this should be changed IMO). So maybe the initialization of the enabled_plugins option might be executed during database initialization. I haven't implemented this yet either.

screenshot

@svivian
Copy link
Collaborator

svivian commented Oct 18, 2016

Cool, this is a feature I wanted to add at some point. Code looks fine from first glance, hopefully should have time to check and merge in the next week or so.

'qa_opt' does not allow to check for the presence of a value without creating the key/title (this should be changed IMO)

Yes, I agree. This actually caused some issues a little while ago with a plugin that was checking for unique options for every question (e.g. my_plugin_thing_1234, my_plugin_other_1234, ...)

@svivian
Copy link
Collaborator

svivian commented Nov 25, 2016

Would you be able to rebase this against the latest version of the 1.8 branch? I've merged in some of your other PRs and it now conflicts.

BTW it would probably be easier if we stick to the 1.8 branch for now instead of dev. I'm not planning to release a v1.7.5 (unless there is an urgent security issue) so it will help avoid conflicts.

@pupi1985
Copy link
Contributor Author

Rebased. Also added a silly thing I missed: Make sure all plugins are enabled after upgrade.

I wasn't sure about the plan to go on or not with a 1.7.5 release... now I think this should have gone to 1.8 from the beginning anyways. I think I have PRd a few bugfixes to the dev branch but nothing critical and should be easy to merge or rebase, if needed.

BTW, while looking at the new database version (which I'm adding only because I know users are already using code from the 1.8 branch) I noticed this:

//	Up to here: Version 1.8 alpha

I wasn't sure whether the current 1.8 branch is already the 1.8 alpha version or if the first official 1.8 release will be the alpha. I assumed the latter, but feel free to move the comment before the db version update if I was wrong.

@svivian svivian merged commit aec53ab into q2a:1.8 Dec 5, 2016
@svivian
Copy link
Collaborator

svivian commented Dec 5, 2016

Thanks for this! In reply to a few of your points:

This generates a possibly undesired behaviour when showing plugin specific options as the "options" link comes from executing the plugin's code.

Yes, it might be handy in some situations to be able to change options while a plugin is disabled. In theory we could split out the "admin" parts of a plugin, and then we could activate those separately somehow. Though it might not be possible, since the admin form could require initialisation or something else first.

I wasn't sure whether the current 1.8 branch is already the 1.8 alpha version or if the first official 1.8 release will be the alpha

Basically, the "repo version" is the alpha (i.e. the latest commit on the 1.8 branch). Any actual release would normally be a beta. So you've done the right thing there. When we release v1.8 beta that comment would change accordingly, then any future database migrations (1.8 final or 1.9) would come below.

That whole system could probably use some cleaning up. Currently it has every migration from the very first release version, so in theory if you were on v1.0 you could directly install v1.8 and it would migrate fully. I'm not sure that's a case we need to worry about - we could probably keep just the last 2-3 versions worth of migrations without trouble.

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.

None yet

2 participants