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

Implement Plugin Classes #11564

Merged
merged 42 commits into from Mar 1, 2018
Merged

Implement Plugin Classes #11564

merged 42 commits into from Mar 1, 2018

Conversation

@burzum
Copy link
Member

burzum commented Dec 21, 2017

  • Plugins have a class that describes bootstrapping and routes
  • Plugin objects are attached to a plugin registry in Application.php

This is still WIP but I opened the PR already to get some feedback before going on with this.

public function initialize()
{

}

This comment has been minimized.

Copy link
@stickler-ci

stickler-ci Dec 21, 2017

Contributor

Expected 0 blank lines before closing function brace; 1 found
Function closing brace must go on the next line following the body; found 1 blank lines before brace

if ((!$instance instanceof HttpApplicationInterface
&& !$instance instanceof ConsoleApplicationInterface)
|| !$instance instanceof PluginInterface) {
throw new RuntimeException(sprintf(

This comment has been minimized.

Copy link
@stickler-ci

stickler-ci Dec 21, 2017

Contributor

Opening parenthesis of a multi-line function call must be the last content on the line

|| !$instance instanceof PluginInterface) {
throw new RuntimeException(sprintf(
'`%s` is not a valid plugin object. It\'s not implementing `%s` or `%s`',
get_class($instance)),

This comment has been minimized.

Copy link
@stickler-ci

stickler-ci Dec 21, 2017

Contributor

Multi-line function call not indented correctly; expected 12 spaces but found 16
Closing parenthesis of a multi-line function call must be on a line by itself

@burzum burzum requested a review from markstory Dec 21, 2017
@burzum burzum self-assigned this Dec 21, 2017
public function console($commands)
{
foreach ($this as $plugin) {
if ($plugin instanceof ConsoleApplicationInterface) {

This comment has been minimized.

Copy link
@ADmad

ADmad Dec 21, 2017

Member

Since you have PluginApp implements ConsoleApplicationInterface doesn't this check become redundant? IMO the base class should not implement ConsoleApplicationInterface, only plugins which provide console commands should implement that interface.

This comment has been minimized.

Copy link
@robertpustulka

robertpustulka Dec 21, 2017

Member

@ADmad you can implement PluginInterface in your own plugin that for example is console-only plugin. Hence the instanceof check I presume.

This comment has been minimized.

Copy link
@burzum

burzum Dec 22, 2017

Author Member

Correct @robertpustulka

This comment has been minimized.

Copy link
@ADmad

ADmad Dec 22, 2017

Member

Gotcha, my bad.

/**
* {@inheritdoc}
*/
public function disableRoutes($disabled)

This comment has been minimized.

Copy link
@ADmad

ADmad Dec 21, 2017

Member

It would be better to have 2 separate methods disableRoutes() and enableRoutes().

*
* @return \Cake\Core\PluginRegistry
*/
public function pluginRegistry()

This comment has been minimized.

Copy link
@ADmad

ADmad Dec 21, 2017

Member

getPluginRegistry()? :)

}

/**
* @expectedException \Error

This comment has been minimized.

Copy link
@ADmad

ADmad Dec 21, 2017

Member

This won't work on PHP 5.6 since it does not have an Error class.
https://travis-ci.org/cakephp/cakephp/jobs/319198618#L917

/**
* {@inheritdoc}
*/
public function routes($routes)

This comment has been minimized.

Copy link
@markstory

markstory Dec 21, 2017

Member

What does this parameter do? It isn't used.

*/
public function __invoke(ServerRequestInterface $request, ResponseInterface $response, $next)
{
return $response;

This comment has been minimized.

Copy link
@markstory

markstory Dec 21, 2017

Member

Shouldn't this be return $next($request, $response) instead?

*/
protected function _throwMissingClassError($class, $plugin)
{
new RuntimeException(sprintf(

This comment has been minimized.

Copy link
@markstory

markstory Dec 21, 2017

Member

There is no throw.

public function middleware($middleware)
{
foreach ($this as $plugin) {
$middleware->add($plugin);

This comment has been minimized.

Copy link
@markstory

markstory Dec 21, 2017

Member

Is the thinking here that plugins will be middleware? Or that plugins can add middleware. I think we want plugins to add middleware, which would look like

foreach ($this as $plugin) {
  if ($plugin instanceof HttpApplicationInterface) {
    $middleware = $plugin->middleware($middleware);
  }
}
return $middleware
public function __invoke(ServerRequestInterface $request, ResponseInterface $response, $next)
{
foreach ($this as $plugin) {
$response = $plugin($request, $response, $next);

This comment has been minimized.

Copy link
@markstory

markstory Dec 21, 2017

Member

This implementation is at odds with the middleware() method. I'm not convinced that plugin classes should be middleware yet.

This comment has been minimized.

Copy link
@burzum

burzum Dec 22, 2017

Author Member

OK, but the method must exists because of the interfaces.

@@ -73,6 +120,8 @@ public function routes($routes)
if (!Router::$initialized) {
require $this->configDir . '/routes.php';
// Prevent routes from being loaded again

$this->pluginRegistry()->routes($routes);

This comment has been minimized.

Copy link
@markstory

markstory Dec 21, 2017

Member

I would prefer to see us load routes in the application's routes file. This allows the app developer the ability to add additional paths/parameters via scopes. In 3.5 we added RouteBuilder::loadPlugin() to load routes from a single plugin. If the route builder had access to the plugin registry, or application instance we'd be able to leverage that existing interface. With that said we may also need a $routes->loadPlugins() to cover the case where people just don't want to customize routes and just load them all.

This comment has been minimized.

Copy link
@burzum

burzum Dec 22, 2017

Author Member

Any suggestion how to have a single plugin registry and share it between the router and the app without having a global DI container? I understand your arguments but it don't think this functionality should exist in two places? Also it's already loading the routes if the plugin is configured to load them? I don't see the issue except that we would lose a previously added methods feature?

This comment has been minimized.

Copy link
@markstory

markstory Dec 22, 2017

Member

We could hide the PluginRegistry inside Plugin. That would let us make the new/old ways to work with plugins easier to deal with.

I understand your arguments but it don't think this functionality should exist in two places?

I don't think we should plan to have the same functionality in two places as a permanent solution. Doing so requires us to document when to use each solution and ensure that both of them continue working.

Also it's already loading the routes if the plugin is configured to load them? I don't see the issue except that we would lose a previously added methods feature?

The routebuilder methods make it easier to customize plugin routes with additional scopes/paths. For example

$routes->scope('/tools', function ($routes) {
  $routes->loadPlugin('DebugKit');
});

Is not possible with the current implementation you have. This approach prevents plugin routes to be placed above routes defined in the application's route list. Placing frequently used routes higher in the file helps improve parse performance as fewer routes are checked on incoming requests.

This comment has been minimized.

Copy link
@robertpustulka

robertpustulka Dec 22, 2017

Member

From the lack of a better designed solution making Plugin a “facade” could work for the time being. We can rethink that in the future (4.x/5.x) when we have a DIC built in.

This comment has been minimized.

Copy link
@burzum

burzum Jan 3, 2018

Author Member

In 3.5 we added RouteBuilder::loadPlugin() to load routes from a single plugin.

Can't we check in the method if the first passed was a plugin registry object and then use the 2nd args as string for the name?

$routes->loadPlugins()

Could take the plugin registry as arg as well.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Dec 22, 2017

Codecov Report

Merging #11564 into 3.next will increase coverage by 0.14%.
The diff coverage is 94.6%.

Impacted file tree graph

@@             Coverage Diff              @@
##             3.next   #11564      +/-   ##
============================================
+ Coverage     91.94%   92.08%   +0.14%     
- Complexity    13288    13659     +371     
============================================
  Files           462      465       +3     
  Lines         34148    35217    +1069     
============================================
+ Hits          31396    32429    +1033     
- Misses         2752     2788      +36
Impacted Files Coverage Δ Complexity Δ
src/Routing/Middleware/RoutingMiddleware.php 100% <100%> (ø) 11 <0> (+1) ⬆️
src/Core/PluginCollection.php 100% <100%> (ø) 13 <13> (?)
src/Core/Plugin.php 95.68% <100%> (+1.09%) 48 <11> (-1) ⬇️
src/Console/CommandRunner.php 92.94% <88.88%> (-1.8%) 34 <6> (+5)
src/Core/BasePlugin.php 90.16% <90.16%> (ø) 25 <25> (?)
src/Http/Server.php 95.65% <91.3%> (-4.35%) 20 <8> (+5)
src/Http/BaseApplication.php 93.75% <96.96%> (+5.51%) 23 <15> (+14) ⬆️
src/View/Widget/WidgetRegistry.php 94.11% <0%> (-3.85%) 24% <0%> (ø)
src/Datasource/Paginator.php 98.01% <0%> (-0.44%) 58% <0%> (+14%)
src/Database/Type/IntegerType.php 100% <0%> (ø) 15% <0%> (+1%) ⬆️
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0c3c770...40fe576. Read the comment docs.

/**
* Disables route loading for the plugin
*
* @param bool $disabled True to disable it, false to enable it

This comment has been minimized.

Copy link
@stickler-ci

stickler-ci Dec 22, 2017

Contributor

Superfluous parameter comment

/**
* Enables route loading for the plugin
*
* @param bool $disabled True to disable it, false to enable it

This comment has been minimized.

Copy link
@stickler-ci

stickler-ci Dec 22, 2017

Contributor

Superfluous parameter comment

/**
* Disables bootstrapping for the plugin
*
* @param bool $disabled True to disable it, false to enable it

This comment has been minimized.

Copy link
@stickler-ci

stickler-ci Dec 22, 2017

Contributor

Superfluous parameter comment

/**
* Enables bootstrapping for the plugin
*
* @param bool $disabled True to disable it, false to enable it

This comment has been minimized.

Copy link
@stickler-ci

stickler-ci Dec 22, 2017

Contributor

Superfluous parameter comment

&& !$instance instanceof ConsoleApplicationInterface)
|| !$instance instanceof PluginInterface
) {
throw new RuntimeException(sprintf(

This comment has been minimized.

Copy link
@stickler-ci

stickler-ci Dec 22, 2017

Contributor

Opening parenthesis of a multi-line function call must be the last content on the line

) {
throw new RuntimeException(sprintf(
'`%s` is not a valid plugin object. It\'s not implementing `%s` or `%s`',
get_class($instance)),

This comment has been minimized.

Copy link
@stickler-ci

stickler-ci Dec 22, 2017

Contributor

Multi-line function call not indented correctly; expected 12 spaces but found 16
Closing parenthesis of a multi-line function call must be on a line by itself

* @param \Psr\Http\Message\ServerRequestInterface $request The request
* @param \Psr\Http\Message\ResponseInterface $response The response
* @param callable $next The next middleware
* @return \Psr\Http\Message\ResponseInterface

This comment has been minimized.

Copy link
@stickler-ci

stickler-ci Dec 22, 2017

Contributor

Function return type is not void, but function has no return statement

*
* @return $this
*/
public function enableRoutes();

This comment has been minimized.

Copy link
@markstory

markstory Dec 22, 2017

Member

I like these method names 👏

@markstory markstory self-assigned this Jan 19, 2018
@dereuromark dereuromark mentioned this pull request Jan 30, 2018
1 of 3 tasks complete
class PluginRegistry extends ObjectRegistry implements PluginRegistryInterface
{

/**

This comment has been minimized.

Copy link
@stickler-ci

stickler-ci Feb 2, 2018

Contributor

Doc comment for parameter "$objectName" missing
Doc comment for parameter "$config" missing


/**
* @inheritDoc
*/

This comment has been minimized.

Copy link
@stickler-ci

stickler-ci Feb 2, 2018

Contributor

Missing @return tag in function comment

@markstory markstory mentioned this pull request Feb 2, 2018
1 of 3 tasks complete
'plugin' => $plugin,
'config' => $config
]);
EventManager::instance()->dispatch($event);

This comment has been minimized.

Copy link
@ADmad

ADmad Feb 5, 2018

Member

Once #11692 is done the application event manager instance could be injected into plugin registry instead of fetching the global instance through static method.

This comment has been minimized.

Copy link
@burzum

burzum Feb 5, 2018

Author Member

This might become a little off topic but by looking at this PR and the use case(s), I think we begin to use the event manager here like a DI container? Wouldn't it be better to wait for that?

This comment has been minimized.

Copy link
@markstory

markstory Feb 5, 2018

Member

I think we can separate off events from this. #11692 will at solve several event manager related requirements.

@markstory

This comment has been minimized.

Copy link
Member

markstory commented Feb 11, 2018

I've pushed up a bunch of changes that make this code more inline with what was documented in the related issue. I've not fully implemented the addPlugin() method yet, but will be doing that shortly. What is 'done' is the plugin collection (instead of a registry) and the plugin hooks being called. Still to be done are:

  • Actually loading plugins.
  • Options and methods for the other plugin hooks
  • Injecting the plugin collection in to Plugin for backwards compatibility
  • Connecting the plugin collection into the route builder so that RouteBuilder::loadPlugin() works as it should.
*/
public function __construct(array $options = [])
{
foreach (['bootstrap', 'routes', 'console', 'middleware'] as $key) {

This comment has been minimized.

Copy link
@robertpustulka

robertpustulka Feb 13, 2018

Member

Can use VALID_HOOKS const here

This comment has been minimized.

Copy link
@markstory

markstory Feb 13, 2018

Member

Good idea. Fixed.

burzum and others added 11 commits Dec 11, 2017
A simpler collection will work for Plugins. This will let us integrate
new style plugins into `Plugin` more easily as we have a simple
container object to work with.
Plugins do not have to implement the Application interfaces as they are
hook based plugins and not the same as applications.

While they could be the same, it doesn't make sense for plugins to add
themselves as a middleware layer when they have a middleware hook. The
Application adds itself as a middleware to ensure there is an 'end' to
the middleware queue and ensure that a response is always generated.
Add the new plugin hooks to the console runner.
@robertpustulka

This comment has been minimized.

Copy link
Member

robertpustulka commented Feb 20, 2018

Overwriting event managers also runs the risk of dropping listeners in the case a plugin accidentally creates a new manager.

The same can happen with routes or middleware as the hooks must return objects. I think that behavior of all hooks that modify an object should be similar.

/**
* {@inheritDoc}
*/
public function pluginEvents()

This comment has been minimized.

Copy link
@robertpustulka

robertpustulka Feb 20, 2018

Member

I think that this method should behave similar to the pluginMiddleware and pluginRoutes and accept an object and return the modified one.

The risk of dropping changes if a new instance is returned by mistake is the same as with the middleware and routes.

This comment has been minimized.

Copy link
@dereuromark

dereuromark Feb 20, 2018

Member

Immutable approach of always using return value to continue sounds indeed more consistent.

This comment has been minimized.

Copy link
@markstory

markstory Feb 21, 2018

Member

Ok. I will get this updated. Do you also think all the new hooks should have typehints added where we have existing interfaces?

This comment has been minimized.

Copy link
@markstory

markstory Feb 21, 2018

Member

I've added typehints for EventManagerInterface as it was the only existing interface.

This comment has been minimized.

Copy link
@robertpustulka

robertpustulka Feb 21, 2018

Member

@markstory pluginEvents() hasn't been updated with type hints.

This comment has been minimized.

Copy link
@markstory

markstory Feb 22, 2018

Member

It also doesn't take parameters or return a value either though. I found that adding them forced callers to know much more about the application and coordinate more logic. The current code better encapsulates the event logic in my opinion.

Perhaps the Server and CommandRunner should drop their event managers and instead proxy the application manager?

This comment has been minimized.

Copy link
@robertpustulka

robertpustulka Feb 22, 2018

Member

It also doesn't take parameters or return a value either though.

pluginRoutes, pluginConsole and pluginMiddleware take an argument and return the modified object. I don't like that pluginEvents is different. Either we remove that from routes, console and middleware, or add to events.

Also having pluginEvents with no arguments assumes that application that implements PluginApplicationInterface must have it's own event manager. Executing that method on an "eventless" application instance would take no effect. The idea of application "hooks" in my opinion is to take an object, modify it and return the modified instance.

Perhaps the Server and CommandRunner should drop their event managers and instead proxy the application manager?

In this case the application would always need to support events. This is optional now, as http and console application interfaces don't extend EventDispatcherInterface.

This comment has been minimized.

Copy link
@robertpustulka

robertpustulka Feb 22, 2018

Member

I feel like I need to explain my point a bit more.

In "isolation" application does not dispatch events by default (it doesn't have to implement the EventDispatcherInterface), the Server and CommandRunner do. So in this scenario event hooks should be executed on Server's and CommandRunner's event manager. So both events and pluginEvents must take an argument in order to modify the server/runner's event manager.
If application does implement EventDispatcherInterface it's event manager is injected into the server/runner in setApp setter. The process of applying hooks on an event manager is still the same, but the subject (event manager instance) is different now (app's event manager)

This comment has been minimized.

Copy link
@markstory

markstory Feb 22, 2018

Member

I hear you on it feeling inconsistent, my concern is how much logic adding a parameter/return value forces onto the caller of pluginEvents(). For example the bootstrap method on both Server and CommmandRunner would need to look like:

        $this->app->bootstrap();
        if ($this->app instanceof EventApplicationInterface) {
            $eventManager = $this->app->events($this->getEventManager());
            $this->setEventManager($eventManager);
        }
        if ($this->app instanceof PluginApplicationInterface) {
            $this->app->pluginBootstrap();
            if ($this->app instanceof EventApplicationInterface) {
                $events = $this->app->getEventManager();
                $events = $this->app->pluginEvents($events);

                $this->app->setEventManager($events);
                $this->setEventManager($events);
            }
        }

To me this puts a lot of onus on the caller and breaks the encapsulation that the interfaces should be trying to provide. Perhaps the PluginApplicationInterfaceshould include/consume the EventApplicationInterface and EventDispatcherInterface? That would allow us to have fewer checks across the framework. It would prevent 'only' using events and not plugins, but I think that is a reasonable tradeoff to make for reduced complexity.

Joining the interfaces may also allow us to make proxying eventmanager methods simpler on Server and CommandRunner. The getEventManager() method could have an implementation that looks something like:

public function getEventManager()
{
  if ($this->app instanceof PluginApplicationInterface) {
    return $this->app->getEventManager();
  }
  return EventManager::instance();
}
* @param \Cake\Routing\RouteBuilder $routes The route builder to update.
* @return \Cake\Routing\RouteBuilder
*/
public function routes($routes);

This comment has been minimized.

Copy link
@dereuromark

dereuromark Feb 21, 2018

Member

Could we use some interface of RouteBuilder here as typehint?

This comment has been minimized.

Copy link
@markstory

markstory Feb 21, 2018

Member

An interface for RouteBuilder seems wasteful to me as it is not something we expect/want users to reimplement.

This comment has been minimized.

Copy link
@dereuromark
* @param \Cake\Event\EventManager $events The application's event manager
* @return void
*/
public function events($events);

This comment has been minimized.

Copy link
@dereuromark

dereuromark Feb 21, 2018

Member

Could we use some interface of EventManager here as typehint?

markstory added 2 commits Feb 21, 2018
Plugin events() hook now works like the application one. This improves
consistency between plugins and applications.
Fix mistakes as `__DIR__` is not going to work.
@markstory

This comment has been minimized.

Copy link
Member

markstory commented Feb 21, 2018

What do folks think about the name PluginApp? Is ApplicationPlugin a better name?

What about the informal convention of having src/Plugin.php in each plugin. Should that instead be src/Debugkit.php where 'DebugKit' is the plugin's name?

@rchavik

This comment has been minimized.

Copy link
Member

rchavik commented Feb 21, 2018

@markstory what about namespaced plugin?

nvm, i misunderstood your comment

@burzum

This comment has been minimized.

Copy link
Member Author

burzum commented Feb 21, 2018

@markstory if we move the whole plugin related classes into Cake\Plugin? We can have a Plugin class there as well then and deprecate the one in Cake\Core?

Let's use ApplicationPlugin. And I prefer to have a file Plugin in each plugin, makes it easy to remember and spot and also tells you what it is about.

@saeideng

This comment has been minimized.

Copy link
Member

saeideng commented Feb 21, 2018

having src/Plugin.php in each plugin

👍 for src/Plugin.php naming

markstory added 2 commits Feb 22, 2018
This better mirrors the base class name for applications.
*/
public function makePlugin($name, array $config)
{
if (!class_exists($name)) {

This comment has been minimized.

Copy link
@ADmad

ADmad Feb 22, 2018

Member

Could just check if name does not have backlash instead of class_exists(), similar to what App::className() does.

if (!class_exists($name)) {
$name = str_replace('/', '\\', $name) . '\\' . 'Plugin';
}
if (!class_exists($name)) {

This comment has been minimized.

Copy link
@ADmad

ADmad Feb 22, 2018

Member

Wouldn't PHP already generate a good enough error message if you try to instantiate a non existent class below?

This comment has been minimized.

Copy link
@markstory

markstory Feb 22, 2018

Member

Could, I thought this error was better though.

markstory added 4 commits Feb 22, 2018
Plugins will need their routes and setup run multiple times during
integration tests. With a require once this is not possible.
markstory added 2 commits Feb 23, 2018
They had an awkward relationship that is better expressed by making them
the same. This also lets the pluginEvents() hook be a 'transparent' hook
method similar to routes, middleware and console.
Instead of copying state around, we can use the existing interfaces to
create proxies for evented applications. This lets us make evented and
non-evented applications work the same as far as the
Server/CommandRunner are concerned.
@markstory

This comment has been minimized.

Copy link
Member

markstory commented Feb 23, 2018

@robertpustulka I've updated these changes to include the merged interface and proxies we talked about today.

* @param \Cake\Event\EventManagerInterface $eventManager Event manager instance.
* @return \Cake\Event\EventManagerInterface
*/
public function events(EventManagerInterface $eventManager);

This comment has been minimized.

Copy link
@robertpustulka

robertpustulka Feb 23, 2018

Member

Since now PluginApplicationInterface extends EventDispatcherInterface would it make sense to remove argument and return form events and pluginEvents method? App that supports event hooks now has it's own event manager and listeners should be attached to that instance anyway.

// App\Application.php
public function events()
{
    $this->getEventManager()->on(new MyListener);
}

and

public function pluginEvents()
{
    $events = $this->getEventManager();
    foreach ($this->plugins->with('events') as $plugin) {
        $events = $plugin->events($events); //this stays as it is, plugin doesn't dispatch events
    }
    $this->setEventManager($events);
}

This way event handling could be even more encapsutaled.

This comment has been minimized.

Copy link
@markstory

markstory Feb 23, 2018

Member

I'm ok with that. I added the argument to make the routes, events, and middleware hooks consistent in that they all get a 'thing' and return that 'thing'. Making them 'closed' would add inconsistency with other hooks but simplify framework code.

This comment has been minimized.

Copy link
@robertpustulka

robertpustulka Feb 23, 2018

Member

Yeah, I know I’ve been advocating for consistency but now with the current API this makes little sense to have event hooks get and return something since it’s going to be always an app’s dependency.

@markstory markstory changed the title New way of loading and initializing plugins Implement Plugin Classes Feb 23, 2018
markstory added 2 commits Feb 24, 2018
Now that plugin applications support events out of the box, we can
remove the additional hook and leverage the bootstrap hook to attach
events. This makes applications and plugins simpler.
@markstory markstory merged commit ec32168 into 3.next Mar 1, 2018
5 checks passed
5 checks passed
codecov/patch 94.6% of diff hit (target 91.94%)
Details
codecov/project 92.08% (+0.14%) compared to 0c3c770
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
stickler-ci No lint errors found.
@markstory markstory deleted the 3.next-plugins branch Mar 1, 2018
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.