From 7691078ce5ec8a0af5ab4dd9eb7ff40317ed6862 Mon Sep 17 00:00:00 2001 From: Azareal Date: Sun, 29 Jul 2018 23:02:48 +1000 Subject: [PATCH] Fixed a bug in AddHook where the indices of the hooks would be off by one. RemoveHook now panics under conditions which should normally never happen, but which when it does, gives off a far more useful message. Changed the order of the conditional after BypassActive in PluginsDeactivate, so the error message will show up now. The deactivation handler is optional again for plugins. Added more tests for the plugin system. --- common/extend.go | 28 ++++++++++++++++++++-------- misc_test.go | 23 +++++++++++++++++++++++ plugin_bbcode.go | 1 - routes/panel/plugins.go | 11 +++++++---- 4 files changed, 50 insertions(+), 13 deletions(-) diff --git a/common/extend.go b/common/extend.go index 2b8470a3..c99f62dd 100644 --- a/common/extend.go +++ b/common/extend.go @@ -309,7 +309,7 @@ func (plugin *Plugin) AddHook(name string, handler interface{}) { } else { Hooks[name] = append(Hooks[name], h) } - plugin.Hooks[name] = len(Hooks[name]) + plugin.Hooks[name] = len(Hooks[name]) - 1 case func(string) string: if len(Sshooks[name]) == 0 { var hookSlice []func(string) string @@ -318,7 +318,7 @@ func (plugin *Plugin) AddHook(name string, handler interface{}) { } else { Sshooks[name] = append(Sshooks[name], h) } - plugin.Hooks[name] = len(Sshooks[name]) + plugin.Hooks[name] = len(Sshooks[name]) - 1 case func(http.ResponseWriter, *http.Request, *User, interface{}) bool: if len(PreRenderHooks[name]) == 0 { var hookSlice []func(http.ResponseWriter, *http.Request, *User, interface{}) bool @@ -327,7 +327,7 @@ func (plugin *Plugin) AddHook(name string, handler interface{}) { } else { PreRenderHooks[name] = append(PreRenderHooks[name], h) } - plugin.Hooks[name] = len(PreRenderHooks[name]) + plugin.Hooks[name] = len(PreRenderHooks[name]) - 1 case func() error: // ! We might want a more generic name, as we might use this signature for things other than tasks hooks if len(taskHooks[name]) == 0 { var hookSlice []func() error @@ -336,7 +336,7 @@ func (plugin *Plugin) AddHook(name string, handler interface{}) { } else { taskHooks[name] = append(taskHooks[name], h) } - plugin.Hooks[name] = len(taskHooks[name]) + plugin.Hooks[name] = len(taskHooks[name]) - 1 case func(...interface{}) interface{}: Vhooks[name] = h plugin.Hooks[name] = 0 @@ -353,7 +353,10 @@ func (plugin *Plugin) AddHook(name string, handler interface{}) { func (plugin *Plugin) RemoveHook(name string, handler interface{}) { switch handler.(type) { case func(interface{}) interface{}: - key := plugin.Hooks[name] + key, ok := plugin.Hooks[name] + if !ok { + panic("handler not registered as hook") + } hook := Hooks[name] if len(hook) == 1 { hook = []func(interface{}) interface{}{} @@ -362,7 +365,10 @@ func (plugin *Plugin) RemoveHook(name string, handler interface{}) { } Hooks[name] = hook case func(string) string: - key := plugin.Hooks[name] + key, ok := plugin.Hooks[name] + if !ok { + panic("handler not registered as hook") + } hook := Sshooks[name] if len(hook) == 1 { hook = []func(string) string{} @@ -371,7 +377,10 @@ func (plugin *Plugin) RemoveHook(name string, handler interface{}) { } Sshooks[name] = hook case func(http.ResponseWriter, *http.Request, *User, interface{}) bool: - key := plugin.Hooks[name] + key, ok := plugin.Hooks[name] + if !ok { + panic("handler not registered as hook") + } hook := PreRenderHooks[name] if len(hook) == 1 { hook = []func(http.ResponseWriter, *http.Request, *User, interface{}) bool{} @@ -380,7 +389,10 @@ func (plugin *Plugin) RemoveHook(name string, handler interface{}) { } PreRenderHooks[name] = hook case func() error: - key := plugin.Hooks[name] + key, ok := plugin.Hooks[name] + if !ok { + panic("handler not registered as hook") + } hook := taskHooks[name] if len(hook) == 1 { hook = []func() error{} diff --git a/misc_test.go b/misc_test.go index 9dcc0483..cf69a86f 100644 --- a/misc_test.go +++ b/misc_test.go @@ -934,6 +934,29 @@ func TestPluginManager(t *testing.T) { hasPlugin, err = plugin.InDatabase() expectNilErr(t, err) expect(t, hasPlugin, "Plugin bbcode should still exist in the database") + + // Bugs sometimes arise when we try to delete a hook when there are multiple, so test for that + // TODO: Do a finer grained test for that case...? A bigger test might catch more odd cases with multiple plugins + plugin2, ok := common.Plugins["markdown"] + expect(t, ok, "Plugin markdown should exist") + expect(t, !plugin2.Installable, "Plugin markdown shouldn't be installable") + expect(t, !plugin2.Installed, "Plugin markdown shouldn't be 'installed'") + expect(t, !plugin2.Active, "Plugin markdown shouldn't be active") + active, err = plugin2.BypassActive() + expectNilErr(t, err) + expect(t, !active, "Plugin markdown shouldn't be active in the database either") + hasPlugin, err = plugin2.InDatabase() + expectNilErr(t, err) + expect(t, !hasPlugin, "Plugin markdown shouldn't exist in the database") + + expectNilErr(t, plugin2.AddToDatabase(true, false)) + expectNilErr(t, plugin2.Init()) + expectNilErr(t, plugin.SetActive(true)) + expectNilErr(t, plugin.Init()) + plugin2.Deactivate() + expectNilErr(t, plugin2.SetActive(false)) + plugin.Deactivate() + expectNilErr(t, plugin.SetActive(false)) } func TestSlugs(t *testing.T) { diff --git a/plugin_bbcode.go b/plugin_bbcode.go index 3f8a9da5..f00b990e 100644 --- a/plugin_bbcode.go +++ b/plugin_bbcode.go @@ -29,7 +29,6 @@ func init() { } func initBbcode() error { - //plugins["bbcode"].AddHook("parse_assign", bbcode_parse_without_code) common.Plugins["bbcode"].AddHook("parse_assign", bbcodeFullParse) bbcodeInvalidNumber = []byte("[Invalid Number]") diff --git a/routes/panel/plugins.go b/routes/panel/plugins.go index 7986a63e..a0122a6b 100644 --- a/routes/panel/plugins.go +++ b/routes/panel/plugins.go @@ -95,19 +95,22 @@ func PluginsDeactivate(w http.ResponseWriter, r *http.Request, user common.User, if !ok { return common.LocalError("The plugin isn't registered in the system", w, r, user) } + log.Printf("plugin: %+v\n", plugin) active, err := plugin.BypassActive() - if !active { - return common.LocalError("The plugin you're trying to deactivate isn't active", w, r, user) - } else if err != nil { + if err != nil { return common.InternalError(err, w, r) + } else if !active { + return common.LocalError("The plugin you're trying to deactivate isn't active", w, r, user) } err = plugin.SetActive(false) if err != nil { return common.InternalError(err, w, r) } - plugin.Deactivate() + if plugin.Deactivate != nil { + plugin.Deactivate() + } http.Redirect(w, r, "/panel/plugins/", http.StatusSeeOther) return nil