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

Hook for service registration in a plugin #4383

Open
ViRuSTriNiTy opened this Issue Jan 19, 2019 · 4 comments

Comments

Projects
None yet
3 participants
@ViRuSTriNiTy
Copy link
Contributor

ViRuSTriNiTy commented Jan 19, 2019

Hi there,

as described here ...

https://forum.pkp.sfu.ca/t/hook-for-service-registration-in-a-plugin/50799

... there should be a hook for providing services from a plugin.

I will provide a PR.

So lonG

@NateWr

This comment has been minimized.

Copy link
Contributor

NateWr commented Jan 28, 2019

This is a great idea, thanks @ViRuSTriNiTy!

I'm trying to think through the implications here, and figure out what is the best approach. One concern I have with putting it into Services::get() is that it's called over and over again. I understand why it can't be in the init(), but I also worry that if a plugin can inject a service at any time, we'll end up with a lot of headaches down the line where a developer won't be able to know when a service will be registered. They'll have to find it out on a plugin-by-plugin basis.

In WordPress, there are a few low-level hooks like init, admin_init, after_plugins_loaded and after_themes_loaded. These are good for when a plugin or theme wants to register some kind of asset that is available generally.

I'd like to try a similar approach here. On the one hand, we'll have a bit more certainty about when a custom Service is registered. On the other hand, we'll be able to use the same hook for lots of things like this, which will help keep our documentation smaller (when we get to writing it out properly!).

Here's what I'm thinking.

Every request to the application is kicked off by Dispatcher::dispatch(). Here, every generic plugin is loaded and the context schema is reinitialized:

https://github.com/pkp/pkp-lib/blob/master/classes/core/Dispatcher.inc.php#L131-L138

AppLocale::initialize($request);
PluginRegistry::loadCategory('generic', true);

// Reload the context after generic plugins have loaded so that changes to
// the context schema can take place
import('classes.core.Services');
$contextSchema = \Services::get('schema')->get(SCHEMA_CONTEXT, true);
$request->getRouter()->getContext($request, 1, true);

I'd like to add a new hook here:

AppLocale::initialize($request);
PluginRegistry::loadCategory('generic', true);

HookRegistry::call('Dispatcher::dispatch', $request);

// Reload the context after generic plugins have loaded so that changes to
// the context schema can take place
import('classes.core.Services');
$contextSchema = \Services::get('schema')->get(SCHEMA_CONTEXT, true);
$request->getRouter()->getContext($request, 1, true);

Then, in PKPServices, we should add a static method for registering a new ServiceContainer:

public static function register($service) {
	self::_instance()->container->register($service);
}

If a plugin wants to register a new Service, it should then create a new ServiceProvider class, just like the OJSServiceProvider:

namespace PluginName;
use \Pimple\Container;
class PluginNameServiceProvider implements \Pimple\ServiceProviderInterface {
	public function register(Container $pimple) {
		$pimple['serviceName'] = function() {
			return new \PluginName\ServiceNameService();
		};
	}
}

Finally, it should hook to Dispatcher::dispatch and register the new container:

public function registerServiceContainer($hookName, $request) {
	Services::register(new \PluginName\PluginNameServiceProvider);
}

Cons:

  • There is a little more boilerplate here. But I think only a big complex plugin should bother with a Service class, so I think that's ok.
  • Only generic plugins can use this hook. I think that's the right call. Other plugin categories should provide all the methods needed to handle those use-cases. If plugin developers find them lacking, we should extend that category's base class.

Pros:

  • Plugins can register multiple services using the same Provider architecture as the apps.
  • The base hook is highly reusable for other low-level plugin needs.
  • If a plugin has a service class, any third-party developer can know with certainty when that service class will be available in the app.

Notes:

  • If a plugin really wants to, it can register a new service class anywhere. This will remain undocumented and not be considered a recommended practice.

@asmecher do you have any thoughts about this?

@ViRuSTriNiTy I haven't tested this at all, but would you be willing to work up a Pull Request along these lines, once we have input from Alec?

@ViRuSTriNiTy

This comment has been minimized.

Copy link
Contributor Author

ViRuSTriNiTy commented Jan 28, 2019

@NateWr Sounds reasonable, waiting for response from @asmecher

@asmecher

This comment has been minimized.

Copy link
Member

asmecher commented Mar 13, 2019

Hi all -- sorry for the delayed response. @NateWr's proposal seems well considered and it looks like you're in agreement, @ViRuSTriNiTy.

(Potentially getting off on a tangent here -- but in rewriting the tests for a newer PHPUnit I had to introduce some changes to reduce the dependence on static function calls e.g. by using singleton or factory patterns instead. I see you're using _instance here. Can we standardize on a function name for this? We have e.g. TemplateManager::getManager, various Application/PKPApplication factory methods, and the soon-to-be-merged Application::get. I think instance (or _instance where it's protected/private) are the clearest.)

@NateWr

This comment has been minimized.

Copy link
Contributor

NateWr commented Mar 13, 2019

Just pointing out that the use of _instance is part of the Pimple dependency injection library that Kassim integrated to manage our services.

It's pretty small so would be easy to replace with something else if we want. We had a PHP 5.3 compatibility requirements at the time so may be able to find something more modern now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.