-
Notifications
You must be signed in to change notification settings - Fork 11.4k
[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
Conversation
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. |
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.
|
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: |
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 |
@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')
}
} |
Is this PR still being worked on? Otherwise it's best to close this and send it in at a later time. |
I was hoping for some feedback on whether the concept would be considered before I do too much more work on it. |
@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. |
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. |
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] AddsWithoutModelEvents
trait for running seeds without Model Events #39922)Model::boot*
andModel::initialize*
TestCase::setUpTraits
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
Trait that interacts with hooks
Application code
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.