Merge remote-tracking branch 'refs/remotes/willkg/plugins-infrastructure-rewrite'

9 files changed, 205 insertions, 194 deletions

diff --git a/docs/source/pluginwriter/quickstart.rst b/docs/source/pluginwriter/quickstart.rst
index 73531381..b5a63f79 100644
--- a/docs/source/pluginwriter/quickstart.rst
+++ b/docs/source/pluginwriter/quickstart.rst
@@ -50,7 +50,8 @@ The inner ``sampleplugin`` directory is the Python package that holds
 your plugin's code.
 
 The ``__init__.py`` denotes that this is a Python package. It also
-holds the plugin code.
+holds the plugin code and the ``hooks`` dict that specifies which
+hooks the sampleplugin uses.
 
 
 Step 2: README
@@ -107,43 +108,39 @@ The code for ``__init__.py`` looks like this:
 
 .. code-block:: python
    :linenos:
-   :emphasize-lines: 8,19
+   :emphasize-lines: 12,23
 
     import logging
     from mediagoblin.tools.pluginapi import Plugin, get_config
 
 
+    # This creates a logger that you can use to log information to
+    # the console or a log file.
     _log = logging.getLogger(__name__)
 
 
-    class SamplePlugin(Plugin):
-        """
-        This is a sample plugin class. It automatically registers itself
-        with MediaGoblin when this module is imported.
+    # This is the function that gets called when the setup
+    # hook fires.
+    def setup_plugin():
+        _log.info("I've been started!")
+        config = get_config('sampleplugin')
+        if config:
+            _log.info('%r' % config)
+        else:
+            _log.info('There is no configuration set.')
 
-        The setup_plugin method prints configuration for this plugin if
-        there is any.
-        """
-        def __init__(self):
-            pass
 
-        def setup_plugin(self):
-            _log.info("I've been started!")
-            config = get_config('sampleplugin')
-            if config:
-                _log.info('%r' % config)
-            else:
-                _log.info('There is no configuration set.')
+    # This is a dict that specifies which hooks this plugin uses.
+    # This one only uses one hook: setup.
+    hooks = {
+        'setup': setup_plugin
+        }
 
 
-Line 8 defines a class called ``SamplePlugin`` that subclasses
-``Plugin`` from ``mediagoblin.tools.pluginapi``. When the class is
-defined, it gets registered with MediaGoblin and MediaGoblin will then
-call ``setup_plugin`` on it.
+Line 12 defines the ``setup_plugin`` function.
 
-Line 19 defines ``setup_plugin``. This gets called when MediaGoblin
-starts up after it's registered all the plugins. This is where you can
-do any initialization for your plugin.
+Line 23 defines ``hooks``. When MediaGoblin loads this file, it sees
+``hooks`` and registers all the callables with their respective hooks.
 
 
 Step 6: Installation and configuration
diff --git a/docs/source/siteadmin/plugins.rst b/docs/source/siteadmin/plugins.rst
index 41f2970f..f5a78da7 100644
--- a/docs/source/siteadmin/plugins.rst
+++ b/docs/source/siteadmin/plugins.rst
@@ -135,3 +135,35 @@ For plugins that you install with pip, you can upgrade them with pip::
     pip install -U <plugin-name>
 
 The ``-U`` tells pip to upgrade the package.
+
+
+Troubleshooting plugins
+=======================
+
+Sometimes plugins just don't work right. When you're having problems
+with plugins, think about the following:
+
+1. Check the log files.
+
+   Some plugins will log errors to the log files and you can use that
+   to diagnose the problem.
+
+2. Try running MediaGoblin without that plugin.
+
+   It's easy to disable a plugin from MediaGoblin. Add a ``-`` to the
+   name in your config file.
+
+   For example, change::
+
+       [[mediagoblin.plugins.flatpages]]
+
+   to::
+
+       [[-mediagoblin.plugins.flatpages]]
+
+   That'll prevent the ``mediagoblin.plugins.flatpages`` plugin from
+   loading.
+
+3. If it's a core plugin that comes with MediaGoblin, ask us for help!
+
+   If it's a plugin you got from somewhere else, ask them for help!
diff --git a/mediagoblin/app.py b/mediagoblin/app.py
index 9ef23ce8..51f5899a 100644
--- a/mediagoblin/app.py
+++ b/mediagoblin/app.py
@@ -32,7 +32,7 @@ from mediagoblin.init.plugins import setup_plugins
 from mediagoblin.init import (get_jinja_loader, get_staticdirector,
     setup_global_and_app_config, setup_workbench, setup_database,
     setup_storage, setup_beaker_cache)
-from mediagoblin.tools.pluginapi import PluginCache
+from mediagoblin.tools.pluginapi import PluginManager
 
 
 _log = logging.getLogger(__name__)
@@ -82,14 +82,14 @@ class MediaGoblinApp(object):
         self.template_loader = get_jinja_loader(
             app_config.get('local_templates'),
             self.current_theme,
-            PluginCache().get_template_paths()
+            PluginManager().get_template_paths()
             )
 
         # Set up storage systems
         self.public_store, self.queue_store = setup_storage()
 
         # set up routing
-        self.routing = routing.get_mapper(PluginCache().get_routes())
+        self.routing = routing.get_mapper(PluginManager().get_routes())
 
         # set up staticdirector tool
         self.staticdirector = get_staticdirector(app_config)
diff --git a/mediagoblin/init/plugins/__init__.py b/mediagoblin/init/plugins/__init__.py
index 279a5ede..cdf9b5ad 100644
--- a/mediagoblin/init/plugins/__init__.py
+++ b/mediagoblin/init/plugins/__init__.py
@@ -15,6 +15,7 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
 import logging
+import sys
 
 from mediagoblin import mg_globals
 from mediagoblin.tools import pluginapi
@@ -36,24 +37,28 @@ def setup_plugins():
         _log.info("No plugins to load")
         return
 
-    pcache = pluginapi.PluginCache()
+    pman = pluginapi.PluginManager()
 
     # Go through and import all the modules that are subsections of
-    # the [plugins] section.
+    # the [plugins] section and read in the hooks.
     for plugin_module, config in plugin_section.items():
+        # Skip any modules that start with -. This makes it easier for
+        # someone to tweak their configuration so as to not load a
+        # plugin without having to remove swaths of plugin
+        # configuration.
+        if plugin_module.startswith('-'):
+            continue
+
         _log.info("Importing plugin module: %s" % plugin_module)
+        pman.register_plugin(plugin_module)
         # If this throws errors, that's ok--it'll halt mediagoblin
         # startup.
         __import__(plugin_module)
+        plugin = sys.modules[plugin_module]
+        if hasattr(plugin, 'hooks'):
+            pman.register_hooks(plugin.hooks)
 
-    # Note: One side-effect of importing things is that anything that
-    # subclassed pluginapi.Plugin is registered.
-
-    # Go through all the plugin classes, instantiate them, and call
-    # setup_plugin so they can figure things out.
-    for plugin_class in pcache.plugin_classes:
-        name = plugin_class.__module__ + "." + plugin_class.__name__
-        _log.info("Loading plugin: %s" % name)
-        plugin_obj = plugin_class()
-        plugin_obj.setup_plugin()
-        pcache.register_plugin_object(plugin_obj)
+    # Execute anything registered to the setup hook.
+    setup_list = pman.get_hook_callables('setup')
+    for fun in setup_list:
+        fun()
diff --git a/mediagoblin/plugins/flatpagesfile/__init__.py b/mediagoblin/plugins/flatpagesfile/__init__.py
index 9ed26102..b9b52012 100644
--- a/mediagoblin/plugins/flatpagesfile/__init__.py
+++ b/mediagoblin/plugins/flatpagesfile/__init__.py
@@ -53,27 +53,27 @@ def flatpage_handler_builder(template):
     return _flatpage_handler_builder
 
 
-class FlatpagesFilePlugin(pluginapi.Plugin):
-    """
-    This is the flatpages plugin class. See the README for how to use
-    flatpages.
-    """
-    def setup_plugin(self):
-        self.config = pluginapi.get_config('mediagoblin.plugins.flatpagesfile')
+def setup_plugin():
+    config = pluginapi.get_config('mediagoblin.plugins.flatpagesfile')
+
+    _log.info('Setting up flatpagesfile....')
+
+    # Register the template path.
+    pluginapi.register_template_path(os.path.join(PLUGIN_DIR, 'templates'))
 
-        _log.info('Setting up flatpagesfile....')
+    pages = config.items()
 
-        # Register the template path.
-        pluginapi.register_template_path(os.path.join(PLUGIN_DIR, 'templates'))
+    routes = []
+    for name, (url, template) in pages:
+        name = 'flatpagesfile.%s' % name.strip()
+        controller = flatpage_handler_builder(template)
+        routes.append(
+            Route(name, url, controller=controller))
 
-        pages = self.config.items()
+    pluginapi.register_routes(routes)
+    _log.info('Done setting up flatpagesfile!')
 
-        routes = []
-        for name, (url, template) in pages:
-            name = 'flatpagesfile.%s' % name.strip()
-            controller = flatpage_handler_builder(template)
-            routes.append(
-                Route(name, url, controller=controller))
 
