From 05e007c1dbe7b5b8a092f1a99ed361c4e6b71f26 Mon Sep 17 00:00:00 2001 From: Will Kahn-Greene Date: Tue, 17 Jul 2012 21:02:12 -0400 Subject: [PATCH] Rework plugin infrastructure to nix side-effects This reworks the plugin infrastructure so as to remove module-loading side-effects which were making things a pain in the ass to test. With the new system, there's no auto-registering meta class. Instead plugins do whatever they want and then specify a hooks dict that maps hook names to callables for the things they're tying into. The most common one (and the only one we've implemented so far) is "setup". This also simplifies the sampleplugin a little by moving the code to __init__.py. --- docs/source/pluginwriter/quickstart.rst | 47 ++++---- mediagoblin/app.py | 6 +- mediagoblin/init/plugins/__init__.py | 26 +++-- mediagoblin/plugins/flatpagesfile/__init__.py | 38 +++---- mediagoblin/plugins/sampleplugin/__init__.py | 28 ++++- mediagoblin/plugins/sampleplugin/main.py | 42 -------- mediagoblin/tests/test_pluginapi.py | 57 +++++----- mediagoblin/tools/pluginapi.py | 100 +++++++----------- 8 files changed, 148 insertions(+), 196 deletions(-) delete mode 100644 mediagoblin/plugins/sampleplugin/main.py 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/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..4ac7a140 100644 --- a/mediagoblin/init/plugins/__init__.py +++ b/mediagoblin/init/plugins/__init__.py @@ -15,6 +15,7 @@ # along with this program. If not, see . import logging +import sys from mediagoblin import mg_globals from mediagoblin.tools import pluginapi @@ -36,24 +37,21 @@ 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(): _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) - - # 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) + plugin = sys.modules[plugin_module] + if hasattr(plugin, 'hooks'): + pman.register_hooks(plugin.hooks) + + # 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 . -# 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 . - -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..8c9c6a04 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,15 @@ 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 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 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) 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 . """ -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): -- 2.25.1