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

Plugin perspective / brainstorm #166

Open
Taluu opened this issue Feb 26, 2015 · 47 comments
Open

Plugin perspective / brainstorm #166

Taluu opened this issue Feb 26, 2015 · 47 comments

Comments

@Taluu
Copy link
Contributor

Taluu commented Feb 26, 2015

What would be awesome would be, using Psysh, to be able to register extensions, so that it may register custom commands (without going through a PR here), outputs, presenters, ... or whatever else.

Just launching the discussion here, so that we may maybe get some ideas. I was thinking of something like how Twig does it (Or Symfony2 and its bundles, as this is more complete but also more tedious)

Any thoughts ?

@Markcial
Copy link
Contributor

You can roll your own commands with the configuration, http://psysh.org/#configure you only need to have the psr0 path defined, extend the basecommand class, and then create the new instance in the commands parameter. Anyways, to load commands from a subfolder if they extend a class might be another option.

@Taluu Taluu changed the title Plugin perspective Plugin perspective / brainstorm Feb 26, 2015
@Taluu
Copy link
Contributor Author

Taluu commented Feb 26, 2015

Yep, but with a Plugin, you may also register not only commands, and I think that this is baseline for such a system. Anyhow, it seems @bobthecow that is interested in such a discussion, as it seems he already tried to approach such a system, without being satisfied.

Just creating the issue so that we can brainstorm on that matter. ;)

It is already great though that we can register commands... Maybe push the config thing but for plugins ? So that they may live as full fledged plugins.

@Markcial
Copy link
Contributor

The ways that came into my mind of registering plugins are; with a base folder that has classes that implements some kind of PluginInterface, with a command with calls like plugin add "class" or with the configuration. My two cents here.

@Taluu
Copy link
Contributor Author

Taluu commented Feb 26, 2015

I don't know if registering a plugin through the app (via a plugin command) wouldn't be too late, as the app is already launched. I was also thinking of an interface to implement, and then somewhere (such as the config file ?) register the plugin.

when the app is launched, it then checks all the plugin to instanciate, and registers all the stuff that goes along with it (such as commands, printers, presenters, ....)

@Markcial
Copy link
Contributor

Well then it depends which layers are explosed publicly, @bobthecow might have a better knowdledge about this. If something cames up i might be able to help a little bit, i will keep an eye open o this thread then.

@bobthecow
Copy link
Owner

Here's a dump of what I'm thinking:

There are basically three (easy) extension points for PsySH (and quite a few not-as-easy ones, but let's focus on the easy ones first)… Commands, Presenters and Tab Completion Matchers. So a plugin implementation would at the very least need to provide a way to discover those three.

I'd prefer to have things "just work" where possible. So while adding a line to your config file would do the trick, it would be awesome if you could install plugins just by installing them. As I see it, there are a couple of ways to get this to work.

Plugin discovery via interface wouldn't really work, because autoloading. There's no way to iterate through all possible classes and check their interfaces. A better approach would probably be to leverage composer for the default case, then users who don't composer could do a slightly more manual and less "just works" version that involves adding something to their config.

Even in the composer-based "just works" version, I'd like to keep things lightweight. This means that a heavy Symfony-style plugin system won't happen. Something like this, though, might:

<?php

namespace Psy\DoctrinePlugin;

use Doctrine\ORM\Tools\Console\Command;
use Psy\DoctrinePlugin\Matchers;
use Psy\DoctrinePlugin\Presenters;
use Psy\Plugins;
use Psy\Plugins\PluginInterface;

class Plugin implements PluginInterface
{
    public static function register()
    {
        Plugins::register('doctrine', __CLASS__);
    }

    public static function getCommands()
    {
        return array(
            // These are the usual doctrine command line commands, which just
            // happen to be Symfony Console commands, so we can use 'em without
            // any wrapping or local code :)
            new Command\RunDqlCommand(),
            new Command\ValidateSchemaCommand(),
        );
    }

    public static function getPresenters()
    {
        return array(
            new Presenters\DoctrineModelPresenter(),
            new Presenters\DoctrineRepositoryPresenter(),
        );
    }

    public static function getMatchers()
    {
        return array(
            new Matchers\DoctrineModelMatcher(),
            new Matchers\DoctrineRepositoryMatcher(),
        );
    }
}

Then Psy\Plugins would be a global registry that keeps a static list of plugins that are available, and Configuration::getPresenters, etc, would check registered plugins and add their Presenters, etc, to the list of defaults.

(Note that this is very much like what Twig does, as suggested by @Taluu, but with a global registry component)

In the non-composer version of this, you'd just have to autoload your plugin somehow, and call Psy\DoctrinePlugin::register() in your the config file.

In the composer version, it's even awesomer. Each plugin would simply add a register.php file to autoload.files in their composer.json:

<?php

Psy\DoctrinePlugin\Plugin::register();

Then if your project adds a dev (or production) dependency on a plugin, that plugin will automatically register itself and be available for PsySH to use without you even touching your config :)