-        pluginapi.register_routes(routes)
-        _log.info('Done setting up flatpagesfile!')
+hooks = {
+    'setup': setup_plugin
+    }
diff --git a/mediagoblin/plugins/sampleplugin/__init__.py b/mediagoblin/plugins/sampleplugin/__init__.py
index b87348af..2cd077a2 100644
--- a/mediagoblin/plugins/sampleplugin/__init__.py
+++ b/mediagoblin/plugins/sampleplugin/__init__.py
@@ -15,6 +15,28 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
 
-# This imports the module that has the Plugin subclass in it which
-# causes that module to get imported and that class to get registered.
-import mediagoblin.plugins.sampleplugin.main
+import logging
+
+from mediagoblin.tools.pluginapi import get_config
+
+
+_log = logging.getLogger(__name__)
+
+
+_setup_plugin_called = 0
+
+def setup_plugin():
+    global _setup_plugin_called
+
+    _log.info('Sample plugin set up!')
+    config = get_config('mediagoblin.plugins.sampleplugin')
+    if config:
+        _log.info('%r' % config)
+    else:
+        _log.info('There is no configuration set.')
+    _setup_plugin_called += 1
+
+
+hooks = {
+    'setup': setup_plugin
+    }
diff --git a/mediagoblin/plugins/sampleplugin/main.py b/mediagoblin/plugins/sampleplugin/main.py
deleted file mode 100644
index 67cc70a5..00000000
--- a/mediagoblin/plugins/sampleplugin/main.py
+++ /dev/null
@@ -1,42 +0,0 @@
-# GNU MediaGoblin -- federated, autonomous media hosting
-# Copyright (C) 2011, 2012 MediaGoblin contributors.  See AUTHORS.
-#
-# This program is free software: you can redistribute it and/or modify
-# it under the terms of the GNU Affero General Public License as published by
-# the Free Software Foundation, either version 3 of the License, or
-# (at your option) any later version.
-#
-# This program is distributed in the hope that it will be useful,
-# but WITHOUT ANY WARRANTY; without even the implied warranty of
-# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-# GNU Affero General Public License for more details.
-#
-# You should have received a copy of the GNU Affero General Public License
-# along with this program.  If not, see <http://www.gnu.org/licenses/>.
-
-import logging
-from mediagoblin.tools.pluginapi import Plugin, get_config
-
-
-_log = logging.getLogger(__name__)
-
-
-class SamplePlugin(Plugin):
-    """
-    This is a sample plugin class. It automatically registers itself
-    with mediagoblin when this module is imported.
-
-    The setup_plugin method prints configuration for this plugin if
-    it exists.
-    """
-    def __init__(self):
-        self._setup_plugin_called = 0
-
-    def setup_plugin(self):
-        _log.info('Sample plugin set up!')
-        config = get_config('mediagoblin.plugins.sampleplugin')
-        if config:
-            _log.info('%r' % config)
-        else:
-            _log.info('There is no configuration set.')
-        self._setup_plugin_called += 1
diff --git a/mediagoblin/tests/test_pluginapi.py b/mediagoblin/tests/test_pluginapi.py
index c5c614f6..315a95da 100644
--- a/mediagoblin/tests/test_pluginapi.py
+++ b/mediagoblin/tests/test_pluginapi.py
@@ -37,8 +37,8 @@ def with_cleanup(*modules_to_delete):
                     pass
             # The plugin cache gets populated as a side-effect of
             # importing, so it's best to clear it before and after a test.
-            pcache = pluginapi.PluginCache()
-            pcache.clear()
+            pman = pluginapi.PluginManager()
+            pman.clear()
             try:
                 return fun(*args, **kwargs)
             finally:
@@ -51,7 +51,7 @@ def with_cleanup(*modules_to_delete):
                         del sys.modules[module]
                     except KeyError:
                         pass
-                pcache.clear()
+                pman.clear()
 
         _with_cleanup_inner.__name__ = fun.__name__
         return _with_cleanup_inner
@@ -93,16 +93,14 @@ def test_no_plugins():
     mg_globals.app_config = cfg['mediagoblin']
     mg_globals.global_config = cfg
 
-    pcache = pluginapi.PluginCache()
+    pman = pluginapi.PluginManager()
     setup_plugins()
 
     # Make sure we didn't load anything.
