docs: replace types in `Logger` #6633

ddevsr · 2022-10-06T02:42:43Z

Description
See #6310.

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

paulbalandan · 2022-10-06T06:45:38Z

system/Log/Handlers/ErrorlogHandler.php

+        if (! is_string($message)) {
+            $message = print_r($message, true);
+        }
+


This seems to be more of a feature enhancement rather than a simple docblock fix, so this is proper for 4.3.

However, I'm not convinced that we should allow array|int|object as additions to possible types for $message. If we are to look at the Psr\Log specification, the $message is only limited to string|\Stringable. Plus, I find it unsightly for an array-type message to be just a print_r() of itself rather than it being properly formatted into a string.

Okay, we can accept array|int|object addition in function handle?

Yes, $message type is string|\Stringable.
https://github.com/php-fig/log/blob/fe5ea303b0887d5caefd3d431c3e61ad47037001/src/LoggerInterface.php#L30

I disagree, changing to a PSR-3 compatible logger may break apps.

@kenjis I found this when git cherry-pick --continue

------ --------------------------------------------------------------------------------------------------------------------- Line system\CLI\Console.php ------ --------------------------------------------------------------------------------------------------------------------- Ignored error pattern #^Strict comparison using === between array<mixed, mixed> and array{} will always evaluate to false\.$# in path D:\Project\laragon\www\ci4\system\CLI\Console.php was not matched in reported errors. 71 Strict comparison using === between array and array{} will always evaluate to false. ------ --------------------------------------------------------------------------------------------------------------------- [ERROR] Found 2 errors Fix the phpstan error(s) before commit.

I fix in phpstan-baseline.neon.dist #^Strict comparison using === between array<mixed, mixed> and array{} will always evaluate to false to #^Strict comparison using === between array and array{} will always evaluate to false

I've fixed the error.
See #6627

In Psr\Log\LoggerInterface still with string only (v1.1.4), I think we set now string only. \Stringable still under develop

Stringable can be used in PHP 8.0 or later.

This comment:

CodeIgniter4/system/Log/Logger.php

Line 23 in 15fa3f1

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

kenjis · 2022-10-06T10:17:04Z

system/Log/Handlers/ErrorlogHandler.php

+        if (! is_string($message)) {
+            $message = print_r($message, true);
+        }


This change is not acceptable.

CodeIgniter4/system/Log/Logger.php

Line 23 in 15fa3f1

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

Still in 4.3 branch if remove this?

If we change, it would be incompatible with PSR-3.

Okay, i will send PR another with develop branch. Many phpstan error in my branch

------ ------------------------------------------------------------------------------------------------------------------- Line system\Cache\Handlers\MemcachedHandler.php ------ ------------------------------------------------------------------------------------------------------------------- Ignored error pattern #^Method MemcachePool\:\:decrement invoked with 4 parameters, 1\-2 required\.$# in path D:\Project\laragon\www\ci4\system\Cache\Handlers\MemcachedHandler.php was not matched in reported errors. Ignored error pattern #^Method MemcachePool\:\:increment invoked with 4 parameters, 1\-2 required\.$# in path D:\Project\laragon\www\ci4\system\Cache\Handlers\MemcachedHandler.php was not matched in reported errors. ------ ------------------------------------------------------------------------------------------------------------------- ------ ------------------------------------------------------------------------------------------------- Line system\HTTP\URI.php ------ ------------------------------------------------------------------------------------------------- Ignored error pattern #^Cannot unset offset 'path' on array{host: non-empty-string}\.$# in path D:\Project\laragon\www\ci4\system\HTTP\URI.php was not matched in reported errors. ------ ------------------------------------------------------------------------------------------------- ------ ----------------------------------------------------------------------------------------------- Line system\Helpers\text_helper.php ------ ----------------------------------------------------------------------------------------------- Ignored error pattern #^Variable \$pool might not be defined\.$# in path D:\Project\laragon\www\ci4\system\Helpers\text_helper.php was not matched in reported errors. ------ ----------------------------------------------------------------------------------------------- ------ ------------------------------------------------------------------------------ Line system\Model.php ------ ------------------------------------------------------------------------------ 684 Offset 'data' on array on left side of ?? always exists and is not nullable. 710 Offset 'data' on array on left side of ?? always exists and is not nullable. ------ ------------------------------------------------------------------------------ [ERROR] Found 7 errors Fix the phpstan error(s) before commit.

@kenjis I create new branch with uptodate remote branch develop. But have result phpstan error

------ ---------------------------------------------------------------------------- Line system\Helpers\array_helper.php ------ ---------------------------------------------------------------------------- 51 Result of && is always false. 51 Strict comparison using !== between 0 and 0 will always evaluate to false. ------ ---------------------------------------------------------------------------- [ERROR] Found 2 errors Fix the phpstan error(s) before commit.

Ohh mistake, 1 hour ago phpstan release 1.8.8 and in this PR using 1.8.7 @kenjis

kenjis

I think $message should be strring or stringable.

ddevsr · 2022-10-06T13:03:39Z

I think $message should be strring or stringable.

in handler function too?

kenjis · 2022-10-06T13:16:45Z

in handler function too?

Yes. handler() gets $message from log_message().

paulbalandan reviewed Oct 6, 2022

View reviewed changes

kenjis added the wrong branch PRs sent to wrong branch label Oct 6, 2022

paulbalandan changed the base branch from develop to 4.3 October 6, 2022 08:49

ddevsr added 6 commits October 6, 2022 15:59

docs: replace type in Logger

c05c4b6

tests: added logger with ext PHP

0ae8cba

docs: replace types in Logger

4dcd6c6

docs: replace types in Logger

fa72676

tests: increase coverage Logger

dded2de

docs: replace types in Logger

f7c0c8e

paulbalandan removed the wrong branch PRs sent to wrong branch label Oct 6, 2022

kenjis reviewed Oct 6, 2022

View reviewed changes

kenjis requested changes Oct 6, 2022

View reviewed changes

ddevsr closed this Oct 6, 2022

ddevsr deleted the docs-logger branch October 6, 2022 14:48

ddevsr mentioned this pull request Oct 6, 2022

docs: replace types in Logger #6638

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docs: replace types in `Logger` #6633

docs: replace types in `Logger` #6633

ddevsr commented Oct 6, 2022

paulbalandan Oct 6, 2022

ddevsr Oct 6, 2022

kenjis Oct 6, 2022

kenjis Oct 6, 2022

ddevsr Oct 6, 2022 •

edited

Loading

kenjis Oct 6, 2022

ddevsr Oct 6, 2022

kenjis Oct 6, 2022

paulbalandan Oct 6, 2022

kenjis Oct 6, 2022

ddevsr Oct 6, 2022

kenjis Oct 6, 2022

ddevsr Oct 6, 2022

ddevsr Oct 6, 2022

ddevsr Oct 6, 2022

kenjis left a comment

ddevsr commented Oct 6, 2022

kenjis commented Oct 6, 2022

docs: replace types in Logger #6633

docs: replace types in Logger #6633

Conversation

ddevsr commented Oct 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ddevsr Oct 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kenjis left a comment

Choose a reason for hiding this comment

ddevsr commented Oct 6, 2022

kenjis commented Oct 6, 2022

docs: replace types in `Logger` #6633

docs: replace types in `Logger` #6633

ddevsr Oct 6, 2022 •

edited

Loading