The next step here would be to add a "global" installation as well, for everyone who uses PsySH outside of a project. For that, I'd just make the PsySH config folder (or maybe a subfolder?) a composer project that depends on the plugins you want to install. Then you'd have a bonus autoloader from that composer project, and Configuration would check for it by convention (and maybe configuration) the same way it checks for the manual db file, or whatever else.

The ultimate awesomeness would then be to have a command for adding dependencies to this "global" plugins composer project, automatically downloading them, and autoloading them right into the current console. This would basically rip off (extend) everything @Markcial's composer and sandbox stuff already does :)

@Taluu
Copy link
Contributor Author

Taluu commented Feb 26, 2015

This seems to be what I first thought about using something similar to Twig then (it registers an extension, which basically registers all its dependencies on the go). So I'm all +1 for that, even though I didn't go deeper with the things you're suggesting with the autoload.files, but it would be awesome.

Even if I think that also having a classic way to register an extension (such as a basic "plugins" key in the config file if it is a global installation, with a basic new Psy\MyPlugin array element) would also be awesome... But just for the global installation though ?

@bobthecow
Copy link
Owner

Even if I think that also having a classic way to register an extension

That's where I was getting at with the "non-composer" comments. If you don't want to go the composer-based route (because you hate yourself, maybe?) then you can always manually call register() at the top of your own config file and it will work just the same.

@Taluu
Copy link
Contributor Author

Taluu commented Feb 26, 2015

I'm not a huge fan of the static things, but on an object point of view and also a pratical point of view, it seems to be the right thing to do.

