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

@@ -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{}

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) {

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("<span style='color: red;'>[Invalid Number]</span>")

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