-
Notifications
You must be signed in to change notification settings - Fork 314
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
base: develop
Are you sure you want to change the base?
plugin manager #171
Changes from 3 commits
243a74d
5971754
c9b34d8
d44d773
f1d7610
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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)); | ||
} | ||
} |
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() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. only the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what use would you have for an instance of a plugin? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, but it is a value-objet, not just a class :D There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Then @Markcial's register method in the (Still subjective, but I really don't like static if I can help otherwise :D) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 : There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Mmh nay it is wrong, as There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
{ | ||
$class = new \ReflectionClass(get_called_class()); | ||
|
||
return preg_replace('#Plugin$#', '', $class->getShortName()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. tbh, how much does the plugin name matter? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Almost none but still, for a registry (such as the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it'd just override the previous one. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But what if there are two people with There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
} | ||
|
||
/** | ||
* @param array $configuration | ||
* | ||
* @return array | ||
*/ | ||
final public static function getConfiguration($configuration = array()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. so basically, 👍 @bobthecow There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not have the manager call There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. all the getX methods should be protected imho, and this There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); | ||
} | ||
} |
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(), | ||
); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
<?php | ||
|
||
namespace Psy\Plugin; | ||
|
||
class Manager | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For consistency with existing managers, this should prolly be called There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you forgot some things here. You're overwrting the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add
(bool)
here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