What was bothering me with the autoload-files way, was that you need to use your composer.json for that. But if the files are loaded after the classes declarations are loaded (and thus, PsySh is accessible) then this seems a good solution (I've never used this mechanism, shame on me)

But I think I got messed up with composer, as I thought is was too coupled with the project where it doesn't. My bad !

@bobthecow
Copy link
Owner

If you don't want to use composer or static, you can totally still use plugins without either. You can also only register part of what a plugin offers, if that's what you're into:

return array(
  'presenters' => array_merge(\Psy\DoctrinePlugin\Plugin::getPresenters(), ...),
  'commands' => array_merge(\Psy\DoctrinePlugin\Plugin::getCommands(), ...),
);

… but realistically, approximately zero of our users will do that. It's not a use case we need to optimize for. We need to optimize for the most common use case of "this project uses doctrine, so i should require psy/doctrine-plugin in my composer.json" :)

@Taluu
Copy link
Contributor Author

Taluu commented Feb 26, 2015

… but realistically, approximately zero of our users will do that

Don't worry, neither will I. :D

Maybe do something with a composer script, to automatically register an extension ? Even though it is kinda the same thing when requiring a plugin / symfony bundle actually (you need to register it yourself in your app / whatever)... I don't think this should be the main concern (maybe over another iteration ?)

@bobthecow
Copy link
Owner

That's the point of using autoload.files in composer.json. Then the plugin is registered when autoloader is loaded.

@Taluu
Copy link
Contributor Author

Taluu commented Feb 26, 2015

Yup, but then you must have a register.php file with the plugin, and you must think of adding it in your autoload-files (or script, or whatever)

@bobthecow
Copy link
Owner

No, the plugin adds it.

@Taluu
Copy link
Contributor Author

Taluu commented Feb 26, 2015

Oooh, I didn't see it like that. It makes senses

@bobthecow
Copy link
Owner

And yes, that would be one required bit of boilerplate for a plugin to be automatically registrable. I don't think that's too much to ask of a plugin developer :P

@Taluu
Copy link
Contributor Author

Taluu commented Feb 26, 2015

Meh, as long as it is documented, it should be fine ! :D

@bobthecow
Copy link
Owner

Naming thoughts. Which of these looks better for package names?

  • psy/doctrine
  • psy/doctrine-plugin
  • psy/psysh-doctrine
  • psy/doctrine-psysh

@bobthecow
Copy link
Owner

I like the way the node and ruby communities have a parentproject-subproject convention in their package names, so I'm leaning toward psy/psysh-doctrine.

(Or fred/psysh-doctrine or whatever)

@Taluu
Copy link
Contributor Author

Taluu commented Feb 26, 2015

doctrine-plugin if I had to pick one, as the psy is already kind of indicative that this is related psysh (you don't see too often symfony/symfony-oauth-bundle or package/symfony/symfony-oauth-bundle for example).

The next step would be to have a "core extension", so that Psysh is really minimal when shipped ? :)

@bobthecow
Copy link
Owner

But the psy/ part is related to who develops it, not what it's for.

@bobthecow
Copy link
Owner

If you shipped a psysh plugin, you'd call it taluu/doctrine-plugin? Now it has no context. taluu/psysh-doctrine does, and would even if someone else forked your package to add their own things, and published it under their own vendor prefix.

@Taluu
Copy link
Contributor Author

Taluu commented Feb 26, 2015

Yup you're right, but my point was that for example the hwioauthbundle (curse that thing), it is hwi/oauth-bundle, no hwi/sf2-oauth-bundle... there aren't any standards on that, so I guess the developper should use what he feels like. I'd me more inclined to have a psysh to be honest, as plugin is too generic. Unless you may fine a cool name that easily identifies a plugin (such as bundle for synfony2, gem for rubies, ...) :D

@bobthecow
Copy link
Owner

The next step would be to have a "core extension", so that Psysh is really minimal when shipped ? :)

Nope nope. PsySH should ship with everything that's generally applicable. No assembly required. Else why do we bother giving people a phar?

There are a handful of things that should be moved out of PsySH once this is available. The MongoDB presenters and Tab Completion Matchers, the PHPParser command... These aren't enabled by default, but they don't really need to be shipped with PsySH itself once there's an easy way to extend it.

@bobthecow
Copy link
Owner

Right, developers can use whatever they want. But we can say "the convention is $vendor/psysh-$plugin right in the docs for "how to make a plugin" and hopefully influence developers to make good decisions ;)

@Taluu
Copy link
Contributor Author

Taluu commented Feb 26, 2015

Still taking Twig as an example, it registers all the core matters in a Core extension, but it is there for the basic installation (on the phar, it should be there, but not on the other ways to install it, such as by composer) ?

Or register them in a Core extension that is registered within the core, removable if the user doesn't have any uses for it (should add a way to remove a plugin btw ?)

@bobthecow
Copy link
Owner

But what would you remove? There aren't any presenters shipped by default that shouldn't be there (other than the mongo ones that aren't enabled). You need all of those in order to actually have output for your REPL. You could make a case for removing commands, but I don't think you could make a very strong one. There aren't a lot of extraneous commands.

I don't like abstraction and compartmentalization just for the sake of abstraction and compartmentalization. "Make everything a plugin!" goes too far, in my opinion. PsySH isn't supposed to be 100%. It's opinionated. It's a REPL, not a REPL framework. :)

@Taluu
Copy link
Contributor Author

Taluu commented Feb 26, 2015

