[9.x] Unify trait set up/tear down and other hooks under a Hook implementation

[inxilpro]

There are a number of places where we use the pattern of class_uses_recursive(static::class) to conditionally initialize traits or otherwise hook into the core logic of a class.

You can see this in:

• WithoutModelEvents (just added in [9.x] Adds WithoutModelEvents trait for running seeds without Model Events #39922)
• Model::boot* and Model::initialize*
• TestCase::setUpTraits
• …and a handful of other places within the framework

There have also been attempts to provide more configurable access to this logic (most recently #39883 and at least as far back as #21126). This is particularly notable with TestCase::setUpTraits, because it maintains a hard-coded list of traits instead of relying on a convention like Eloquent models.

This PR is an exploration into providing a unified Hook concept that covers all these scenarios and provides a standardized way to use this behavior both in application code and future framework code.

The primary API looks like:

Class that supports hooks

class Demo
{
  use Hookable;
  
  public function __construct()
  {
    // Just call a hook at a specific execution point
    $this->runHooks('initialize', $this);
  }
  
  public function handle()
  {
    $this->runHooks('handle', $this, function() {
      // Call a hook and do some work (allows for pre/post hooks)
      DB::table('demo')->insert(['foo' => 'bar']);
    });
  }
}

Trait that interacts with hooks

trait LogDatabaseQueries
{
  public Collection $queries;
  
  // Any hook methods must type-hint Hook as the return type
  public static function getInitializeHook(): Hook
  {
    return Hook::make('initialize', function() {
      $this->queries = new Collection();
    });
  }

  public static function getHandleHook(): Hook
  {
    // Hooks can be registered at different priority levels
    return Hook::highPriority('handle', function () {
      // This runs at the beginning of "handle"
      DB::enableQueryLog();
      DB::flushQueryLog();
      
      // Return a callback if you want to perform "cleanup" after the hook
      return function() {
        $this->queries->push(...DB::getQueryLog());
      };
    });
  }
}

Application code

class AppDemo extends Demo
{
  // Nothing is necessary beyond adding the trait, thanks to hooks!
  use LogDatabaseQueries;
}

This implementation takes into consideration static and non-static hooks, and also provides a backwards-compatibility layer so that you can easily register hooks that also support a prefix + TraitName naming convention.

(I've decided to use the Hook return type for hook discovery, as this makes the change 100% opt-in (where using a naming convention could potentially interfere with existing methods). I know there's generally a hesitance to rely on type-hints in the framework, so I'm open to revisiting this decision.)

As a proof of concept I've implemented the WithoutModelEvents trait using hooks. I've also identified some other areas in the framework that would benefit from hooks. I've decided to submit this as a draft PR in its current state for feedback before I spend much more time on it. With support, I'll add more tests and migrate more code over to hooks.

[inxilpro]

I just implemented hooks in the Eloquent Model class as well, and I think this makes the most compelling case for this change:

😄

[DarkGhostHunter]

because it maintains a hard-coded list of traits instead of relying on a convention like Eloquent models.

Mainly because, as I infer, these need to run in a particular order when present.

[inxilpro]

Mainly because, as I infer, these need to run in a particular order when present.

This new hook implementation allows for registering hooks at specific priority levels, which means all the existing test case hooks run in the same order.

[DarkGhostHunter]

Lol didn’t read that. I would recommend to use bitwise constants so you can mix it instead of adding them. Italo Baeza C.

…

El 13-12-2021, a la(s) 13:30, Chris Morrell ***@***.***> escribió:  Mainly because, as I infer, these need to run in a particular order when present. This new hook implementation allows for registering hooks at specific priority levels, which means all the existing test case hooks run in the same order. — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

[inxilpro]

I would recommend to use bitwise constants so you can mix it instead of adding them.

Can you give an example? I don't see how you would ever want to mix priorities.

The system, as is stands, sets three priority conventions: 100 is high priority, 200 is normal, and 300 is low. You can use the shortcuts to add a hook to one of those three buckets, or use any number you choose (lower numbers are processed first).

[DarkGhostHunter]

I would recommend to use bitwise constants so you can mix it instead of adding them.

Can you give an example? I don't see how you would ever want to mix priorities.

The system, as is stands, sets three priority conventions: 100 is high priority, 200 is normal, and 300 is low. You can use the shortcuts to add a hook to one of those three buckets, or use any number you choose (lower numbers are processed first).

For example, if I want to only priorice high and normal priority, I get 300, which is low priority.

When using anything that requiere mixing two or more options, always use bitwise. Like on json_encode.

[inxilpro]

@DarkGhostHunter maybe I didn't make the priority concept clear. It's meant to be used to indicate where in the order of things the hook should run. So, for example:

trait Foo
{
  public static function registerFooHook(): Hook
  {
    return Hook::lowPriority('boot', fn() => dump('foo'));
  }
}

trait Bar
{
  public static function registerBarHook(): Hook
  {
    return Hook::make('boot', fn() => dump('bar'));
  }
}

trait Baz
{
  public static function registerBazHook(): Hook
  {
    return Hook::highPriority('boot', fn() => dump('baz'));
  }
}

class Demo
{
  use Foo, Bar, Baz;

  public function boot()
  {
    // will dump "baz", then "bar", and finally "foo"
    $this->runHooks('boot')
  }
}

[driesvints]

Is this PR still being worked on? Otherwise it's best to close this and send it in at a later time.

[inxilpro]

Is this PR still being worked on?

I was hoping for some feedback on whether the concept would be considered before I do too much more work on it.

[driesvints]

@inxilpro okay. Please remember that Taylor doesn't sees draft PRs. So mark your PR as ready to review when you want a fresh review.

[taylorotwell]

I dunno - I think trying to maintain this in a flexible way that makes everyone happy isn't really something I want to take on right now.