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 manager #171

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 36 additions & 0 deletions src/Psy/Command/DemoCommand.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
<?php

namespace Psy\Command;

use Symfony\Component\Console\Input\InputInterface;
use Symfony\Component\Console\Input\InputOption;
use Symfony\Component\Console\Output\OutputInterface;

class DemoCommand extends Command
{
/**
* {@inheritdoc}
*/
protected function configure()
{
$this
->setName('demo')
->setDefinition(array(
new InputOption('message', 'm', InputOption::VALUE_REQUIRED, 'Message to send.'),
))
->setDescription('Sample command just for testing.')
->setHelp(
<<<HELP
HELP
);
}

/**
* {@inheritdoc}
*/
protected function execute(InputInterface $input, OutputInterface $output)
{
$message = $input->getOption('message');
$output->writeln(sprintf('<info>Received message "%s". </info>', $message));
}
}
25 changes: 24 additions & 1 deletion src/Psy/Configuration.php
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
use Psy\ExecutionLoop\Loop;
use Psy\Output\OutputPager;
use Psy\Output\ShellOutput;
use Psy\Plugin\Manager;
use Psy\Presenter\PresenterManager;
use Psy\Readline\GNUReadline;
use Psy\Readline\Libedit;
Expand All @@ -33,9 +34,10 @@ class Configuration
'defaultIncludes', 'useReadline', 'usePcntl', 'codeCleaner', 'pager',
'loop', 'configDir', 'dataDir', 'runtimeDir', 'manualDbFile',
'requireSemicolons', 'historySize', 'eraseDuplicates', 'tabCompletion',
'tabCompletionMatchers',
'tabCompletionMatchers', 'registerPlugins',
);

private $registerPlugins = true;
private $defaultIncludes;
private $configDir;
private $dataDir;
Expand Down Expand Up @@ -96,6 +98,27 @@ public function __construct(array $config = array())
// go go gadget, config!
$this->loadConfig($config);
$this->init();

// ask the plugin manager for configurations
if ($this->getRegisterPlugins()) {
$this->loadConfig(Manager::getConfiguration($config));
}
}

/**
* @param $bool
*/
public function setRegisterPlugins($bool)
{
$this->registerPlugins = $bool;
Copy link
Owner

Choose a reason for hiding this comment

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

Please add (bool) here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

}

/**
* @return bool
*/
public function getRegisterPlugins()
{
return $this->registerPlugins;
}

/**
Expand Down
68 changes: 68 additions & 0 deletions src/Psy/Plugin/AbstractPlugin.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
<?php

namespace Psy\Plugin;

abstract class AbstractPlugin
{
public static function register()
{
Manager::register(new static(), static::getName());
}

/**
* @return string
*/
final public static function getName()
Copy link
Contributor

Choose a reason for hiding this comment

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

only the register function should be static imho, as it initializes the plugin

Copy link
Owner

Choose a reason for hiding this comment

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

what use would you have for an instance of a plugin?

Copy link
Contributor

Choose a reason for hiding this comment

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