I'm probably too engaged towards a REPL Framework, but what you're saying makes sense... I'm still convinced that some things that are currently shipped with Psysh should be put into an extension (such as the presenters you listed, ...). I wouldn't be able to say which exactly, but still, it would need some thinking.

Maybe in another iteration though, this is already a feature big enough to develop ;)

@Markcial
Copy link
Contributor

WHOA! that scalated quickly 😆

@Markcial
Copy link
Contributor

Well in briefly i have to say, is a really nice feature to add to the psy, making it really extensible and personallizable, anyways i would recommend some api methods call for disabling some presenters or matchers or commands, i mean, maybe someone develops a plugin and it possibly conflicts with another shipped element, maybe someone want to use a matcher for files and doesnt want to use the default one.

@bobthecow
Copy link
Owner

@Markcial a way to remove things might be nice to have, but for the most part I'm not convinced it's necessary. For example, later presenters win over earlier ones. So the defaults are easily overridden by adding another presenter that claims to present the same thing.

Again, this isn't about creating a REPL framework, it's about creating an opinionated REPL that usually does the right thing. That's why people like it more than the other options. That's a core value proposition of PsySH :)

@Markcial
Copy link
Contributor

Well, totally agree. the main issue with libraries is that there is no perfect fit, so the opinionated/agnosthic pattern is nice for all the different comunities that would want a nice REPL for their environment.

with all that said, i am willing to help, so count me in.

@Taluu
Copy link
Contributor Author

Taluu commented Feb 26, 2015

If I can help for anything, don't mind to ask :)

@Markcial
Copy link
Contributor

I am currently working on a couple of experiments, but when i got this experiments in a decent state i might be able to work on a experiment for this ticket, maybe on mid march.

@Markcial
Copy link
Contributor

Markcial commented Mar 2, 2015

Draft open for review here => #171 it has a working sample, simply write :

\Psy\Plugin\DemoPlugin::register();
$config = \Psy\Plugin\Manager::getConfiguration();
$shell = new Shell(new Configuration($config));
$shell->run();

screenshot from 2015-03-02 18 09 58

@Markcial
Copy link
Contributor

Markcial commented Mar 3, 2015

Hi! I am currently fiddling with the code and i am wondering how to set the link to the plugin in a passive but non intrussive way. I tried to code a discovery function but it might not be the best solution. because the plugin manager will need to be called every time, check for vendor packages, decode the installed files etc, maybe a little bit too much.

But then digging in the composer docs i found this https://getcomposer.org/doc/articles/scripts.md and @bobthecow already mentioned to use a post install script, so maybe it is the best chance, but after having the post install script as a start point i don't know where to store the "plugin refs", maybe a file called ".plugin-registry" in the root folder? dunno.. i am a little bit stuck, i am pinging here so maybe anyone has a good idea about this.

@Markcial
Copy link
Contributor

Markcial commented Mar 3, 2015

nevermind, i finally understood what the autoload.files was for, i created a package in packagist : https://packagist.org/packages/markcial/psysh-twitter, i fiddled a little bit with the composer.json file and it worked like a charm.

Follow this steps to reproduce:

  • composer init
  • paste this in the composer.json file:
    {
    "name": "meh/whatever",
    "require": {
        "markcial/psysh-twitter": "dev-master",
        "psy/psysh": "dev-plugin_manager"
    },
    "repositories": [
      {
        "type": "vcs",
        "url":  "git@github.com:Markcial/psysh.git"
      }
    ]
    }
    
  • composer install.
  • run vendor/bin/psysh
  • and finally >>> twitter --help
    screenshot from 2015-03-03 12 47 21

@bobthecow
Copy link
Owner

Nice!

@bobthecow
Copy link
Owner

@Markcial
Copy link
Contributor

Markcial commented Mar 4, 2015

Wow, so much plugin aware shell! nice :D

@bobthecow
Copy link
Owner

:)

@bobthecow
Copy link
Owner

I've been thinking a bit more about this.

PsySH is a weird library, because it can be installed locally or globally, as either a Composer dependency or a Phar. So we need to support each of those use cases as much as we can without making things crappy. This means several things, to which I have several answers. The things first, then the answers :)


