Userbag.co.uk

In SaaS or similar applications, there often comes a point when you want to start adding custom or tweaked functionality for clients without the codebase itself diverging. One of the most common solutions to this is to use hooks.

Hooks effectively allow you to make certain parts of your applications functionality open to modification by another part. My own usage of this for instance is allowing a single white label configuration file to adjust (in some cases quite deeply) how the larger application works. WordPress is probably the most well known for using this approach in order to allow its plugins and themes to easily interact with WordPress’s functionalty, without needing to make any changes WordPress’s own code.

To support this functionally in a Laravel application, for the last few years I have been using the wonderful esemve/Hook library by Bence Kádár. Unfortunately the library now appears to be mostly inactive – most critically lacking support for Laravel 8 (as well as PHP8 itself in some areas).

As such, with an growing requirement for additional functionality and updates, I decided to take the plunge and create a new maintained fork of the library.

Although for simple use cases coinvestor/larahook can be used as a drop in replacement for the original esemve/hook, I utilised the opportunity of it being a clean break to make some more involved changes to the libraries functionality.

The most important changes are listed below.

Laravel 8 and PHP 8 compatibility plus auto-discovery support.

The library has been updated to work with the latest version of Laraval, as well as to make use of the newer package auto-discovery features meaning you will no longer need to update your app.php directly.

Retired the initial content parameter, and replaced it with $useCallbackAsFirstListener.

In the original version of the library, you needed to specify both a call-back (to run when the hook was not being listened to) as well as optionally a default $output value to be passed in as the 4th parameter to the get hook method. This lead to some confusing code where a hook would need to invoke the original call-back directly to get a default value (where output was null), but if a second hook had run previously, may instead include data in the $output the hook would need to be aware of.

To simplify this I changed the 4th parameter on get to instead be a Boolean called $useCallbackAsFirstListener, as such by setting this as true, the hooks default callback will always run and pass its value in as the $output value for every subsequent listener. As such listener logic can be simplified to always expect to be working with the value of output. For now this option is left as false, as in the case where the default hook is taking an action (say sending an email) this behaviour would not be desired, and so for safety must be specifically enabled.

Listeners at the same hook at the same priority will no longer overwrite each other.

Unlike the original version of the library, lara-hook will allow multiple hooks to be registered on the same event at the same priority level. In the case this happens the hook listeners will be run in the order they are registered.

If you are making use of the original functionality where hooks at the same priority overwrite each other, code changes will be needed.

Support for falsely return values.

In our application there were a number of cases where we’d wanted a hook listener to return a falsey value back to an underlying function. This was previously not possible, as a hook returning false would trigger an abort – causing the hook to return the default value (rather than the falsey value itself).

This is no longer the case in larahook, meaning the Hook:stop(); method will now have to be used directly where a hook does need to abort, as 0/false/nulls will simply be returned from the hook like any other value.

New Methods: getListeners, removeListener and removeListeners.

Listeners can now be unregistered, both individually as well as all listeners for a specific hook.

Additionally a full list of listeners on a hook can be returned using the getListeners method.

Test and bugfixes.

In addition to the functionality changes, I also spent some time adding unit tests and basic CI for the library. As is always the case when adding tests I managed to find and fix a number of minor bugs and edge cases across the library.

If you’d like to swap over to the new library, please have a look at our repo at https://github.com/CoInvestor/larahook/

Given the challenge I had in my own recent googling, I thought it would be worth while putting together this quick blog post to provide a simple/direct answer as to how to configure a GitHub Action on a pull request, so that CI tasks are only run when a certain reviewer has been added.

Quick background

• We had a large application with a significant test suite.
• We try to create our pull requests early in the development of a feature to aid visibility.
• We need all pull requests to require a successful run of the test suite before they can be merged.
• Finally: We didn’t want to keep run the test suite over and over unnecessarily (for example every time the branch is pushed to while being developed).

As such our preferred mechanism was for the test suite to only run when a specific reviewer is attached to the pull request. In our case the reviewer is our service account.

Solution

Lets assume our existing Github action looked something like the below. The Github action is set to run whenever a new PR is opened, a new reviewer is added or when new commits are pushed in to the pull request itself.

name: ci

on:
  pull_request:
    types: [ opened, review_requested, synchronize ]

jobs:
  run_our_ci:
    runs-on: ubuntu-latest
    steps:
    - name: Run our CI
      run: |
        echo "Hello world!"

Currently this will be run every time one of those actions takes place, which isn’t something we want. The basic solution is to add a condition to the task, such as the below.

if: contains(github.event.pull_request.requested_reviewers.*.login, '<CI_SERVICE_ACCOUNT>')

The above will cause triggered actions to skip whenever the condition is not met. The condition in this case being that one of the attached reviewers has a login name matching <CI_SERVICE_ACCOUNT>.

The same can be achieved with a label instead using something along the lines of

if: contains(github.event.pull_request.labels.*.name, '<LABEL_NAME>')

With the above condition added to your task, your YAML should now resemble the below

on:
  pull_request:
    types: [ opened, review_requested, synchronize ]

jobs:
  run_our_ci:
    if: contains(github.event.pull_request.requested_reviewers.*.login, '<CI_SERVICE_ACCOUNT>')
    runs-on: ubuntu-latest
    steps:
    - name: Run our CI
      run: |
        echo "Hello world!"

If you now open a pull request with the above, you will note that each commit continues to get a “tick” next to it – but when you click in for detail the task itself will have skipped.

It is also worth being aware that making a base task skip will also cause any tasks that depend on it to skip as well (ie. any referencing the task as a “needs:”. This is useful in that you don’t have to add the “if” to every task, but can also be annoying given that tasks are considered to been run successfully (and skipped) rather than having not run at all by the Github protected branch feature.

This presented us with an issue, as due to the protection rules seeing “skip” as a “success” state, the branch protection rules will now happily let you merge a branch without any CI being run on it, something we certainly didn’t want.

To work around this (although not ideal), and ensure the branch protections do enforce that the full test suite has run before allowing a merge, we can add an inverse version of the job such as the below;

no_ci_has_run:
    if: "!contains(github.event.pull_request.requested_reviewers.*.login, '<CI_SERVICE_ACCOUNT>')"
    runs-on: ubuntu-latest
    steps:
    - name: "No tests have run"
      run: |
        exit 1

This task will explicitly fail whenever the main CI steps have been skipped. By adding this as a “required” check in your protected branch settings, you can therefore ensure that people can only merge the branch when the the full test suite has been run & passed on the given pull request.

This works due to the no_ci_has_run task “skipping” when the test suite is being run – which as mentioned above the branch protection feature sees as a success.

TLDR

The combined github actions YAML may look something like the below.

• The real CI task will only run when CI_SERVICE_ACCOUNT is added as a reviewer.
• The no_ci_has_run task is run whenever it is not, preventing the branch from being mergeable (If you use branch protection)
• Multi step tasks will automatically skip if a previous task’s conditional fails.

name: ci

on:
  pull_request:
    types: [opened, review_requested, synchronize ]

jobs:
  run_our_ci:
    if: contains(github.event.pull_request.requested_reviewers.*.login, '<CI_SERVICE_ACCOUNT>')
    runs-on: ubuntu-latest
    steps:
    - name: Run our CI
      run: |
        echo "Hello world!"
 no_ci_has_run:
    if: "!contains(github.event.pull_request.requested_reviewers.*.login, '<CI_SERVICE_ACCOUNT>')"
    runs-on: ubuntu-latest
    steps:
    - name: "No tests have run"
      run: |
        exit 1