as an object point of view I guess (I still have Twig's extensions system in mind, which allows from an object pov to instantiate an extension)

Copy link
Owner

Choose a reason for hiding this comment

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

But it doesn't make sense as an object. It's an object with no responsibility :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but it is a value-objet, not just a class :D

Copy link
Contributor

Choose a reason for hiding this comment

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

Then @Markcial's register method in the AbstractPlugin is wrong. Why instanciate the plugin when all it should do is just a __CLASS__ instead ?

(Still subjective, but I really don't like static if I can help otherwise :D)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

new __CLASS__ is an alias of new static, so it's only about coding style.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was refering to only register the class name, not make an instance of it : Manager::register(__CLASS__, static::getName())

Copy link
Contributor

Choose a reason for hiding this comment

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

Mmh nay it is wrong, as __CLASS__ is not statictly bound, it will return "AbstractPlugin" here. So it should use get_called_class() instead : Manager::register(get_called_class(), static::getName())

Copy link
Contributor Author

Choose a reason for hiding this comment

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

objects can hold static calls too, it doesn't matter, you can call ClassName::method or $object::method. BTW, done this way we have the AbstractPlugin enforcer on the register function.

{
$class = new \ReflectionClass(get_called_class());

return preg_replace('#Plugin$#', '', $class->getShortName());
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure that this is really relevant, I think it should be an abstract

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure too, maybe automation is not the solution, but maybe writting the name for all the plugins is not necessary. I leave this like this temporarily, but i am not convinced too.

Copy link
Owner

Choose a reason for hiding this comment

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

tbh, how much does the plugin name matter?

Copy link
Contributor

Choose a reason for hiding this comment

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

Almost none but still, for a registry (such as the Manager) it could be good to ensure that a plugin is registered only once. Never tried to register a same command more than once, but wouldn't it cause problems ?

Copy link
Owner

Choose a reason for hiding this comment

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

I think it'd just override the previous one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, the name is the unique key indexer. see src/Psy/Plugin/Manager.php line 16, so if anyone adds a plugin with the same name it will be overwritten like a FIFO pile.

Copy link
Owner

Choose a reason for hiding this comment

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

But what if there are two people with DevToolsPlugin or AwesomePlugin? Is disambiguating by the first part of the short class name the best way to go?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So true, at first was thinking about using the full namespaced class. Anyways i am not yet convinced about this, maybe is hould open up this method and get a default naming.

Copy link
Owner

Choose a reason for hiding this comment

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

Add an abstract static function that plugins have to implement and call it a day. That way plugins can just use their Packagist package names. Mine can be called bobthecow/psysh-devtools and yours can be called markcial/psysh-devtools, and we'll never collide :)

}

/**
* @param array $configuration
*
* @return array
*/
final public static function getConfiguration($configuration = array())
Copy link
Owner

Choose a reason for hiding this comment

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

This method doesn't feel quite right. It doesn't feel like it should be this plugin's responsibility to worry about any other configuration, and in the current implementation, every plugin has a chance to mess with the global configuration. Yeah, I get that this is final, but it still feels wrong. The manager should be responsible for doing the merging.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

totally agree, i am not convinced yet, i was waiting to get with a good idea to put there, let's see.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've been thinking about this method, and I do agree that having "2" entry point (one through a getConfiguration and one through the getters) is two much. More over, this method is doing a merge to "chain" the methods, and I don't think that this should be the scope of this method. it should just return a plain array if it is left as-is

Copy link
Contributor

Choose a reason for hiding this comment

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

so basically, 👍 @bobthecow

Copy link
Owner

Choose a reason for hiding this comment

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

Why not have the manager call getCommands, getMatchers, etc on each plugin and merge those, respectively? I'd much prefer something explicit like that at the manager level.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes that's what I was referring to, let the plugin manager call by itself the getters

{
return array_merge_recursive(
$configuration,
array(
'commands' => static::getCommands(),
'presenters' => static::getPresenters(),
'matchers' => static::getMatchers(),
Copy link
Contributor

Choose a reason for hiding this comment

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

all the getX methods should be protected imho, and this getConfig should be the only one accessible with the getName

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what if they only want the matchers for the plugin but not the presenters or the commands? I do not see a problem to expose publicly that methods, are only getters with static data.

Copy link
Owner

Choose a reason for hiding this comment

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

It'd be super useful for plugins which depend on plugins. For example, Drupal, Laravel and EZ all depend on some parts of Symfony. So if there were a Symfony plugin, perhaps those others would want the Symfony commands, but not presenters or matchers. Or if there were a Doctrine plugin, I could see wanting the ability to present and match Doctrine models, but not expose all the Doctrine console commands, because they are legion.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems fair, even though (still on a (quite) subjective object pov) I don't know if there is really any interest for the thing that registers the plugin to have any access to other methods

// if any more parts of the config are exposed publicly, remember to add here with the static ref.
)
);
}

// any publicly exposed configuration piece below here ↓

/**
* @return array
*/
public static function getCommands()
{
// add your own commands
return array();
}

/**
* @return array
*/
public static function getPresenters()
{
// add your own presenters
return array();
}

/**
* @return array
*/
public static function getMatchers()
{
// add your own presenters
return array();
}
}
18 changes: 18 additions & 0 deletions src/Psy/Plugin/DemoPlugin.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
<?php

namespace Psy\Plugin;

use Psy\Command\DemoCommand;

class DemoPlugin extends AbstractPlugin
{
/**
* @return array
*/
public static function getCommands()
{
return array(
new DemoCommand(),
);
}
}
32 changes: 32 additions & 0 deletions src/Psy/Plugin/Manager.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
<?php

namespace Psy\Plugin;

class Manager
Copy link
Owner

Choose a reason for hiding this comment

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

For consistency with existing managers, this should prolly be called PluginManager.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

{
/** @var AbstractPlugin[] */
protected static $plugins = array();

/**
* @param AbstractPlugin $plugin
* @param $name
*/
public static function register(AbstractPlugin $plugin, $name)
{
self::$plugins[$name] = $plugin;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not quite sure that this class should be static (Or otherwise it should be called something else, such as PluginRegistry ?) ; thus, we would need to pass the Manager instance to the register method of the Plugin

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is the reason behind that? why the plugin has to be able to modify the manager? The manager should not be accessible, if the manager goes public we might be needing some intermediate layer for protection like the registry one you said before.

Copy link
Contributor

Choose a reason for hiding this comment

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

With the method you used, it is accessible though... it should also check that the plugin is not already registered

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 indeed are accessible, but no the plugins variable. maybe an exception if someone tries to register an already registered plugin, but why another intermediate layer for plugin handling?


/**
* @param array $configuration
*
* @return array
*/
public static function getConfiguration($configuration = array())
{
foreach (self::$plugins as $plugin) {
$configuration = $plugin::getConfiguration($configuration);
Copy link
Contributor

Choose a reason for hiding this comment

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

you forgot some things here. You're overwrting the $configuration with the last plugin found in the $plugins array, and overwriting the parameter $configuration, so it is not correct anymore with the 2nd+ plugin...

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 configuration is merged, the configurations are preserved, but the commands, matchers or presenters are not removed.

Copy link
Contributor

Choose a reason for hiding this comment

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

oh, them I don't think it should merge anything, it should just return the brute configuration rather than try to merge it with something else

Even though it is really subjective, I think it could be confusing (at least I was confused :x)

}

return $configuration;
}
}