docs: replace types in `Logger` #6638

ddevsr · 2022-10-06T14:53:37Z

Description

See Dev: replace type mixed with more specific types #6310
Supersedes docs: replace types in Logger #6633

Checklist:

Securely signed commits
Component(s) with PHPDoc blocks, only if necessary or adds value
Unit testing, with >80% coverage
User guide updated
Conforms to style guide

kenjis · 2022-10-07T09:53:13Z

tests/system/Log/Handlers/FileHandlerTest.php

+        $date     = date('Y-m-d');
+        $expected = 'log-' . $date . '.php';
+        $logger->handle('info', 'This is a test log');


These codes are time-dependent.
If date('Y-m-d') is called on today 23:59:59.9999 and $logger->handle() is called on tomorrow 0:00:00,
This test fails.

Yes, rare moment to get difference ms. If want real same time must give parameter to set name file manual.

Now FileHandler::handle() is using date(). We should use Time instead of date().
Then we can stop time during test execution.

MGatner · 2022-10-07T10:50:16Z

system/Log/Handlers/ChromeLoggerHandler.php

@@ -13,6 +13,7 @@

 use CodeIgniter\HTTP\ResponseInterface;
 use Config\Services;
+use Stringable;


Stringable comes with PHP 8, we can't use it yet. 😢

Yes, that's true. But a use statement won't error on an unknown class.

PHP-CS-Fixer automate use statement if set string|\Stringable

As long as supporting PHP 7.4, we should not use Stringable.
It means a normal class Stringable and does not make sense.
See https://3v4l.org/TF3Ct

paulbalandan · 2022-10-07T15:12:39Z

tests/system/Log/Handlers/FileHandlerTest.php

+    public function testHandleWithInteger()
+    {
+        $config                                       = new LoggerConfig();
+        $config->handlers[TestHandler::class]['path'] = $this->start;
+        $logger                                       = new MockFileLogger($config->handlers[TestHandler::class]);
+
+        $logger->setDateFormat('Y-m-d');
+        $date     = date('Y-m-d');
+        $expected = 'log-' . $date . '.log';
+        $logger->handle('info', 123456);
+
+        $fp     = fopen($config->handlers[TestHandler::class]['path'] . $expected, 'rb');
+        $result = stream_get_contents($fp);
+        fclose($fp);
+
+        // did the log file get created?
+        $expectedResult = 'INFO - ' . $date . " --> 123456\n";
+        $this->assertSame($expectedResult, $result);
+    }
+
+    public function testHandleWithArray()
+    {
+        $config                                       = new LoggerConfig();
+        $config->handlers[TestHandler::class]['path'] = $this->start;
+        $logger                                       = new MockFileLogger($config->handlers[TestHandler::class]);
+
+        $logger->setDateFormat('Y-m-d');
+        $date    = date('Y-m-d');
+        $arrData = [
+            'firstName' => 'John',
+            'lastName'  => 'Doe',
+        ];
+        $expected = 'log-' . $date . '.log';
+        $logger->handle('info', print_r($arrData, true));
+
+        $fp     = fopen($config->handlers[TestHandler::class]['path'] . $expected, 'rb');
+        $result = stream_get_contents($fp);
+        fclose($fp);
+
+        // did the log file get created?
+        $expectedResult = 'INFO - ' . $date . " --> Array\n(
+    [firstName] => John
+    [lastName] => Doe\n)\n\n";
+        $this->assertSame($expectedResult, $result);
+    }
+
+    public function testHandleWithObject()
+    {
+        $config                                       = new LoggerConfig();
+        $config->handlers[TestHandler::class]['path'] = $this->start;
+        $logger                                       = new MockFileLogger($config->handlers[TestHandler::class]);
+
+        $logger->setDateFormat('Y-m-d');
+        $date = date('Y-m-d');
+
+        $obj            = new stdClass();
+        $obj->firstName = 'John';
+        $obj->lastName  = 'Doe';
+
+        $expected = 'log-' . $date . '.log';
+        $logger->handle('info', print_r($obj, true));
+
+        $fp     = fopen($config->handlers[TestHandler::class]['path'] . $expected, 'rb');
+        $result = stream_get_contents($fp);
+        fclose($fp);
+
+        // did the log file get created?
+        $expectedResult = 'INFO - ' . $date . " --> stdClass Object\n(
+    [firstName] => John
+    [lastName] => Doe\n)\n\n";
+        $this->assertSame($expectedResult, $result);
+    }
+


This formatting of $message was from commit 4fcb760. However, I think this is in contradiction to this line:

CodeIgniter4/system/Log/Logger.php

Line 44 in 53f17e8

* The message MUST be a string or object implementing __toString().

Also, this is not compliant with PSR-3 considering that the Logger is implementing Psr\Log\LoggerInterface.

All in all, I believe this behavior is a bug to begin with.

What about now?

I did not know that.

log_message('error', $this); // TypeError $this->logger->error($this); // Okay. but PHPStan error

------ --------------------------------------------------------------------------------------------------------------------- Line app/Controllers/Home.php ------ --------------------------------------------------------------------------------------------------------------------- 9 Parameter #2 $message of function log_message expects string, $this(App\Controllers\Home) given. 10 Parameter #1 $message of method Psr\Log\LoggerInterface::error() expects string, $this(App\Controllers\Home) given. ------ ---------------------------------------------------------------------------------------------------------------------

Also, this is not compliant with PSR-3 considering that the Logger is implementing Psr\Log\LoggerInterface.
All in all, I believe this behavior is a bug to begin with.

Yes, but fixing this may break existing apps.

@kenjis Before changes in this PR appear PHP 2 errors. With condition now PR, what about ignoring this 2 error phpstan?

Ignoring PHPStan errors is very bad practice.
We should remove phpstan-baseline.neon.dist entries as possible.

Please revert.

system/Common.php

system/Log/Handlers/ChromeLoggerHandler.php

system/Log/Handlers/ErrorlogHandler.php

system/Log/Handlers/FileHandler.php

system/Log/Handlers/HandlerInterface.php

system/Log/Logger.php

system/Log/Handlers/ChromeLoggerHandler.php

system/Log/Logger.php

kenjis

It is recommended to only change the PHPDoc types. Because this PR title is docs: replace types in Logger.
If you want to change any implementation, it is better to send another PR.

paulbalandan

Let's minimize for now the changes in this PR to the docblock changes, otherwise those "other" changes would be a blocker for the merge.

Also, please squash the unrelated "trial-and-error" commits into the others. We don't want to show an unordered history of the file.

system/Log/Logger.php

system/Test/Mock/MockLogger.php

paulbalandan · 2022-10-09T14:43:41Z

tests/system/Log/Handlers/FileHandlerTest.php

+    public function testHandleWithInteger()
+    {
+        $config                                       = new LoggerConfig();
+        $config->handlers[TestHandler::class]['path'] = $this->start;
+        $logger                                       = new MockFileLogger($config->handlers[TestHandler::class]);
+
+        $logger->setDateFormat('Y-m-d');
+        $date     = date('Y-m-d');
+        $expected = 'log-' . $date . '.log';
+        $logger->handle('info', 123456);
+
+        $fp     = fopen($config->handlers[TestHandler::class]['path'] . $expected, 'rb');
+        $result = stream_get_contents($fp);
+        fclose($fp);
+
+        // did the log file get created?
+        $expectedResult = 'INFO - ' . $date . " --> 123456\n";
+        $this->assertSame($expectedResult, $result);
+    }
+
+    public function testHandleWithArray()
+    {
+        $config                                       = new LoggerConfig();
+        $config->handlers[TestHandler::class]['path'] = $this->start;
+        $logger                                       = new MockFileLogger($config->handlers[TestHandler::class]);
+
+        $logger->setDateFormat('Y-m-d');
+        $date    = date('Y-m-d');
+        $arrData = [
+            'firstName' => 'John',
+            'lastName'  => 'Doe',
+        ];
+        $expected = 'log-' . $date . '.log';
+        $logger->handle('info', print_r($arrData, true));
+
+        $fp     = fopen($config->handlers[TestHandler::class]['path'] . $expected, 'rb');
+        $result = stream_get_contents($fp);
+        fclose($fp);
+
+        // did the log file get created?
+        $expectedResult = 'INFO - ' . $date . " --> Array\n(
+    [firstName] => John
+    [lastName] => Doe\n)\n\n";
+        $this->assertSame($expectedResult, $result);
+    }
+
+    public function testHandleWithObject()
+    {
+        $config                                       = new LoggerConfig();
+        $config->handlers[TestHandler::class]['path'] = $this->start;
+        $logger                                       = new MockFileLogger($config->handlers[TestHandler::class]);
+
+        $logger->setDateFormat('Y-m-d');
+        $date = date('Y-m-d');
+
+        $obj            = new stdClass();
+        $obj->firstName = 'John';
+        $obj->lastName  = 'Doe';
+
+        $expected = 'log-' . $date . '.log';
+        $logger->handle('info', print_r($obj, true));
+
+        $fp     = fopen($config->handlers[TestHandler::class]['path'] . $expected, 'rb');
+        $result = stream_get_contents($fp);
+        fclose($fp);
+
+        // did the log file get created?
+        $expectedResult = 'INFO - ' . $date . " --> stdClass Object\n(
+    [firstName] => John
+    [lastName] => Doe\n)\n\n";
+        $this->assertSame($expectedResult, $result);
+    }
+


Please revert.

kenjis

Please squash commits.

tests: increase coverage tests in Logger use dedicated clean_path refactor: implement __toString() in Logger docs: replace param string message to phpdocblock docs: replace param string message to phpdocblock docs: replace types in Logger Co-authored-by: kenjis <[email protected]> docs: replace types in Logger docs: replace types in Logger revert refactoring in Logger

MGatner

Thanks @ddevrs

paulbalandan · 2022-10-11T02:09:02Z

Thank you, @ddevsr

kenjis reviewed Oct 7, 2022

View reviewed changes

MGatner requested changes Oct 7, 2022

View reviewed changes

paulbalandan reviewed Oct 7, 2022

View reviewed changes