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

V1 plugin architecture #9203

Closed
wants to merge 32 commits into from
Closed

Conversation

gdpelican
Copy link
Contributor

Hello again Mastodon!

In reference to #7717, I've been chipping away at a plugin architecture over the past while, which will allow people to write their own functionality into Mastodon without forking the repo. Here's a first look at some real simple (but powerful) things you can do with it:
mastodon1
mastodon2

I've written some basic instructions on usage into the api file itself (and will write some more as this matures a bit):
https://github.com/gdpelican/mastodon/blob/plugin-arch/lib/mastodon/plugin.rb#L42

For the video inclined, check out this mildly long (4:30) explainer video with a bit more of a demonstration of what this thing can do at the moment:
https://www.useloom.com/share/0a4643867f3a42c38c56e48fe3b2c008

# example usage: `use_route(:get, "examples/:id", "examples#show")`
# ^^ create an endpoint at /api/v1/examples/:id which routes to the show action on ExamplesController
# NB: be sure to define a controller action by adding a controller or using the 'extend_class' option!
def use_route(verb, route, action)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if it was something like a in_router method that takes a block and executes it in the router context, thereby allowing the plugin to use the Rails routes API fully?


# defined by the plugin
def setup!
raise NotImplementedError.new, "Please define a setup! method"
Copy link
Member

@Gargron Gargron Nov 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't assets and paths be defined using class methods instead of requiring them to be executed inside this method? E.g.:

class MyPlugin < Mastodon::Plugin
  translation '/some/path'

  routes do
    get :my_extra_route, to: 'my_controller#index'
  end
end

However, unlike setup, those wouldn't immediately execute anything outside of the plugin. You would still have an internal setup method that would be run only if the plugin is activated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, happy to do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The methods as is don't actually run anything either, except to define a Proc which gets queued up in a list of 'stuff to do' when the plugin gets installed.

This means that we could introduce a concept of uninstalled or disabled plugins, which would still be evaluated but don't actually apply any changes to core unless they're marked as 'installed' - one thing I think would be good to include in an early iteration is a 'safe mode'

foreman start --safe
// or maybe
foreman start --disable-plugins example
foreman start --disable-plugins all

which would allow admins to more easily diagnose whether issues they might be having are because of some installed plugin or something in core

# ^^ create a new 'examples' table in the database which has timestamps
# NB: the block here is passed to create_table in a normal migration,
# so you can use anything that create_table will accept here.
def use_database_table(table_name, &block)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if this is a good approach, to be honest. It feels dirty. What if plugins were inside folders and contained standard migration files, and they would be added to the rails migration path (just like post_migrate) when the plugin is activated?

use_directory(glob) { |path| use_asset(path) }
end

def use_class(path)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it would be good to have a lib folder inside plugin directory, and load everything from it when the plugin is activated. It seems unnecessary to explicitly load classes this way...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For more complicated plugins [and even in the simple example plugin], load order becomes a thing (You need to define your models before you define your controller, otherwise everything blows up). Rails handles this with const_missing lookups, but that approach felt complicated to me (and reliant on naming conventions that may make less sense to enforce within a plugin), and as a personal preference I like having a single file which explicitly says 'this is all of the things this plugin does' in one place, rather than needing to root through a big file structure to figure out what the relationships are

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no reason to worry about load order in Rails, and we shouldn't try to make a problem where none exists by going against rails' autoloader. We should be trying to work within Rails conventions as much as possible. Adding the plugins folder to the rails loader paths is the way to go

@Gargron
Copy link
Member

Gargron commented Nov 4, 2018

I left some comments, though I have not looked at everything yet. This is a complicated topic. What definitely concerns me is that the plugin system is the number one source of vulnerabilities in WordPress. So implementing an official plugin system in Mastodon is explicitly taking responsibility for what's to come in that regard. The surface area of securing Mastodon becomes much larger. Another thought is that one other Rails application that has a plugin system is Discourse, although I have not researched it thoroughly.

@nightpool
Copy link
Member

nightpool commented Nov 4, 2018 via email

@gdpelican
Copy link
Contributor Author

I feel like I should drop here that I've worked quite a bit with the Discourse plugin system, and that at the moment this is a quick and dirty reproduction of it in a lot of ways.

Here's the discussion on using a PluginData table, (which Discourse went with initially), but more recently they've been leaning towards allowing migrations because it's the more flexible solution.
https://meta.discourse.org/t/adding-jsonb-columns-for-custom-fields/93418/2

I totally appreciate that this is a complicated topic so I'm not in a rush to merge this, but I think even in it's current state it offers site admins a lot of powerful options (for example, this is enough to replicate Discourse's voting plugin, or Loomio's tagging plugin, both of which are hefty pieces of functionality), and of course you can start responding to people with requests like "I want this to say 'Bloop' instead of 'Blorp', or "I want this to be 3px to the left!" with 'Cool, you can do that, just write a plugin'

.gitignore Outdated
@@ -58,3 +58,9 @@ yarn-debug.log
# Ignore Docker option files
docker-compose.override.yml

# Ignore plugin files
/plugins/*
!/plugins/.keep
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you don't need to have these in gitignore, just add them with git add -f (and now that they're in the index, it doesn't matter anyway)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, will do

.gitignore Outdated
!/plugins/.keep
/app/javascript/packs/plugins.js
/app/javascript/mastodon/pluginConfig.js
!/config/plugins/.keep
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ibid

@nightpool
Copy link
Member

Here's the discussion on using a PluginData table, (which Discourse went with initially), but more recently they've been leaning towards allowing migrations because it's the more flexible solution.
https://meta.discourse.org/t/adding-jsonb-columns-for-custom-fields/93418/2

I'm not sure I understand sam's rationale there. If we're worried about indexes, why not just add jsonb indexes on the fields we care about?

@gdpelican
Copy link
Contributor Author

I'm all for just adding a table with a plugin_name and data column of type jsonb (that's kinda what I was proposing in that post). I think Sam is worried about transferring over data from existing plugins, which of course Mastodon wouldn't need to care about since no plugins exist yet.

@Gargron
Copy link
Member

Gargron commented Nov 7, 2018

Side comment: I would maybe remove the React.js parts of the plugin system for now. There's just too many potential places where you'd need to put some kind of hooks in, I can't even imagine.

Another side comment: It would be useful to have some kind of real plugin that is being developed to measure these APIs against.

@Gargron
Copy link
Member

Gargron commented Nov 7, 2018

Pros for using native migrations: More powerful, more versatile, very conventional for Rails developers.

Pros for using a jsonb store: Less surface area for plugin authors screwing up the Mastodon schema. Modifying upstream schema is not possible. Enabling/disabling plugins does not affect non-plugin data.

@nightpool
Copy link
Member

nightpool commented Nov 8, 2018 via email

@gdpelican
Copy link
Contributor Author

@Gargron I wonder if you or some community members have some 'Things I wish existed' list which might prompt a plugin to be developed alongside this? The aforementioned tagging one for Loomio was developed alongside the plugin system there for that purpose, which was really great because it hit the whole api (database table, ruby code, api, frontend components, css changes) without being a massive unwieldy piece.

@gdpelican
Copy link
Contributor Author

@nightpool @Gargron

I believe this one is ready for another look; I've greatly simplified the plugin interface (it's just 4 methods now), added the PluginData table, and made it so that assets and ruby files within the file structure are automatically added to the path.

Here's the example plugin I'd been working off:
captura de pantalla 2018-11-24 a la s 10 06 32 a m

And the template for the newly created generator (rails generate mastodon_plugin my_plugin), with some help on how to use each command:
captura de pantalla 2018-11-24 a la s 10 06 40 a m

@Gargron
Copy link
Member

Gargron commented Apr 2, 2019

Plugin idea: Stripe integration for paid accounts?

@stale
Copy link

stale bot commented Oct 26, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the status/wontfix This will not be worked on label Oct 26, 2019
@stale stale bot closed this Nov 2, 2019
@chandrn7
Copy link
Contributor

chandrn7 commented Aug 5, 2021

@Gargron @gdpelican Any interest in reopening this? I'd be willing to try and work on it. Seems like it is pretty far along and the next step is to build a real plugin?

@gdpelican
Copy link
Contributor Author

I'd of course be happy for anyone to continue down this path if there's appetite for it :)

@chandrn7
Copy link
Contributor

chandrn7 commented Aug 6, 2021

@gdpelican Cool! I think I'll take a stab at @Gargron's idea for a Stripe plugin.

Some questions:

  • As currently written, is there a way to modify html (thinking about public pages) from a plugin?
  • I'm a little confused on how to use plugin_config.js. Is it supposed to contain a list of outlets and locales that are available to plugin developers? What's the format?
  • Is this a good example of how to use the PluginData table? Add JSONB data field to PluginStoreRow discourse/discourse#6269

@gdpelican
Copy link
Contributor Author

Caveat on everything: I wrote this a long time ago and everything is very fuzzy.

  • I don't think this supports standalone pages at the moment, although you should be able to write your own controllers, so hopefully you'd be able to do something like
// plugin.rb
use_routes do
  get "plugin_name/example", to: "examples#show"
end
// examples_controller.rb
class ExamplesController < ApplicationController
  def show
  end
end
// define an 'examples/show.html.erb' within your plugin and get the controller to render it correctly somehow
  • No need to use plugin_config.js, per se; it essentially sends to the client a list of outlets and locales defined in the plugin.rb file; something like
// plugin.rb
use_outlet "assets/component.js", "outlet-name"
// assets/component.js
const SomeComponent = (props) => {
  ...
}
export default SomeComponent

and maybe have a locales/en.yml file in your plugin:

// locales/en.yml
en:
  some_translation: Hello world

, and the client ends up with some file like

{
  "outlet-name": [SomeComponent],
  "locales": { "some_translation": "Hello world" }
}

which it uses to automatically populate outlets and merge plugin locale strings into the current set of translations

  • Storing data in plugins is an interesting topic; Discourse has moved away from using PluginStore as a table and I think now supports writing your own migrations from within plugins. It'll be up to you and the Mastodon maintainers to figure out what the right approach to storing data like this is

@weex
Copy link
Contributor

weex commented Aug 28, 2021

@chandrn7 I suppose you have a starting plugin in mind but if not, I think a Web of trust moderation system would be really interesting. The plugin might just be an interface to call a REST api.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/wontfix This will not be worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants