Allow integer log levels #3

jonathanjfshaw · 2023-01-05T14:00:25Z

This is an alternative implementation to wimleers#1

Fixes #2

jonathanjfshaw · 2023-01-06T11:21:01Z

I've used union types for string|int, which require php 8. Not sure whether you want to make a php 8 release, or drop the explicit type.

colinodell

Overall I think this approach is on the right track! Besides a few nits, I think the only other thing this PR needs is some tests showing that the level mapping works properly.

With regards to PHP 7.4: I'm fine with dropping support for that and bumping the minimum version to ^8.0. We can then release this as a minor version update since the changes would be backward-compatible.

src/TestLogger.php

colinodell · 2023-01-06T20:34:51Z

src/TestLogger.php

-        if (\preg_match('/(.*)(Debug|Info|Notice|Warning|Error|Critical|Alert|Emergency)(.*)/', $method, $matches) > 0) {
+        $levelNames = \implode('|', \array_map('ucfirst', \array_keys($this->levelMap)));
+        if (\preg_match('/(.*)(' . $levelNames . ')(.*)/', $method, $matches) > 0) {


I'm guessing the idea behind this change is that a user may have instantiated the TestLogger without defining all of those levels in their map? If so, I think my preference would be to revert this change and add some validation to the constructor to ensure that the map contains all log levels. That would ensure that all of the magic methods defined on lines 15-54 are always available and guaranteed to never throw a BadMethodCallException.

Actually this was motivated by DRY. It smelled to me to repeat the name information twice in the class. I have kept this code, but added the requested validation to the __construct() method. Happy to remove this code if you prefer the simplicity of explicit repetition instead of DRY.

Co-authored-by: Colin O'Dell <[email protected]>

jonathanjfshaw · 2023-01-08T12:09:37Z

All tests and checks are green now, I think this is good to go. Only outstanding question is the DRY vs explicit issue in your review point discussed above.

jonathanjfshaw · 2023-03-09T20:42:20Z

@colinodell Any chance of this getting merged? Drupal are still keen to adopt this package.

colinodell · 2023-03-10T02:30:12Z

Sorry for the delay here! Life has been quite busy lately :)

After giving this a thorough review, I think I'd like to remove the $levelMap functionality. On the surface it seems potentially useful, but it leads to unexpected behavior like this:

$levelMap = [
    LogLevel::EMERGENCY => 0,
    LogLevel::ALERT => 1,
    LogLevel::CRITICAL => 2,
    LogLevel::ERROR => 3,
    LogLevel::WARNING => 4,
    LogLevel::NOTICE => 5,
    LogLevel::INFO => 6,
    LogLevel::DEBUG => 7,
];

$logger = new TestLogger($levelMap);

$logger->debug('foo');
$logger->log(LogLevel::DEBUG, 'bar');
$logger->log(7, 'baz');

$this->assertTrue($logger->hasDebugThatContains('foo')); // FAILS
$this->assertTrue($logger->hasDebugThatContains('bar')); // FAILS
$this->assertTrue($logger->hasDebugThatContains('baz')); // passes

After re-reading section 1.1 of PSR-3, it seems like 7 and LogLevel::DEBUG should pass into the logger as completely distinct, unrelated values. But the level map conflates the two. I can see how that mapping might be useful in a system that doesn't use LogLevel constants at all, but from this library's perspective, I think we should avoid the assumption that people will always use ONLY the constants or ONLY custom levels. So while I'm sure we could find a smart implementation that would work in all cases, I feel like doing so would contradict the intent of the spec.

Yes, this would mean that Drupal would not be able to leverage the magic methods. But all of the other methods would work just fine with int log levels, which I think would be sufficient.

WDYT?

If you think this is fine, I'd be happy to make the final changes to the PR and get this merged/released ASAP :)

jonathanjfshaw · 2023-03-10T12:08:24Z

Thanks for looking @colinodell . I think removing the level map and not being able to use the magic methods will cause Drupal no problems at all.

I've asked @wimleers the Drupal expert who detected the present issue to give this MR a final check.

colinodell · 2023-03-10T14:13:35Z

Sounds good! I've updated the code in this PR accordingly and look forward to hearing the results of that final check :)

colinodell

Ready to merge once feedback is received

wimleers · 2023-03-13T14:14:44Z

Yep, that seems reasonable to me too!

This PR looks fine to me, with the sole exception that bumping the required PHP version in anything else than a major version release AFAICT violates https://semver.org/ 😅 If you feel strongly about not tagging a new major version, fine, but … I'm fairly sure this would make the Drupal maintainers feel uneasy because if this happens again at some point in the future, then it technically breaks backwards compatibility for Drupal 🤓

wimleers · 2023-03-13T14:02:04Z

composer.json

-        "php": "^7.4 || ^8.0",
+        "php": "^8.0",


According to semver this means it should be version 2.0.0? 🤔

semver doesn't tie you to a particular PHP version support cycle, it's up to the project to determine what counts as a breaking API change and document it, and if PHP version increases don't count, then they don't.

Correct, a bump in dependencies is not a BC break - see https://github.com/semver/semver/blob/d460299638d3d2af6fcf809285dc6a469e15742a/semver.md#what-should-i-do-if-i-update-my-own-dependencies-without-changing-the-public-api

The only reason we might need to release this as a new major version would be if we BC in the public API. Although the API signature of TestLogger did change, all of those changes are backward-compatible:

The class is final, so we don't need to worry about child classes suddenly becoming incompatible

The only type changes were parameter type widening. The new string|int types will continue to accept string inputs with no changes needed by end users.

I'm fairly sure this would make the Drupal maintainers feel uneasy because if this happens again at some point in the future, then it technically breaks backwards compatibility for Drupal

This shouldn't be an issue as Composer will prevent the installation of newer versions with incompatible dependencies.

For example, this change will be released as v1.2.0, so I imagine Drupal will use a constraint like ^1.2. If I were to release v1.3.0 in the future with a new minimum PHP version of 8.2.0, Composer would still allow v1.2.0 on PHP 8.0 and 8.1 - only users on 8.2 could install v1.3.0. And because it would be a minor version bump and we follow semver, there would be no breaking changes to the API. This means that both versions would function identically as far as Drupal is concerned.

Bumping dependencies (including minimum PHP versions) in a minor release is done by many other large PHP projects, including Doctrine: https://www.doctrine-project.org/2017/07/25/php-7.1-requirement-and-composer.html

TL;DR: Everything will "just work" :)

Allow integer log levels

8233f95

jonathanjfshaw mentioned this pull request Jan 5, 2023

Stop assuming LogLevel::* levels, because \Psr\Log\LoggerInterface::log()does not require it either #2

Closed

Fix coding standards

3b01f4d

colinodell reviewed Jan 6, 2023

View reviewed changes

jonathanjfshaw and others added 11 commits January 8, 2023 10:54

Test on php 8 only

6cad040

Require php 8

8dcba68

levelMap private

27d1c31

Co-authored-by: Colin O'Dell <[email protected]>

Fix review points in TestLogger

7ee4d26

Add a test

436295a

Update php version in readme

f04316c

Whitespace in test

7edf059

Split test

18ca37c

Fix coding standards

3f3cdf8

More coding standards

6a941ab

Fix coding standards in test

650d13d

Allow level maps to define keys in any order

33ea077

colinodell added 3 commits March 10, 2023 09:08

Remove levelMap functionality

17f02d9

Throw an exception on unsupported log levels

79ce374

Minor code tweaks

45ab7eb

Update CHANGELOG

4faa982

colinodell approved these changes Mar 10, 2023

View reviewed changes

wimleers approved these changes Mar 13, 2023

View reviewed changes

colinodell merged commit 28b1e4a into colinodell:main Mar 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow integer log levels #3

Allow integer log levels #3

jonathanjfshaw commented Jan 5, 2023 •

edited

Loading

jonathanjfshaw commented Jan 6, 2023

colinodell left a comment •

edited

Loading

colinodell Jan 6, 2023

jonathanjfshaw Jan 8, 2023

jonathanjfshaw commented Jan 8, 2023

jonathanjfshaw commented Mar 9, 2023

colinodell commented Mar 10, 2023

jonathanjfshaw commented Mar 10, 2023

colinodell commented Mar 10, 2023

colinodell left a comment

wimleers commented Mar 13, 2023

wimleers Mar 13, 2023

catch56 Mar 14, 2023

colinodell Mar 14, 2023

Allow integer log levels #3

Allow integer log levels #3

Conversation

jonathanjfshaw commented Jan 5, 2023 • edited Loading

jonathanjfshaw commented Jan 6, 2023

colinodell left a comment • edited Loading

Choose a reason for hiding this comment

colinodell Jan 6, 2023

Choose a reason for hiding this comment

jonathanjfshaw Jan 8, 2023

Choose a reason for hiding this comment

jonathanjfshaw commented Jan 8, 2023

jonathanjfshaw commented Mar 9, 2023

colinodell commented Mar 10, 2023

jonathanjfshaw commented Mar 10, 2023

colinodell commented Mar 10, 2023

colinodell left a comment

Choose a reason for hiding this comment

wimleers commented Mar 13, 2023

wimleers Mar 13, 2023

Choose a reason for hiding this comment

catch56 Mar 14, 2023

Choose a reason for hiding this comment

colinodell Mar 14, 2023

Choose a reason for hiding this comment

jonathanjfshaw commented Jan 5, 2023 •

edited

Loading

colinodell left a comment •

edited

Loading