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

plugin manager #171

wants to merge 5 commits into from

Conversation

Markcial
Copy link
Contributor

@Markcial Markcial commented Mar 2, 2015

What it currenlty does:

  • successfully creates a configuration array after registering the plugins.
  • add matchers, commands or presenters to the configuration.
  • give static access to the configuration or plugins.
    Example:
    \Psy\Plugin\DemoPlugin::register();
    \Psy\Plugin\Manager::getConfiguration(); // will return the democommand added in the configuration
    
    What it is not yet able to do.
  • Magically discover plugins by vendor path.
  • Replace properties in the configuration keeping the newest ones.

{
$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 :)

@bobthecow
Copy link
Owner

One more piece, configuration-wise. I think we should have a disableAutoRegister (or some other name meaning something like that) static method on the plugin manager which could be called in a config file or shell wrapper to prevent automatically registering plugins.

@Markcial
Copy link
Contributor Author

Markcial commented Mar 3, 2015

Good catch, after the $this->init which loads the files and stuff might be ok, right?

@bobthecow
Copy link
Owner

Hmm. The order things happen might need to be tweaked. We want presenters defined in the config file to be registered after automatically discovered ones, so that they can be locally overridden, but we also want to be able to suppress automatic registration inside the config file.

@Markcial
Copy link
Contributor Author

Markcial commented Mar 3, 2015

ok,then i temporarily add the function call at the end, later we could be able to add test coverage or tweak it as needed.

*/
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 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

@bobthecow
Copy link
Owner

More commentary back on the main thread :)

#166 (comment)


namespace Psy\Plugin;

class PluginManager
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this class be made final perhapse?

@bobthecow bobthecow closed this Mar 15, 2020
@bobthecow bobthecow reopened this Mar 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants