Implement plugin system and add PreserveWorkingTree plugin

[ramsey]

Description

I went through several refactorings of this PR before I decided to open it up for final review.

In its current state, this diverges from the proposed functionality in #122 in that this implements a plugin system that allows for plugins that can execute code before and after a hook runs, as well as before and after each action runs. This allows extensions to hook into the beforeHook(), beforeAction(), etc. methods to extend what CaptainHook can do.

This PR also provides an initial plugin to illustrate the plugin behavior: PreserveWorkingTree. This plugin implements some of the feature I proposed in #122. When added to the plugins list in captainhook.json, this plugin will run before and after each hook to move any working tree changes out of the way and then reapply them. It will do this for any hook, since it's possible that an action in any hook could modify the working tree and/or staged files. Others can extend this class and implement the Constrained interface to constrain it to only specific hooks.

Notable Changes/Details

• I've changed the CaptainHook\App\Runner\Hook::beforeAction() signature to include a CaptainHook\App\Config\Action $action parameter. This is so we can pass this value to the plugin. Technically, this is a BC break, since this is a public method, and children of Hook that override this method will need to update their signatures. However, in practice, I don't think CaptainHook provides a way to inject your own hook classes to override the ones in CaptainHook\App\Runner\Hook, so this change might not affect any external users.
• I've changed the CaptainHook\App\Runner\Hook::afterAction() signature for the same reason.
• I've moved Hook::beforeAction() and Hook::afterAction() into the Hook::handleAction() method, so they will now run for both PHP and CLI actions (previously, they only ran before and after PHP actions).
• As long as the hook is enabled, Hook::beforeHook() and Hook::afterHook() will always run, even if there are no actions. This allows runner plugins to run their beforeHook() and afterHook() methods, even if the hook has no actions configured. Previously, if there were no actions, these methods did not execute.
• Added Hook::shouldSkipActions(?bool $shouldSkip = null): bool that allows plugins to tell the hook to skip any remaining actions.
• Added Hook::getName() to return the string name of the hook, so plugins can use this.
• Added a CaptainHookException interface that all CaptainHook exceptions implement.

Documentation

The following documentation has also been submitted as part of #130

Plugins

Actions and Conditions are suitable for most needs, but there may be times you need to add more functionality to CaptainHook, such as performing tasks before or after a hook runs. CaptainHook's plugin system provides this flexibility!

All CaptainHook plugins implement the CaptainHook\App\Plugin\CaptainHook interface. To use a plugin, add it to the plugins array in your config. If a plugin requires options, you may also provide those.

{
  "config": {
    "plugins": [
      {
        "plugin": "\\MyName\\GitHook\\MyPlugin",
        "options": {
          "myOption": true,
          "stuff": "cool things"
        }
      }
    ]
  }
}

Runner Plugins

Runner plugins are so-called because they execute during the hook runner stage of CaptainHook, allowing you to do things before and after the hook runs, as well as before and after each action in the hook.

To create a runner plugin, implement the CaptainHook\App\Plugin\Runner interface. Optionally, you may extend the CaptainHook\App\Plugin\Runner\Base abstract class, which provides some basic functionality.

To see an example runner plugin, check out CaptainHook\App\Plugin\Runner\PreserveWorkingTree.

namespace MyName\GitHook;

use CaptainHook\App\Config;
use CaptainHook\App\Plugin;
use CaptainHook\App\Runner\Hook as RunnerHook;

class MyPlugin extends Plugin\Runner\Base implements Plugin\Runner
{
    /**
     * Runs before the hook.
     *
     * @param RunnerHook $hook This is the current hook that's running.
     */
    public function beforeHook(RunnerHook $hook): void
    {
        $stuff = $this->plugin->getOptions()->get('stuff');
        $this->io->write("Do {$stuff} before {$hook->getName()} runs");
    }

    /**
     * Runs before each action.
     *
     * @param RunnerHook $hook This is the current hook that's running.
     * @param Config\Action $action This is the configuration for action that will
     *                              run immediately following this method.
     */
    public function beforeAction(RunnerHook $hook, Config\Action $action): void
    {
        $this->io->write("Do stuff before action {$action->getAction()} runs");
    }

    /**
     * Runs after each action.
     *
     * @param RunnerHook $hook This is the current hook that's running.
     * @param Config\Action $action This is the configuration for action that just
     *                              ran immediately before this method.
     */
    public function afterAction(RunnerHook $hook, Config\Action $action): void
    {
        $this->io->write("Do stuff after action {$action->getAction()} runs");
    }

    /**
     * Runs after the hook.
     *
     * @param RunnerHook $hook This is the current hook that's running.
     */
    public function afterHook(RunnerHook $hook): void
    {
        $this->io->write("Do stuff after {$hook->getName()} runs");
    }
}

[sebastianfeldmann]

Did I get that right that pre-commit the Captain will always move the working tree changes?
So you don't have to add an Action that handles it only if you want that kind of safety instead it will always happen because it is included in the default PreCommitRunner? 🤔

Im not sure it is the right thing to make any changes with a potentially empty configuration.

[ramsey]

It will always happen, but only if there are actions configured in pre-commit. I don't think the hook runs beforeHook() or afterHook() if there are no actions. Is that what you're asking?

[sebastianfeldmann]

Would it be possible to configure this functionality in an Action?
Like I said I'm not sure if I'm comfortable changing the users working tree without them actively configuring it.
But I haven't thought about it that much yet.

[ramsey]

The only problem with it happening in an Action is that an action has a single execute() method and can't control the working tree before and after the hook runs.

What do you think about a global config value that turns on/off this behavior?

The main problem I ran into (which is why I decided to work on this) is because I had some hooks that changed file contents and then ran git add {$STAGED_FILES} as an Action. Since some of the staged files also had unstaged changes in them that we didn't want to commit yet, this also added those changes to the index and then proceeded to commit them without us knowing. It was behavior we didn't expect, and it also wasn't noticed until later. 😄

[sebastianfeldmann]

Building a switch to turn it on and off sounds actually perfect.

[ramsey]

Building a switch to turn it on and off sounds actually perfect.

Cool! I'll do that, then.

[ramsey]

I decided to take a different route with this. Rather than create a new config option and hard-code this into the pre-commit hook, I decided to abstract it into a separate class, and then I thought it might make more sense to create a plugin system to support what I'm trying to do.

So, this PR now includes a plugin system for the Runner (though this could be expanded to support plugins that provide a list of pre-configured actions, etc.), and a PreserveWorkingTree plugin. I'll update the full description above with all the info. I'll also update the documentation before opening this for review.

[ramsey]

This is now ready for review!

[larsroettig]

From my point it will be clean up PHP itself should be unscary

[ramsey]

I'm not sure what you're saying.

[sebastianfeldmann]

I think Lars is referring to the unset and that PHPs internal garbage collector will clean it up automatically. But I'm not sure either :)

[ramsey]

Oh, I'm unsetting it here so that it doesn't cause any problems in getJsonArray() here:
https://github.com/captainhookphp/captainhook/pull/125/files#diff-9727fe62ad60c6637a7def31b06f908a39f99f118a78280469d7a62eb67a9d68R260-R267

Maybe this is unnecessary?

[sebastianfeldmann]

Nope, totally necessary since you are not redefining $config['plugins'] you are just appending.

$data['config']['plugins'][] = $plugin->toJsonData();

Maybe this is a good hint to change it up, remove the unset, but completely overwrite $config['plugins'] in toJsonData to be sure.

[sebastianfeldmann]

Wow I like this approach much better then the previous one :)

I really like the implementation. I'm still reviewing but I wanted to get your opinion on two points.

1. Plugin creation

I'm not sure creating the plugin "inside" the Config\Plugin is the right location. Actions are not created by Config\Action and Conditions are not created by Config\Condition . Currently the object creation is handled by everything under Runner\*. So the Runner\Hook should take care of it. It would be nice to separate it from the Runner\Hook, maybe something like a Runner\Plugin would be optimal for creating and executing the objects at least for consistency reasons. What do you think?

2. Plugin naming (just thinking out "loud")

Currently the plugins are called RunnerPlugins => Plugin\Runner. That is where they are located technically but I'm not sure we should name them like that. My first thought was Plugin\Hook as in HookPlugin. The only issue I might have with it is that is quite unspecific. But I think I like HookPlugin better... as I said, just thinking out loud. What do you think? Any particular reason you would prefer the current RunnerPlugin name?

[ramsey]

I'm not sure creating the plugin "inside" the Config\Plugin is the right location.

I can't remember why I did it this way, but I agree that it shouldn't happen here. I'll move it back to inside Runner\Hook and give some thought to a Runner\Plugin.

I think I like HookPlugin better... as I said, just thinking out loud. What do you think? Any particular reason you would prefer the current RunnerPlugin name?

I have no preference for the RunnerPlugin name. I named it this way to differentiate it from other potential types of plugin interfaces you may want to add in the future. While I'm not sure what other kinds of plugins you might want to have, I wanted to leave it open for the possibility.

I'll give it some more thought and post some ideas here for feedback before updating the PR.

[ramsey]

About the idea of a Runner\Plugin, do you see that as something that gets instantiated in \CaptainHook\App\Console\Command\Hook::execute() and passed into the constructor of CaptainHook\App\Runner\Hook?

Something like this?

/** @var \CaptainHook\App\Runner\Plugin **/
$plugin = $this->getPlugin($io, $config, $repository);

$class = '\\CaptainHook\\App\\Runner\\Hook\\' . Util::getHookCommand($this->hookName);
/** @var \CaptainHook\App\Runner\Hook $hook */
$hook  = new $class($io, $config, $repository, $plugin);

Then, in Runner\Plugin we instantiate and configure the hook plugins using the config, and Runner\Plugin would have methods like this:

public function beforeAction(Runner\Hook $hook, Config\Action $action): void
{
    foreach ($this->plugins as $plugin) {
        $plugins->beforeAction($hook, $action);
    }
}

And this would get called in Runner\Hook::beforeAction()?

public function beforeAction(Config\Action $action): void
{
    $this->plugin->beforeAction($this, $action);
}

[sebastianfeldmann]

Currently the HookRunner handles all the sub runner creation.

    private function doConditionsApply(array $conditions): bool
    {
        $conditionRunner = new Condition($this->io, $this->repository, $this->hook);

    private function executeCliAction(Config\Action $action): void
    {
        $runner = new Action\Cli();
        $runner->execute($this->config, $this->io, $this->repository, $action);
    }

I like the injection idea, but for now I think it is better to let the HookRunner create it and later inject something like a SubRunnerFactory so all "sub-runners" can be "injected" without adding more and more arguments to the constructor.

Other then the single sub-runner injection it looks great :)

[ramsey]

@sebastianfeldmann I've made changes based on your feedback. Let me know if these changes work. Thanks!

[sebastianfeldmann]

Looks amazing, I have only one more question, but that's more an understanding thing then an issue preventing me from merging this thing.

[ramsey]

Did you forget to include your question, or am I missing it somewhere? 😄

[sebastianfeldmann]

I added it the the code, but had difficulties finding it as well 🙈

Here it is :)
Does it make sense to move the $this->beforeHook(); and $this->afterHook(); inside the else statement.
So if no action will be performed the before and after is skipped as well. Or does it make sense to actually call before and after anyway?

[ramsey]

I did that on purpose because I thought it made sense for a plugin to have the ability to execute before and after the hook, even if there are no actions configured for the hook. Most of the time, a hook without any actions will probably be disabled, so it won't execute at all.

I could go either way on this, though.

What do you think? If a hook is enabled but has no actions configured, should the plugins be allowed to execute their beforeHook() and afterHook() methods?

[sebastianfeldmann]

If the hook is activated, but all actions are skipped because of pre conditions this might occure. In case of your plugin it would be fine to skip the patch creation, cleanup and patch re-applying but I understand the argument that we don't want to inhibit people from executing their plugin code anyway.
So lets keep it as is :)

[sebastianfeldmann]

I'm not sure creating the plugin "inside" the Config\Plugin is the right location. Actions are not created by Config\Action and Conditions are not created by Config\Condition . Currently the object creation is handled by everything under Runner\*. So the Runner\Hook should take care of it. It would be nice to separate it from the Runner\Hook, maybe something like a Runner\Plugin would be optimal for creating and executing the objects at least for consistency reasons. What do you think?

[sebastianfeldmann]

Does it make sense to move the $this->beforeHook(); and $this->afterHook(); inside the else statement.
So if no action will be performed the before and after is skipped as well. Or does it make sense to actually call before and after anyway?