-    eq_(len(pcache.plugin_classes), 0)
-    eq_(len(pcache.plugin_objects), 0)
+    eq_(len(pman.plugins), 0)
 
 
-@with_cleanup('mediagoblin.plugins.sampleplugin',
-              'mediagoblin.plugins.sampleplugin.main')
+@with_cleanup('mediagoblin.plugins.sampleplugin')
 def test_one_plugin():
     """Run setup_plugins with a single working plugin"""
     cfg = build_config([
@@ -115,22 +113,21 @@ def test_one_plugin():
     mg_globals.app_config = cfg['mediagoblin']
     mg_globals.global_config = cfg
 
-    pcache = pluginapi.PluginCache()
+    pman = pluginapi.PluginManager()
     setup_plugins()
 
-    # Make sure we only found one plugin class
-    eq_(len(pcache.plugin_classes), 1)
-    # Make sure the class is the one we think it is.
-    eq_(pcache.plugin_classes[0].__name__, 'SamplePlugin')
+    # Make sure we only found one plugin
+    eq_(len(pman.plugins), 1)
+    # Make sure the plugin is the one we think it is.
+    eq_(pman.plugins[0], 'mediagoblin.plugins.sampleplugin')
+    # Make sure there was one hook registered
+    eq_(len(pman.hooks), 1)
+    # Make sure _setup_plugin_called was called once
+    import mediagoblin.plugins.sampleplugin
+    eq_(mediagoblin.plugins.sampleplugin._setup_plugin_called, 1)
 
-    # Make sure there was one plugin created
-    eq_(len(pcache.plugin_objects), 1)
-    # Make sure we called setup_plugin on SamplePlugin
-    eq_(pcache.plugin_objects[0]._setup_plugin_called, 1)
 
-
-@with_cleanup('mediagoblin.plugins.sampleplugin',
-              'mediagoblin.plugins.sampleplugin.main')
+@with_cleanup('mediagoblin.plugins.sampleplugin')
 def test_same_plugin_twice():
     """Run setup_plugins with a single working plugin twice"""
     cfg = build_config([
@@ -144,15 +141,35 @@ def test_same_plugin_twice():
     mg_globals.app_config = cfg['mediagoblin']
     mg_globals.global_config = cfg
 
-    pcache = pluginapi.PluginCache()
+    pman = pluginapi.PluginManager()
     setup_plugins()
 
-    # Make sure we only found one plugin class
-    eq_(len(pcache.plugin_classes), 1)
-    # Make sure the class is the one we think it is.
-    eq_(pcache.plugin_classes[0].__name__, 'SamplePlugin')
+    # Make sure we only found one plugin
+    eq_(len(pman.plugins), 1)
+    # Make sure the plugin is the one we think it is.
+    eq_(pman.plugins[0], 'mediagoblin.plugins.sampleplugin')
+    # Make sure there was one hook registered
+    eq_(len(pman.hooks), 1)
+    # Make sure _setup_plugin_called was called once
+    import mediagoblin.plugins.sampleplugin
+    eq_(mediagoblin.plugins.sampleplugin._setup_plugin_called, 1)
+
+
+@with_cleanup()
+def test_disabled_plugin():
+    """Run setup_plugins with a single working plugin twice"""
+    cfg = build_config([
+            ('mediagoblin', {}, []),
+            ('plugins', {}, [
+                    ('-mediagoblin.plugins.sampleplugin', {}, []),
+                    ])
+            ])
+
+    mg_globals.app_config = cfg['mediagoblin']
+    mg_globals.global_config = cfg
+
+    pman = pluginapi.PluginManager()
+    setup_plugins()
 
-    # Make sure there was one plugin created
-    eq_(len(pcache.plugin_objects), 1)
-    # Make sure we called setup_plugin on SamplePlugin
-    eq_(pcache.plugin_objects[0]._setup_plugin_called, 1)
+    # Make sure we didn't load the plugin
+    eq_(len(pman.plugins), 0)
diff --git a/mediagoblin/tools/pluginapi.py b/mediagoblin/tools/pluginapi.py
index 0c338540..bf3775d5 100644
--- a/mediagoblin/tools/pluginapi.py
+++ b/mediagoblin/tools/pluginapi.py
@@ -15,8 +15,7 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
 """
-This module implements the plugin api bits and provides the plugin
-base.
+This module implements the plugin api bits.
 
 Two things about things in this module:
 
@@ -30,8 +29,8 @@ How do plugins work?
 ====================
 
 Plugins are structured like any Python project. You create a Python package.
-In that package, you define a high-level ``__init__.py`` that either defines
-or imports modules that define classes that inherit from the ``Plugin`` class.
+In that package, you define a high-level ``__init__.py`` module that has a 
+``hooks`` dict that maps hooks to callables that implement those hooks.
 
 Additionally, you want a LICENSE file that specifies the license and a
 ``setup.py`` that specifies the metadata for packaging your plugin. A rough
@@ -42,23 +41,19 @@ file structure could look like this::
      |- README           # holds plugin project information
      |- LICENSE          # holds license information
      |- myplugin/        # plugin package directory
-        |- __init__.py   # imports myplugin.main
-        |- main.py       # code for plugin
+        |- __init__.py   # has hooks dict and code
 
 
 Lifecycle
 =========
 
 1. All the modules listed as subsections of the ``plugins`` section in
-   the config file are imported. This causes any ``Plugin`` subclasses in
-   those modules to be defined and when the classes are defined they get
-   automatically registered with the ``PluginCache``.
+   the config file are imported. MediaGoblin registers any hooks in
+   the ``hooks`` dict of those modules.
 
-2. After all plugin modules are imported, registered plugin classes are
-   instantiated and ``setup_plugin`` is called for each plugin object.
+2. After all plugin modules are imported, the ``setup`` hook is called
+   allowing plugins to do any set up they need to do.
 
-   Plugins can do any setup they need to do in their ``setup_plugin``
-   method.
 """
 
 import logging
@@ -69,14 +64,19 @@ from mediagoblin import mg_globals
 _log = logging.getLogger(__name__)
 
 
-class PluginCache(object):
-    """Cache of plugin things"""
+class PluginManager(object):
+    """Manager for plugin things
+
+    .. Note::
+
+       This is a Borg class--there is one and only one of this class.
+    """
     __state = {
         # list of plugin classes
-        "plugin_classes": [],
+        "plugins": [],
 
-        # list of plugin objects
-        "plugin_objects": [],
+        # map of hook names -> list of callables for that hook
+        "hooks": {},
 
         # list of registered template paths
         "template_paths": set(),
@@ -87,19 +87,31 @@ class PluginCache(object):
 
     def clear(self):
         """This is only useful for testing."""
-        del self.plugin_classes[:]
-        del self.plugin_objects[:]
+        # Why lists don't have a clear is not clear.
+        del self.plugins[:]
+        del self.routes[:]
+        self.hooks.clear()
+        self.template_paths.clear()
 
     def __init__(self):
         self.__dict__ = self.__state
 
-    def register_plugin_class(self, plugin_class):
+    def register_plugin(self, plugin):
         """Registers a plugin class"""
-        self.plugin_classes.append(plugin_class)
+        self.plugins.append(plugin)
+
+    def register_hooks(self, hook_mapping):
+        """Takes a hook_mapping and registers all the hooks"""
+        for hook, callables in hook_mapping.items():
+            if isinstance(callables, (list, tuple)):
+                self.hooks.setdefault(hook, []).extend(list(callables))
+            else:
+                # In this case, it's actually a single callable---not a
+                # list of callables.
+                self.hooks.setdefault(hook, []).append(callables)
 
-    def register_plugin_object(self, plugin_obj):
-        """Registers a plugin object"""
-        self.plugin_objects.append(plugin_obj)
+    def get_hook_callables(self, hook_name):
+        return self.hooks.get(hook_name, [])
 
     def register_template_path(self, path):
         """Registers a template path"""
@@ -117,38 +129,6 @@ class PluginCache(object):
         return tuple(self.routes)
 
 
-class MetaPluginClass(type):
-    """Metaclass for PluginBase derivatives"""
-    def __new__(cls, name, bases, attrs):
-        new_class = super(MetaPluginClass, cls).__new__(cls, name, bases, attrs)
-        parents = [b for b in bases if isinstance(b, MetaPluginClass)]
-        if not parents:
-            return new_class
-        PluginCache().register_plugin_class(new_class)
-        return new_class
-
-
-class Plugin(object):
-    """Extend this class for plugins.
-
-    Example::
-
-        from mediagoblin.tools.pluginapi import Plugin
-
-        class MyPlugin(Plugin):
-            ...
-
-            def setup_plugin(self):
-                ....
-
-    """
-
-    __metaclass__ = MetaPluginClass
-
-    def setup_plugin(self):
-        pass
-
-
 def register_routes(routes):
     """Registers one or more routes
 
@@ -182,9 +162,9 @@ def register_routes(routes):
     """
     if isinstance(routes, (tuple, list)):
         for route in routes:
-            PluginCache().register_route(route)
+            PluginManager().register_route(route)
     else:
-        PluginCache().register_route(routes)
+        PluginManager().register_route(routes)
 
 
 def register_template_path(path):
@@ -205,7 +185,7 @@ def register_template_path(path):
        that will have no effect on template loading.
 
     """
-    PluginCache().register_template_path(path)
+    PluginManager().register_template_path(path)
 
 
 def get_config(key):