Introduce IShutdownManager which allows us to order the shutdown func… #31857
Conversation
| } | ||
|
|
||
| public function run(): void { | ||
| foreach ($this->callbacks as $callbacks) { |
There was a problem hiding this comment.
This doesn't ensure a priority order according to the $priority variable:
php > $items = [];
php > $items[500] = 500;
php > $items[30] = 30;
php > $items[100] = 100;
php > foreach ($items as $item) { echo "$item\n";}
500
30
100
| * @return void | ||
| * @since 11.0.0 | ||
| */ | ||
| public function register(\Closure $callback, int $priority = self::LOW): void { |
There was a problem hiding this comment.
I'd prefer to use an interface instead of a closure. It should be easiear to test and also clarifies how tha callback should look like.
There was a problem hiding this comment.
Interface handler {
Public function Run() : void;
}
I don't See much benefit tbh
There was a problem hiding this comment.
For testing, it feels easier and cleaner to mock an interface even if you only need to check if the target method is being called. If you use a closure you'll need to create a custom implementation and use an external counter to test the same thing.
In addition, you'll need to document how the callback should be. You can use
function ($a = null, $b = null) {
if ($a !== null) $a->doSomething();
if ($b !== null) $b->doOtherThing();
}
That would be confusing on the long run because it seems to work, but it might not do what we expect.
There was a problem hiding this comment.
from my understanding with a closure all you need to do for testing is passing a test function:
$obj->register(function() use (&$called) { $called = true; });
$obj->doYourThingWithAllClosures();
$this->assertTrue($called);
| */ | ||
| interface IShutdownManager { | ||
|
|
||
| public const LOW = 1000; |
There was a problem hiding this comment.
I'd prefer to define now some additional priorities to minimize the changes we'll need to do to the interface in the future.
I only changed/grepped for usages of register_shutdown_function. destroy methods are called even later in the process from my understanding |
|
Quoting official documentation:
We shouldn't need to care about the destructor code as long as it's properly coded. The purpose of the destructor is a bit different. |
DeepDiver1975
force-pushed
the
feature/shutdown-handler
branch
from
3a437d5 to
2372c76
Compare
Codecov Report
@@ Coverage Diff @@
## master #31857 +/- ##
============================================
+ Coverage 63.27% 63.39% +0.12%
+ Complexity 18479 18461 -18
============================================
Files 1161 1162 +1
Lines 69377 69252 -125
Branches 1261 1261
============================================
+ Hits 43895 43904 +9
+ Misses 25112 24978 -134
Partials 370 370
Continue to review full report at Codecov.
|
DeepDiver1975
force-pushed
the
feature/shutdown-handler
branch
from
2372c76 to
7b3e9b7
Compare
|
If we aren't going to use an interface shutdownManager registration, we should document what is the expected signature of the closure function, whether it is going to be We should also document that under the same priority, the tasks will be done in the same order they were registered. |
DeepDiver1975
force-pushed
the
feature/shutdown-handler
branch
from
7b3e9b7 to
f75e4e6
Compare
|
@jvillafanez your objections have been addressed |
…e on this one .....
…tions
Description
php's register_shutdown_function offers no capability to define an execution order.
The IShutdownManager allows this.
Related Issue
#31851 and #31758 require this
Motivation and Context
We need a way to define that some shutdown functions are executed before the cleanup routines are triggered.
How Has This Been Tested?
Types of changes
Checklist: