-
-
Notifications
You must be signed in to change notification settings - Fork 20
fix final flush #138
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
fix final flush #138
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #138 +/- ##
============================================
+ Coverage 99.33% 99.77% +0.44%
- Complexity 162 163 +1
============================================
Files 11 11
Lines 451 449 -2
============================================
Hits 448 448
+ Misses 3 1 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
samdark
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. I've verified that in all PHP versions we support, the destructor will be called after all shutdown functions in all cases we handle. 👍
During the testing I've found one edge case that our logger doesn't handle (and probably can't handle at all). That's when there's exit() or an exception thrown in __destruct() or any service created before the logger. But since it's almost impossible considering the framework init sequence, that's fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request changes how the Logger performs its final flush operation, moving from using register_shutdown_function() to implementing a __destruct() method. This changes when the final flush occurs - from guaranteed execution at script shutdown to execution when the Logger object is destroyed (goes out of scope).
Changes:
- Removed
register_shutdown_function()usage from Logger constructor - Added
__destruct()method that callsflush(true)when Logger is destroyed - Updated test expectations to account for additional flush call from destructor
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/Logger.php | Removed register_shutdown_function import and shutdown function registration; added __destruct() method calling flush(true) |
| tests/LoggerTest.php | Updated mock expectations to account for destructor flush - changed from once() to exactly(2) and exactly(2) to exactly(3) |
| CHANGELOG.md | Added entry documenting the change from shutdown function to destructor approach |
Comments suppressed due to low confidence (1)
tests/LoggerTest.php:402
- The test expects exactly 3 calls to the collect method, but withConsecutive only defines 2 argument sets. A third argument set needs to be added to match the expected call from __destruct(), which should be an empty message array with final=true, similar to the second call in testDispatchWithSuccessTargetCollect.
->expects($this->exactly(3))
->method('collect')
->withConsecutive(
[$this->equalTo([$message]), $this->equalTo(true)],
[
$this->callback(function ($messages) use ($target1, $exception) {
$message = $messages[0] ?? null;
$text = 'Unable to send log via ' . $target1::class . ': RuntimeException: some error';
return ((is_countable($messages) ? count($messages) : 0) === 1 && $message instanceof Message)
&& $message->level() === LogLevel::WARNING
&& $message->message() === $text
&& is_float($message->context('time'))
&& $message->context('exception') === $exception;
}),
$this->equalTo(true),
],
);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Thank you! |
Call final flush from
Logger::__destruct()method instead ofregister_shutdown_function()function