Plugins shouldn't depend on PsySH

That's just asking for dependency hell. And it means they wouldn't play nice with the Phar installs, which, IMHO, are one of the best ways to do it (see #170 for some issues with the alternatives).

Plugins should be installable both globally and locally

Whatever solution we come up with, it needs to be able to load both sorts of plugins.

A Configuration should be able to load only specific plugins

Some users use project or environment specific PsySH config files. For them, it would make a ton of sense to be able to explicitly load a subset of all available plugins.


Plugins shouldn't depend on PsySH

Since plugins don't depend on PsySH itself, they must depend on something else

My vote is a super lightweight plugin registry library, based on @Markcial's PluginManager work. It should have no dependencies, and serve only as a container for collecting plugins which have been registered. Because it will be both bundled in the Phar and installed locally to the plugins via Composer, it must be 100% compatible no matter which version is found first. It should essentially never have a backwards compatibility break, ever.

Here's what I'm thinking for that: https://github.com/bobthecow/psysh-plugin-registry

With this, plugins depend on just the registry, keeping us from getting into dependency hell with multiple PsySH versions (see psysh-mongodb, for example):

    "require": {
        "psy/plugin-registry": "~1.0"
    },

And because it has the lightweight, (probably) future-proof API, it should be resilient to improvements to plugins. If we decide that plugins should be able to provide alternate execution loops, for example, we'd add a new interface to the plugin registry package, and any plugin can implement the ExecutionLoopProvider interface. If the PsySH instance that's loading plugins from the registry supports them, the plugin's execution loop(s) may be used. But if any of the pieces involved are on an older version, everything should degrade gracefully.

Plugins should be installable both globally and locally

Plugins installed globally prolly belong somewhere in $XDG_DATA_DIRS (e.g. ~/.local/share/psysh/plugins).

Plugins installed locally prolly belong in the project's composer requirements (or dev requirements).

Ideally, a single PsySH instance should be able to load plugins from both places, and have everything work out okay. And this isn't as complicated as it sounds, fwiw.

Note that I haven't addressed how plugins get into ~/.local/share/psysh/plugins. That'll be the subject of a future issue or pull request, once the composer/sandbox command pull request is finished :)

A Configuration should be able to load only specific plugins

This should be straightforward enough. The Registry knows the names of plugins. The Configuration has a property for suppressing plugin registration. We can use these two together for awesome :)

To completely disable automatically registered plugins, via PsySH config:

<?php

return [
  'registerPlugins' => false
];

To enable only specific plugins, via PsySH config:

<?php

return [
  'registerPlugins' => ['psy/psysh-mongodb', 'markcial/psysh-twitter']
];

@bobthecow
Copy link
Owner

