8000 [9.x] Unify trait set up/tear down and other hooks under a `Hook` implementation by inxilpro · Pull Request #39947 · laravel/framework · GitHub
[go: up one dir, main page]

Skip to content

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

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

Closed
wants to merge 19 commits into from

Conversation

inxilpro
Copy link
Contributor
@inxilpro inxilpro commented Dec 8, 2021

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:

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
Copy link
Contributor Author
inxilpro commented Dec 8, 2021

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

Screen Shot 2021-12-08 at 4 53 31 PM

😄

@DarkGhostHunter
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor
DarkGhostHunter commented Dec 13, 2021 via email

@inxilpro
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

@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
Copy link
Member

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

@inxilpro
Copy link
Contributor Author
inxilpro commented Jan 3, 2022

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 driesvints marked this pull request as ready for review January 4, 2022 08:55
@driesvints
Copy link
Member

@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
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants
0