plugin manager #171

Markcial · 2015-03-02T16:54:39Z

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.

Taluu · 2015-03-02T17:16:16Z

src/Psy/Plugin/AbstractPlugin.php

+    {
+        $class = new \ReflectionClass(get_called_class());
+
+        return preg_replace('#Plugin$#', '', $class->getShortName());


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

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.

tbh, how much does the plugin name matter?

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 ?

I think it'd just override the previous one.

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.

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?

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.

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 · 2015-03-03T12:34:26Z

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 · 2015-03-03T12:36:56Z

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

bobthecow · 2015-03-03T12:39:56Z

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 · 2015-03-03T12:40:46Z

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.

bobthecow · 2015-03-03T12:59:39Z

src/Psy/Configuration.php

+     */
+    public function setRegisterPlugins($bool)
+    {
+        $this->registerPlugins = $bool;


Please add (bool) here.

bobthecow · 2015-03-03T13:04:10Z

src/Psy/Plugin/AbstractPlugin.php

+     *
+     * @return array
+     */
+    final public static function getConfiguration($configuration = array())


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.

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

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

so basically, 👍 @bobthecow

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.

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

…vices

bobthecow · 2015-03-05T19:01:14Z

More commentary back on the main thread :)

#166 (comment)

GrahamCampbell · 2015-03-26T23:59:53Z

src/Psy/Plugin/PluginManager.php

+
+namespace Psy\Plugin;
+
+class PluginManager


Should this class be made final perhapse?

Markcial force-pushed the plugin_manager branch from 0aa1a68 to 5270927 Compare March 2, 2015 16:58

plugin manager

243a74d

Markcial force-pushed the plugin_manager branch from 5270927 to 243a74d Compare March 2, 2015 17:02

Markcial mentioned this pull request Mar 2, 2015

Plugin perspective / brainstorm #166

Open

Taluu reviewed Mar 2, 2015
View reviewed changes

added manager configuration load in the configuration class

5971754

Markcial force-pushed the plugin_manager branch from 8c718e0 to 5971754 Compare March 3, 2015 12:33

Added parameter that forbids plugin discovery

c9b34d8

Markcial force-pushed the plugin_manager branch from 0e75892 to c9b34d8 Compare March 3, 2015 12:45

bobthecow reviewed Mar 3, 2015
View reviewed changes

boolean enforcer added to bolean parameter

d44d773

bobthecow reviewed Mar 3, 2015
View reviewed changes

Markcial force-pushed the plugin_manager branch from f8c127e to 4d1130f Compare March 3, 2015 15:59

Added testing, refactored a little bit the configuration building ser…

f1d7610

…vices

Markcial force-pushed the plugin_manager branch from 4d1130f to f1d7610 Compare March 3, 2015 16:17

GrahamCampbell reviewed Mar 26, 2015
View reviewed changes

src/Psy/Plugin/PluginManager.php

namespace Psy\Plugin;

class PluginManager

Copy link

Contributor

GrahamCampbell Mar 26, 2015

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 closed this Mar 15, 2020

bobthecow reopened this Mar 15, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

plugin manager #171

plugin manager #171

Markcial commented Mar 2, 2015

Taluu Mar 2, 2015

Markcial Mar 2, 2015

bobthecow Mar 2, 2015

Taluu Mar 2, 2015

bobthecow Mar 2, 2015

Markcial Mar 3, 2015

bobthecow Mar 3, 2015

Markcial Mar 3, 2015

bobthecow Mar 3, 2015

bobthecow commented Mar 3, 2015

Markcial commented Mar 3, 2015

bobthecow commented Mar 3, 2015

Markcial commented Mar 3, 2015

bobthecow Mar 3, 2015

Markcial Mar 3, 2015

bobthecow Mar 3, 2015

Markcial Mar 3, 2015

Taluu Mar 3, 2015

Taluu Mar 3, 2015

bobthecow Mar 3, 2015

Taluu Mar 3, 2015

bobthecow commented Mar 5, 2015

GrahamCampbell Mar 26, 2015

plugin manager #171

Are you sure you want to change the base?

plugin manager #171

Conversation

Markcial commented Mar 2, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobthecow commented Mar 3, 2015

Markcial commented Mar 3, 2015

bobthecow commented Mar 3, 2015

Markcial commented Mar 3, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobthecow commented Mar 5, 2015

Choose a reason for hiding this comment