This also ties in quite nicely with the psysh bin/phar change I just made (see #172). This means that if psysh is a dependency of the current project, running the global psysh will use that project's autoloader to load the local version of psysh instead — along with all the automatically registered plugins for that project.

@Markcial
Copy link
Contributor

Markcial commented Mar 6, 2015

Great explanation. I agree about the idea of using a environment variable where phar packages can be added. So the plugin registry might be able to check for the phar packages. Then the registry might be able to ask the plugin for a bootstrap method that could return a closure that loads all necessary stuff or a file path for inclusion that might bootstrap instead. This way the plugin knows how to get loaded and delegates to the registry for this task. I will try to pull an example to your registry repository.

@Markcial
Copy link
Contributor

Markcial commented Mar 6, 2015

Well, my idea. I will post it here.

I was thinking about using two environment variables, so it can be system wide.

$paths = array(
    "Psy\\MongodbPlugin\\" => "/usr/local/share/psysh/plugins/psysh-mongodb.phar/src",
    "Psy\\AwesomePlugin\\" => "~/.local/plugins/awesome-plugin/src"
);

$bootstraps = array(
    "Psy\\MongodbPlugin\\Plugin::register",
    "Psy\\AwesomePlugin\\Plugin::register"
);

putenv("PSYSH_CLASSPATH=" . json_encode($paths));
putenv("PSYSH_BOOTSTRAPS=" . json_encode($bootstraps));

Then the registry might be able to work over this environment variables.

    public static function bootstrap()
    {
        if (!$paths = getenv('PSYSH_CLASSPATH')) {
            // warn about missing environment variable
        }

        $paths = json_decode($paths, true);
        $loader = new ClassLoader();
        $loader->addClassMap($paths);

        if (!$bootstraps = getenv('PSYSH_BOOTSTRAPS')) {
            // warn about missing environment variable
        }
        $bootstraps = json_decode($bootstraps, true);
        foreach ($bootstraps as $bootstrap) {
            call_user_func($bootstrap);
        }
    }

It is only a idea, but configuration files can be fine too, but then you have to check for local files, then global, rewrite over the file, etc. IMHO is a huge mess, but a wide env variable can be worked over on it, loaded, modified and reworked.

BTW, the pahr itselves could add settings to that environment variable and export it to the system via call to a file or command.
EDIT
After some little bit of meditation, seems to be that the plugin manager might not have dependencies, but some of the plugins might have some instead. So when the plugins will get loaded will search for its libraries and then throw exceptions for repeated classes, redefinition of functions etc.

A possible solution could be to use a loader that will not depend on composer, but might be able to understand composer, so if the phar is shipped, it will have the composer.json files or composer.lock, the loader checks for the libraries, if they are not found, download them with the composer command. this way the composer handles dependencies, but will not conflict. Dunno, just some ideas that came through my mind.

@bobthecow
Copy link
Owner

I wasn't suggesting using an environment variable to define where things are installed. I think that's just complication for complication's sake. I was suggesting using XDG, which is a very well defined standard :)

I'm not suggesting reinventing autoloading, or package management. Composer does a great job at that.

I'm not suggesting giving users a way to execute arbitrary code via environment variables. There's already a perfectly good way to do that. Your PsySH config file is made out of code instead of JSON or YAML for a reason :)

This isn't to say I'd stop you from using environment variables to load plugins. You can do that all you want. You can put plugins wherever you want, and load them however you want. Your config file is PHP. Go crazy. I'm just saying it's probably not a good idea to enable that by default, as a blessed code path.

I'm suggesting having a single canonical place where plugins are automatically loaded from, e.g. ~/.local/share/psysh/plugins. This would be a Composer project. If you wanted to put phar files in there, that would be perfectly legal, you'd just have to add them to the autoload section in ~/.local/share/psysh/plugins/composer.json. Additionally, if you're using PsySH locally in a project, for example, the way Laravel does, you could install plugins specifically for that project in that project's composer.json (dev) dependencies. Then it'll work too, because autoload magic.

So my suggestion is to give two automatic, default ways to load plugins automatically, without any configuration or craziness. If you want to load them a different way, or scatter them out over your hard drive, or whatever, go for it. Just add a line or two to your PsySH config that executes some arbitrary code to do what you're looking for. But that's not a good default user experience :)

@Markcial
Copy link
Contributor

Markcial commented Mar 6, 2015

Sorry, i replied agreeing topics that other ones said, not yours. So it's my fault for not detailing it precisely, my apologies.

I doesn't mean to reinvent autoloading, just using the composer autoloading out of the phar package. so the dependencies are not held inside the package, and can be updated in a place where the phar package can be able to load this libraries.

About the configuration part, you are right. But then the plugin registry would need one important dependency, psysh itself for its configuration, because registry should be able to know which plugins should be called or not. If i have a global install of psysh-plugin1 that uses cool-lib-v1.0 and a local install of psysh-plugin2 that uses cool-lib-v2.0 which has conflicts with its prior version, you decide to load psysh-plugin2, then the configuration part would look like this :

return [
  'registerPlugins' => ['psy/psysh-plugin2']
];

registry would try to load the psr0 paths, and will warn about a library conflict (i suppose, i have not tested but my experience tolds me so). maybe a solution could be if the plugins could be referenced without inclusion, but AFAIK PHP does not work like that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants